TheExtensionLab / MegaMenu

Magento MegaMenu extension
27 stars 6 forks source link

Category URLs Containing ?___SID=U #44

Closed sprankhub closed 8 years ago

sprankhub commented 8 years ago

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 commented 8 years ago

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.

sprankhub commented 8 years ago

To be honest, I did not understand as well :laughing:

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 commented 8 years ago

@JamesAnelay any news?

sprankhub commented 8 years ago

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

JamesAnelay commented 8 years ago

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

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 https://github.com/TheExtensionLab/MegaMenu/issues/44#issuecomment-239883467, or mute the thread https://github.com/notifications/unsubscribe-auth/AHXszDSfN-nkqA2hvlURoyN9JK8M8EDGks5qgK30gaJpZM4JaVOa .

JamesAnelay commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

JamesAnelay commented 8 years ago

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

JamesAnelay commented 8 years ago

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!