adampmoss / CreareSEO

A free SEO extension for Magento 1
GNU General Public License v2.0
151 stars 72 forks source link

Redirection loop on urls with more query params #126

Closed tataencu closed 6 years ago

tataencu commented 6 years ago

If you turn on "Force canonical product redirect" and you have URLs pointing to your product that have more query params (utm_source&utm_campaign for example) the querystring gets exploded to utm_source&utm_campaign and then exploded again and again until you hit a "URL is not properly redirecting"error.

sprankhub commented 6 years ago

I cannot reproduce this issue. I set "Enable Canonical Product Redirecting" to yes and then opened the page http://magento193.local/first-test-product.html?utm_source=test&utm_campaign=test - the page works as expected. Can you share more details?

hejhog commented 6 years ago

I can confirm this bug. I have just installed an AMP module and it does not like the ?amp=1. On category product listing pages the correct URL is shown as: product.html?amp=1 but this redirects to: product.html?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1?amp=1

Have tested on both production server and test server with same results.

sprankhub commented 6 years ago

Could you please try it with the latest master branch? This has been fixed here https://github.com/adampmoss/CreareSEO/commit/4da8581f29b113fb1881c313626698fa0e0159b5#diff-e20d5543a4f63016ef12355c547f3bcf. See also the related issue https://github.com/adampmoss/CreareSEO/issues/79.

hejhog commented 6 years ago

Applied patch - still doing the same as in my comment above.

tataencu commented 6 years ago

Actually the queryString escaping leads to & being escaped to & that is what was going on on my setup.

On Sun, 15 Oct 2017 at 18:16, Shaun notifications@github.com wrote:

Applied patch - still doing the same as in my comment above.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/adampmoss/CreareSEO/issues/126#issuecomment-336718498, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0nf8kyj5QBnI9zbV_8getsVkE1Hhnwks5ssiG4gaJpZM4PUYfh .

-- George Maicovschi

Phone: +40726270377

sprankhub commented 6 years ago

Are you 100% sure you are using the latest master branch @hejhog? I really do not know how I should reproduce the issue.

hejhog commented 6 years ago

Yes. I even downloaded Observer.php $url = Mage::helper('core/url')->escapeUrl($product->getUrlModel()->getUrl($product, array('_ignore_category'=>true)).$querystring);

If you would like to investigate on my test server you are more than welcome or I could ask the module provider if I could forward a copy of their AMP module or I could insert code to generate logs for you.

sprankhub commented 6 years ago

I could use some debug output from https://github.com/adampmoss/CreareSEO/blob/34bacede6c2a34b50607b032554ebd33ccff4918/app/code/community/Creare/CreareSeoCore/Model/Observer.php#L191-L208, yes:

And besides of that: What is your Magento version?

I hope that I have a better understanding of the issue afterwards...

hejhog commented 6 years ago

Can I email these details to you. creare.co.uk ??

sprankhub commented 6 years ago

Could you just change the domain name and post them here (preferred)? If not, contact me.

sprankhub commented 6 years ago

The issue is that the extension TM_Amp rewrites Mage_Core_Model_Url::getUrl and adds the amp parameter in certain cases. We then add the amp parameter via the $querystring variable again leading to an infinite loop. My PR #135 should solve this issue.