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

[5.1] SEF: Implementing trailing slash behavior #42702

Merged
merged 10 commits into from Feb 28, 2024

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Jan 23, 2024

Summary of Changes

Joomla has been improving its SEO performance constantly and one issue which is still open is the behavior of trailing slashes in URLs. Right now, Joomla has largely been following a no-trailing-slashes policy, however we never enforced this. At the same time, some SEO people wanted to have trailing slashes on all URLs. This PR introduces this feature.

This introduces 2 new settings in the SEF system plugin. The first setting lets you decide if you want to have a trailing slash, no trailing slash or no special behavior (which is the b/c setting). Depending on the setting, it either adds or removes all trailing slashes.

The second setting is available when one of the former options is set and redirects a GET request with missing or trailing slash to the "correct" URL with a 301 depending on the other option.

This PR depends on #42692.

I'd like to thank djumla GmbH for sponsoring this feature.

Testing Instructions

  1. Apply the PR
  2. Check that no URLs changed
  3. Edit libraries/src/Router/SiteRouter.php and change line 466 to $uri->setPath($uri->getPath() . '/' . $tmp . '/');
  4. See that all URLs now contain a trailing slash.
  5. Go to the SEF system plugin in the backend and set the setting to remove the trailing slash.
  6. See that all URLs now don't contain a trailing slash.
  7. Revert the change from step 3.
  8. Change the setting in the SEF plugin to always add a trailing slash.
  9. See that all URLs now contain a traling slash again.
  10. Set the second option to enforce the behavior with a 301 redirect.
  11. Go to the frontend again and either add or remove the trailing slash, depending on your setting, in your browser bar.
  12. See that Joomla redirected you to the same page with the correct URL again.

Link to documentations

Please select:

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-5.1-dev labels Jan 23, 2024
@brianteeman
Copy link
Contributor

brianteeman commented Jan 23, 2024

  1. Instead of adding another option which a large % of users will never use why not simply do this in the htaccess.
    2. the description is confusing "Remove unneeded index.php from URLs" because it sounds the same as the existing option which removes index.php
  2. If accepted dont you need to update the installation and the configuration.php.dist
  3. What happened to the requirement for all new "features" to be accompanied with documentation

@Hackwar
Copy link
Member Author

Hackwar commented Jan 23, 2024

To 1.: it is a difference if we output wrong URLs and then redirect in the htaccess or do it correct right away. Yes, the redirect could be done in the htaccess, but that requires error prone actions from the user, while this here is the simpler solution.

To 2.: no, since these options are not stored in the configuration.php.

To 3.: the PR is not merged yet, is it. 😉 Documentation is on its way and will be combined with the other 2 router related PRs.

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 131eafe


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42702.

@brianteeman
Copy link
Contributor

brianteeman commented Jan 23, 2024

Tested with both of the following

In both cases with the sample joomla data I observed no trailing slash in the url

@Hackwar
Copy link
Member Author

Hackwar commented Jan 23, 2024

Did you read that this PR depends on #42692 ?

@brianteeman
Copy link
Contributor

no I didnt see that

@brianteeman
Copy link
Contributor

I have not tested this item.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42702.

plugins/system/sef/src/Extension/Sef.php Outdated Show resolved Hide resolved
plugins/system/sef/src/Extension/Sef.php Outdated Show resolved Hide resolved
plugins/system/sef/src/Extension/Sef.php Outdated Show resolved Hide resolved
@SniperSister
Copy link
Contributor

I have tested this item ✅ successfully on d49ccdf

Work as described!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42702.

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on d49ccdf


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42702.

@richard67
Copy link
Member

Setting RTC as it has 2 good tests, but setting also the RMDG (release manager decision queue) label because this PR is subject of discussion among maintainers.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42702.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 14, 2024
@richard67 richard67 added the RMDQ ReleaseManagerDecisionQueue label Feb 14, 2024
plugins/system/sef/sef.xml Outdated Show resolved Hide resolved
@Hackwar
Copy link
Member Author

Hackwar commented Feb 14, 2024

In the CMS Maintainer meeting we talked about this PR and decided to join the two options into one. So if you decide for one behavior, it automatically also means when the URL is not as expected, it does a redirect to the right one. I made the necessary changes. @SniperSister and @viocassel would you be able to test this again?

@richard67 richard67 added RMDQ ReleaseManagerDecisionQueue and removed RMDQ ReleaseManagerDecisionQueue labels Feb 14, 2024
@richard67
Copy link
Member

Back to pending


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42702.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 14, 2024
@richard67 richard67 removed the RMDQ ReleaseManagerDecisionQueue label Feb 14, 2024
@richard67
Copy link
Member

Back to pending as there have been made changes.
@SniperSister @viocassel Could you test again? Thanks in advance.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42702.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 20, 2024
@SniperSister
Copy link
Contributor

I have tested this item ✅ successfully on 74383a3


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42702.

1 similar comment
@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on 74383a3


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42702.

@richard67 richard67 added the RTC This Pull Request is Ready To Commit label Feb 20, 2024
@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42702.

@Hackwar
Copy link
Member Author

Hackwar commented Feb 24, 2024

Link to documentation has been added.

@LadySolveig LadySolveig merged commit 96b5735 into joomla:5.1-dev Feb 28, 2024
1 of 2 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 28, 2024
@LadySolveig
Copy link
Contributor

Thank you @Hackwar and for testing and support @SniperSister @viocassel @richard67

@LadySolveig LadySolveig added this to the Joomla! 5.1.0 milestone Feb 28, 2024
@Hackwar Hackwar deleted the 5.1-router-trailing branch February 28, 2024 21:49
@Hackwar
Copy link
Member Author

Hackwar commented Feb 28, 2024

Thank you!

@pAnd0rASBG
Copy link

Thank you very much for this @Hackwar ! I'm glad it didn't get dismissed.

my 2c on the importance:
according to Google's Martin Splitt, who I had a conversation with on this topic late last year, this still counts: https://developers.google.com/search/blog/2010/04/to-slash-or-not-to-slash?hl=en

tl;dr:
Trailing slashes do matter to Google. Traditionally that comes from url w/ trailing slash being seen as directory and url w/o trailing slash as file. Google respects same urls with and without trailing slashes to have different content, even.
On the other hand, if e.g. https://mysite.com/blog and https://mysite.com/blog/ show the same content, Google may punish you for duplicate content and even if that's not the case, your "Link Juice" will be split 50/50, so instead of getting one better ranked entry in the SERPs, you get 2 worse.

So even if a large % of users will not actively use it, if there is a default behaviour for either with or without trailing slash, still all users will profit from the SEO impact.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42702.

@Hackwar
Copy link
Member Author

Hackwar commented Apr 11, 2024

You're welcome. ;-)

@rfmjoe
Copy link

rfmjoe commented Apr 11, 2024

Thank you Hackwar for bringing this finally into the joomla core! :-)

@brbrbr
Copy link

brbrbr commented Apr 16, 2024

Are you sure the slashes are working in the attachBuildRule part? Appears to be a bit of number bonanza on the trailingslash param: 1 and 2 in onAfterInitialise 0 and 1 in enforceTrailingSlash

@SniperSister
Copy link
Contributor

@brbrbr see #43292

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators PR-5.1-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet