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

Content detailed page throwing 404 when URL contains a trailing slash. #23276

Closed
muhammadfaizandotcms opened this issue Nov 1, 2022 · 8 comments · Fixed by #24822
Closed

Comments

@muhammadfaizandotcms
Copy link

muhammadfaizandotcms commented Nov 1, 2022

Describe the bug

Content detailed page throwing 404 when URL contains a trailing slash.
Reproduced on 22.10
Related ticket: https://dotcms.zendesk.com/agent/tickets/108893

To Reproduce

https://demo.dotcms.com/store/products/k2-mens-mindbender-99ti-skis Works fine
https://demo.dotcms.com/store/products/k2-mens-mindbender-99ti-skis/ throws 404

Expected behavior

The detailed page should return the desired contentlet when a trailing slash exists

Additional context

This behavior works fine on 21.06., 5.3.8.

@stale
Copy link

stale bot commented Dec 31, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 31, 2022
@yolabingo
Copy link
Contributor

yolabingo commented Jan 24, 2023

This Rule may suffice in many cases to preserve the trailing slash behavior of URL mapped content after an upgrade. It obviously could be onerous if too many paths need to be matched.

It provides a 301 HTTP redirect from
https://demo.dotcms.com/store/products/k2-mens-mindbender-99ti-skis/
to
https://demo.dotcms.com/store/products/k2-mens-mindbender-99ti-skis

Googlebot and friends respect 301 redirects so this will preserve SEO for any old URLs with trailing slashes that may be out in the wild.

Screenshot 2023-04-20 at 8 09 09 AM

@waqasakramdot
Copy link

dcolina pushed a commit that referenced this issue Apr 27, 2023
* Avoid the clash between IndexPages and UrlMaps cases.
dcolina pushed a commit that referenced this issue Apr 27, 2023
dcolina pushed a commit that referenced this issue Apr 28, 2023
* We created a IAm subtype enum to manage different page types instead of creating a new IAm type.

* We use tuples to manage isPageAsset and resourceResolveType methods.
dcolina pushed a commit that referenced this issue May 3, 2023
* New method has been created in order to avoid changes on third classes.
dcolina pushed a commit that referenced this issue May 3, 2023
* Avoid the clash between IndexPages and UrlMaps cases.

* We created a IAm subtype enum to manage different page types instead of creating a new IAm type.

* We use tuples to manage isPageAsset and resourceResolveType methods.

* resolvePageAssetSubtype method has been created in order to avoid changes on third classes that use isPageAsset method.
dcolina pushed a commit that referenced this issue May 4, 2023
* Replacing new Tuple2<> constructor statements by Tuple.of() method.

* Replacing condition NOT isPresent() by isEmpty method
dcolina pushed a commit that referenced this issue May 8, 2023
dcolina pushed a commit that referenced this issue May 18, 2023
* resolveResourceType method

* resolvePageAssetSubType method
@swicken-dotcms
Copy link
Contributor

Another ticket: https://dotcms.zendesk.com/agent/tickets/110282

nollymar added a commit that referenced this issue May 25, 2023
* #23276 IAm subtypes have been included.

* Avoid the clash between IndexPages and UrlMaps cases.

* We created a IAm subtype enum to manage different page types instead of creating a new IAm type.

* We use tuples to manage isPageAsset and resourceResolveType methods.

* resolvePageAssetSubtype method has been created in order to avoid changes on third classes that use isPageAsset method.

* #23276 Some improvements.

* Replacing new Tuple2<> constructor statements by Tuple.of() method.

* Replacing condition NOT isPresent() by isEmpty method

* #23276 Refactoring resolvePageAssetSubtype

* #23276 Some test have been added to CMSUrlUtil

* resolveResourceType method

* resolvePageAssetSubType method

* #23276 Test documentation has been added.

* #23276 Some unuseful code has been removed.

* #23276 Some unuseful code has been removed.

---------

Co-authored-by: daniel.colina <daniel.colina@dotcms.com>
Co-authored-by: Nollymar Longa <nollymar.longa@dotcms.com>
@nollymar nollymar reopened this May 25, 2023
manuelrojas pushed a commit that referenced this issue May 26, 2023
* #23276 IAm subtypes have been included.

* Avoid the clash between IndexPages and UrlMaps cases.

* We created a IAm subtype enum to manage different page types instead of creating a new IAm type.

* We use tuples to manage isPageAsset and resourceResolveType methods.

* resolvePageAssetSubtype method has been created in order to avoid changes on third classes that use isPageAsset method.

* #23276 Some improvements.

* Replacing new Tuple2<> constructor statements by Tuple.of() method.

* Replacing condition NOT isPresent() by isEmpty method

* #23276 Refactoring resolvePageAssetSubtype

* #23276 Some test have been added to CMSUrlUtil

* resolveResourceType method

* resolvePageAssetSubType method

* #23276 Test documentation has been added.

* #23276 Some unuseful code has been removed.

* #23276 Some unuseful code has been removed.

---------

Co-authored-by: daniel.colina <daniel.colina@dotcms.com>
Co-authored-by: Nollymar Longa <nollymar.longa@dotcms.com>
oidacra pushed a commit that referenced this issue May 31, 2023
* #23276 IAm subtypes have been included.

* Avoid the clash between IndexPages and UrlMaps cases.

* We created a IAm subtype enum to manage different page types instead of creating a new IAm type.

* We use tuples to manage isPageAsset and resourceResolveType methods.

* resolvePageAssetSubtype method has been created in order to avoid changes on third classes that use isPageAsset method.

* #23276 Some improvements.

* Replacing new Tuple2<> constructor statements by Tuple.of() method.

* Replacing condition NOT isPresent() by isEmpty method

* #23276 Refactoring resolvePageAssetSubtype

* #23276 Some test have been added to CMSUrlUtil

* resolveResourceType method

* resolvePageAssetSubType method

* #23276 Test documentation has been added.

* #23276 Some unuseful code has been removed.

* #23276 Some unuseful code has been removed.

---------

Co-authored-by: daniel.colina <daniel.colina@dotcms.com>
Co-authored-by: Nollymar Longa <nollymar.longa@dotcms.com>
@damen-dotcms
Copy link
Contributor

damen-dotcms commented Jun 16, 2023

Nice! I saw this as an issue on our site while doing a crawl. (=

Yeah Google doesn't care if you have a / or not at the end of a URL. So we should just ignore that.

Doesn't matter if you have a / or not in the canonical URL either -- except to say that sometimes you have to write another bit of regex to get the crawls to line up if you have different canonical URLs -- so ideally we would just load the same consistent / or no / for canonical URLs between the pages.

We don't really need to 301 one to the other... Google views them both as the same URL. And... since it views the URL as the same, adding a 301 redirect can muddy things (A 301 from one identical page to the other can inflate the "hops" if we have any other redirects present.) So I think ideally they would both just load as a 200... But let me know when this is released on our demo site and I'll run some further scans to verify. @nollymar

I just closed #25264 as a dupe of this.

@damen-dotcms
Copy link
Contributor

Yeah OK, so doing another test...

It's really not good to 404 on the / since if we have any links that have a / that Google finds... it'll record the 404 as being on the page -- it doesn't care if there is a / or no slash, it sees the URLs as identical.

https://demo.dotcms.com/store/products/k2-mens-mindbender-99ti-skis
https://demo.dotcms.com/store/products/k2-mens-mindbender-99ti-skis/

But also, 301 from / to no slash seems OK... as long as there are no other 301s ahead in the "chain" and as long as the final version of the page shows a 200.

But yeah, I think ideally... both should just give a 200. No redirect needed. As long as the Canonical URLs are identical (but again, that's just to be kind to people doing crawls so they don't have to write any regex to sort out the different URLs).

@nollymar
Copy link
Contributor

@damen-dotcms both urls give a 200 code, but this change is not yet released. It will be for the next release.

Internal QA: passed

Tested on master.

@josemejias11
Copy link

josemejias11 commented Jul 3, 2023

Approved QA - Tested on master_b295876_SNAPSHOT // Docker // macOS 13.0 // FF v113.0

erickgonzalez added a commit that referenced this issue Jul 19, 2023
erickgonzalez added a commit that referenced this issue Jul 31, 2023
@AndreyDotcms AndreyDotcms added the Release : 22.03.8 Included in LTS patch release 22.03.8 label Jul 31, 2023
@erickgonzalez erickgonzalez added Release : 23.01.5 Included in LTS patch release 23.01.5 and removed Next LTS Release Release : 23.08.16 labels Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.