Skip to content

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 index.php in URLs. In some rare cases, the URL might end with index.php, even though that is not needed anymore, resulting in duplicate content, since the same URL without index.php gets the same result. A common case is the URL for the homepage, which often enough contains that reference. Another case is sites which switch from non-rewrite to rewrite the URLs.

This PR introduces one new setting in the SEF system plugin. This setting enforces removing the index.php from the URL with a redirect when the URL starts or ends with index.php. This setting only takes effect if URL rewriting is enabled.

This PR depends on #42692.

I'd like to thank ithelps Digital for sponsoring this feature.

Testing Instructions

  1. Apply the PR
  2. Enable the option in the SEF system plugin.
  3. Open a URL with /index.php/ at the beginning and see that the site is redirected to the version without.
  4. Open a URL with /index.php at the end and see that the site is redirected to the version without.

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

  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
  3. If accepted dont you need to update the installation and the configuration.php.dist
  4. 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 the other points: see other PR

To 2.: I will reword that.

@brianteeman
Copy link
Contributor

I'm misunderstanding something

Why would you have the first option enabled but not have the second option enabled

@Hackwar
Copy link
Member Author

Hackwar commented Jan 24, 2024

The first option modifies how URLs are created. The second one modifies how URLs are parsed. Since people are doing awful things in routing, we can't be sure that their changes don't break this feature, this the 2 options can be enabled separately.

@brianteeman
Copy link
Contributor

Honestly I cant see the value for core of adding 4 extra options (this PR and the other one) to address issues in other peoples code. Thats a maintainability nightmare and a usability issue for all those people that wonder what these options do etc. If, as you say, core does not create these urls then it should be for the extension dev to fix their own problem. And if they wont the site owner can easily create an htaccess rule.

@ot2sen
Copy link
Contributor

ot2sen commented Jan 29, 2024

Brian got quite a few valid points here. Why?
The instruction for test even tells why not. "Unfortunately for testing, core Joomla does not produce such URLs anymore, so we need to do some trickery to trigger the problem." Then why is it a core issue?

Wouldn´t it better go into a checking feature for the official extension JED Checker, for it to teach third-party devs how not to do awful things in routing? IE, do this and you won´t get approved for fame and showcase. Fix it and get in.
https://extensions.joomla.org/extension/jedchecker/

@Hackwar
Copy link
Member Author

Hackwar commented Jan 30, 2024

And at the same time, our users are complaining about sub-par behavior of Joomla. So, should we shift the blame and force users to be stuck in that situation or provide a solution for now to get around that?

I'm happy to add this to the JED checker as soon as you solved the Halting problem.

@brianteeman
Copy link
Contributor

Adding more options is never a solution. Those same users will still complain as they dont know about the options (unless they ask). If its such a real problem (I have no idea) then make the changes without options

Co-authored-by: Quy <quy@nomonkeybiz.com>
@brianteeman
Copy link
Contributor

Confusing with the existing option

image

Hackwar and others added 2 commits February 10, 2024 11:58
@Hackwar
Copy link
Member Author

Hackwar commented Feb 10, 2024

I changed the feature and description above.

@SniperSister
Copy link
Contributor

I have tested this item ✅ successfully on 4e471b0

Works as described!


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

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on 4e471b0


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

@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/42704.

@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
@richard67 richard67 removed the RMDQ ReleaseManagerDecisionQueue label Feb 14, 2024
Co-authored-by: Harald Leithner <leithner@itronic.at>
@Hackwar Hackwar requested a review from HLeithner February 15, 2024 10:41
@Quy Quy removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Feb 15, 2024
@Hackwar
Copy link
Member Author

Hackwar commented Feb 20, 2024

After some discussions, we decided to switch this to use 301 redirects instead of 303s.

@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/42704.

@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 c35a3a0


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

1 similar comment
@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on c35a3a0


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

@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/42704.

@Hackwar
Copy link
Member Author

Hackwar commented Feb 24, 2024

Link to documentation has been added.

@bembelimen bembelimen merged commit 8da0b7c into joomla:5.1-dev Feb 29, 2024
@bembelimen
Copy link
Contributor

Thx

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 29, 2024
@bembelimen bembelimen added this to the Joomla! 5.1.0 milestone Feb 29, 2024
heelc29 added a commit to heelc29/joomla-cms that referenced this pull request Feb 29, 2024
heelc29 added a commit to heelc29/joomla-cms that referenced this pull request Feb 29, 2024
@Hackwar Hackwar deleted the 5.1-routing-indexphp branch March 4, 2024 17:00
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.