Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Fixed bug with friendly URLs and Media Servers #9203
I could draw your attention to these bugs?
I think they are quite important as errors, but the months go by and nobody solves them
if a product has some combinations (now each combination has a own url, to avoid duplications it is used the canonical url) the canonical url of these pages does not exist, prestashop will redirect to the main combination (isas/3-5-prueb) but this has the canonical url (camisas/3-prueba) to the product page like in this example
https://www.mysite.com.ar/camisas/3-prueba-camisa.html (canonical url)
this one for DNI
problem when you update
changed the title from
CO: Fix bug with friendly URLs and Media Servers
Fixed bug with friendly URLs and Media Servers
Jun 25, 2018
@prasanthSelvar https://github.com/PrestaShop/PrestaShop/blob/1.7.3.x/classes/Tools.php - the code is exactly the same - so it affects every Prestashop version containing the same code.
Apache "handles" it (thus you won't experience a bug), since it kinda ignores
Litespeed for example, the problem is reproducible because it actually does the
In my comment over at the forge.prestashop, I also explain why it's not working as intended with a test case: http://forge.prestashop.com/browse/PSCSX-9983?focusedCommentId=170029&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-170029
I can set up a Prestashop 1.7.3.x test installation on a Litespeed server if you'd like, so you can see the broken behaviour.
@prasanthSelvar - small update - regarding Prestashop 1.7.3.x it's a bit harder to test actually since it requires an additional step - after you've enabled a media URL, then go back to SEO settings and click "Save" again in the section where the
Updating the Media Servers alone in Prestashop 1.7, doesn't call the
I'm sorry I didn't catch this in the reproduction steps I did, so to reproduce in 1.7:
This will render the .htaccess file with following content:
As you see, once again we end up with the
The rewrite debugging:
On 7th line in the above block, we see that there's a match with the pattern
However, because of the RewriteCond
This will result in a 404 for the image.
The fix I do in the PR will basically just ensure we always have the shop URL in the
This is a working Prestashop 1.7.x installation, it has a "Media Server" configured with URL
I've patched the above Prestashop 1.7.x installation with my pull request, as you can see images such as the Media URL and shop URL works:
This Prestashop 1.7.x installation, has a "Media Server" configured with the URL
Basically same setup as the first shop.
In this case I've not patched the code to include my fix.
As you can see on the front-page, the product images doesn't work.
The CDN URL will give a
The shop URL will also give a
This happens because of the missing
If we access the raw URL to the image, you can see it exists on the shop: http://broken.presta17.elamedia.org/img/p/2/2-home_default.jpg
The only difference between the two shops (other than domain names) is the fact that one contains my fix in this PR (presta17.elamedia.org) and one does not (broken.presta17.elamedia.org)
The fix ensures images are accessible on the shop + media servers - and that's ideally what Prestashop wants.
My end goal, is the fact Prestashop (1.6 and 1.7) does proper rewrites and not rely on a buggy condition handling in Apache - as a result we'll also end up with working sites.
@prasanthSelvar I put the target branch as 1.6.x, 1.7.x and develop - since I want it to go into all releases.
The testing was done on 1.6.x and 1.7.4.x - so, in fact, it's already tested, and was targeted to the specific versions.
I even replied to the comment you made ( #9203 (comment) ) - that it's concerning all Prestashop versions, and since it's a bug - I'd like it to get fixed in all Prestashop versions.
I don't want a (serious) bug that makes Prestashop unusable to only target a new version - that's just silly.