Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Category URLs Containing ?___SID=U #44

Closed
sprankhub opened this issue Aug 2, 2016 · 10 comments
Closed

Category URLs Containing ?___SID=U #44

sprankhub opened this issue Aug 2, 2016 · 10 comments

Comments

@sprankhub
Copy link
Collaborator

The category URLs may contain a ?___SID=U part. It is a bit weird since it is not really shown, but if you do a bin2hex($url), you see 3f5f5f5f5349443d55 at the end, which is exactly the ?___SID=U part. In order to get rid of it, I think we should process the session IDs. So in TheExtensionLab_MegaMenu_Helper_Category_Url#L16 instead of:

return Mage::getUrl($category->getMenuLink());

we should do a:

$url = Mage::getUrl($category->getMenuLink());
$url = Mage::getModel('core/url')->sessionUrlVar($url);
return $url;

This may be needed in other places as well, but for me, this did the trick. Could you please check? Thanks!

@JamesAnelay
Copy link
Member

Hello Simon,
Thanks for the suggestion, I have seen this only a couple of times - seems to be rare and difficult to re-produce. If i'm honest I didn't understand why it happens.

Do you know a way I can re-produce it everytime? So that when I implement your suggestion I can check it?

Either way the suggestion looks fine and i'll look to implement it over the next couple of days.

  • James

@sprankhub
Copy link
Collaborator Author

To be honest, I did not understand as well 😆

I think it only happens if you have multiple StoreViews. And the option System > Configuration > Web > Session Validation Settings > Use SID on Frontend has to be enabled. In my case, it only happened on the server and not on my local machine. The only relevant difference is the usage of https on the server.

Hope this helps in order to reproduce the issue.

@sprankhub
Copy link
Collaborator Author

@JamesAnelay any news?

@sprankhub
Copy link
Collaborator Author

Sorry for bugging you again, but could you please give me an update?

@JamesAnelay
Copy link
Member

Hi Simon,
Not got around to this yet will take a long look this evenings update you
tomorrow morning.

  • James

On 15 Aug 2016 20:20, "Simon Sprankel" notifications@github.com wrote:

Sorry for bugging you again, but could you please give me an update?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#44 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHXszDSfN-nkqA2hvlURoyN9JK8M8EDGks5qgK30gaJpZM4JaVOa
.

@JamesAnelay
Copy link
Member

So I wasn't able to reproduce from the limited tests I did however a bit of reading and the improvement you suggested seems to be the way to go with this, and with the limited tests I was able to do without reproducing the issue it seems to fix the issue.

I've pushed to master and once I do a few more test will release an updated version. (In 1-2 days I imagine)

Thanks again for your contributions!

@sprankhub
Copy link
Collaborator Author

Thanks again! Please note my comment on your commit.

I am happy to test and give you feedback as soon as you released a new version. Please bring the infinaterenderer branch up-to-date when you release a new version (that is the branch I am using right now).

@sprankhub
Copy link
Collaborator Author

Could you release a new version and/or update the infinaterenderer branch?

@JamesAnelay
Copy link
Member

Should get this released and merged at some point Today simon, or early tomorow if not.

@JamesAnelay
Copy link
Member

A new version has now been released (1.5.5) and i've also merge the changes into the infinaterenderer branch for you.

Thanks again for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants