joomla-cms icon indicating copy to clipboard operation
joomla-cms copied to clipboard

[5.1] SEF: Enforce suffix

Open Hackwar opened this issue 1 year ago • 20 comments

Pull Request for Issue #15025 .

Summary of Changes

Joomla has been improving its SEO performance constantly and one issue which is still open is the behavior of the suffix. Right now, you can access a URL in Joomla with or without suffix when the option is enabled in the global configuration. This PR introduces a new setting the SEF system plugin which enforces a consistent suffix behavior.

When SEF URLs are enabled, the suffix is enabled and this option is enabled, Joomla will always redirect GET requests to a URL with a suffix if none is present. It will also redirect URLs with a query format parameter to the nicer URL and replace the "nice" suffix with the format parameter if the two collide.

With Joomla 6.0 the option to switch this on/off should be removed again and this should be the default behavior, which would then be added to SiteRouter::parseFormat(). Right now this is YASO (Yet Another Stupid Option) to allow people to test this in live systems and to switch it off if we encounter unforseen issues. The time from 5.1 to 6.0 could be seen as a grace period.

This PR depends on #42692.

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

Testing Instructions

  1. Make sure that you have PR #42692 in your test installation
  2. Apply the patch
  3. Enable SEF URLs and the "Add Suffix to URL" options in global configuration
  4. Go to "System - SEF" plugin and enable the option to "Enforce Suffix"
  5. Open a URL in the frontend and see that URLs ending in a / don't have a suffix, everything else has a suffix.
  6. Take a URL with a suffix and remove the suffix. See that Joomla redirects back to a URL with .html.
  7. Take a URL with a suffix and add ?format=feed to it, for example for a category blog view. See that you get the feed output with the right URL.

Link to documentations

Please select:

  • [X] Documentation link for docs.joomla.org: https://docs.joomla.org/Search_Engine_Friendly_URLs

  • [ ] No documentation changes for docs.joomla.org needed

  • [ ] Pull Request link for manual.joomla.org:

  • [X] No documentation changes for manual.joomla.org needed

Hackwar avatar Feb 21 '24 15:02 Hackwar

This looks like it addresses a 7 year old bug #15025

brianteeman avatar Feb 21 '24 15:02 brianteeman

I have tested this item :white_check_mark: successfully on 784cfdae7d1732ea9ac9f40e699b9c8e36ad82ab


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

ceford avatar Feb 23 '24 09:02 ceford

I have tested this item :white_check_mark: successfully on 784cfdae7d1732ea9ac9f40e699b9c8e36ad82ab


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

nielsnuebel avatar Feb 24 '24 10:02 nielsnuebel

I have tested this item :white_check_mark: successfully on 784cfdae7d1732ea9ac9f40e699b9c8e36ad82ab


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

viocassel avatar Feb 24 '24 10:02 viocassel

RTC


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

richard67 avatar Feb 24 '24 10:02 richard67

In favour of the bug fix. Not in favour of the option. As such this should be in 4.4 as a bug fix and not as a new feature in a future release

brianteeman avatar Feb 24 '24 10:02 brianteeman

Link to documentation has been added.

Hackwar avatar Feb 24 '24 11:02 Hackwar

This is a change in behavior and not just a bugfix. That's why we wont get around this option. For Joomla 6.0 this option will be removed again and the behavior added to the normal behavior of the Add Suffix to URL option. So as long as we will stick to semantic versioning we will have to add this as an option.

Hackwar avatar Feb 24 '24 11:02 Hackwar

Failed. Too bad, because it's a strong request from a client of mine and for that reason SEO agency it's working with ask to migrate to another CMS...

Nuyonuyonoina avatar Feb 24 '24 13:02 Nuyonuyonoina

@Nuyonuyonoina could you comment what is failing on your end? Otherwise I can't fix that and this PR wont be merged.

Hackwar avatar Feb 24 '24 13:02 Hackwar

failed. followed the instructions, but if I remove .html from the URL it does not get added back

webfeuerflo avatar Feb 24 '24 13:02 webfeuerflo

I have tested this item :red_circle: unsuccessfully on 784cfdae7d1732ea9ac9f40e699b9c8e36ad82ab

failed. followed the instructions, but if I remove .html from the URL it does not get added back


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

webfeuerflo avatar Feb 24 '24 13:02 webfeuerflo

Tested on PHP 8.1.27, 10.4.32-MariaDB, Joomla! 5.1.0-alpha4 Alpha [ Kudumisha ] 20-February-2024 16:31 GMT


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

webfeuerflo avatar Feb 24 '24 13:02 webfeuerflo

the pages can now be accessed with 3 URLs: with .html with / and without /. All show the same article


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

webfeuerflo avatar Feb 24 '24 13:02 webfeuerflo

Did you apply the PR #42692 as well?

Hackwar avatar Feb 24 '24 13:02 Hackwar

yes, both patches are installed and active in the patchtester

webfeuerflo avatar Feb 24 '24 13:02 webfeuerflo

Found out, why it failed! I had yootheme installed, although not set as standard template, the framework is doing something that interferes with this patch. When I deactivate all yootheme extensions, the patch works


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

webfeuerflo avatar Feb 24 '24 17:02 webfeuerflo

@saschadube the yootheme system plugin is keeping this patch from working. Any idea what is going on?


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

webfeuerflo avatar Feb 24 '24 17:02 webfeuerflo

My assumption would be that they are creating the router object so early in the process that no other plugin is able to run and thus these new options can't attach themselfs to the router. To be honest, this is something Yootheme needs to fix.

Hackwar avatar Feb 24 '24 22:02 Hackwar

I've unset @webfeuerflo 's negative test result as it has turned out that it was not caused by this PR but by a 3rd party plugin.

richard67 avatar Feb 25 '24 11:02 richard67

Thanks, the next YOOtheme Pro version will resolve the problem with booting com_media and by that creating the SiteRouter too early. 🙏

janschoenherr avatar Feb 26 '24 11:02 janschoenherr

Hello @Hackwar ,

today in the maintainer meeting we decided, that we would really like to have this function in core but there shouldn't be two parameters (one in the global configuration and one on the plugin). Could you merge them, so we have only one parameter in the global configuration and no in the SEF plugin? (so remove the xml + line 74 in the "if".

bembelimen avatar Feb 28 '24 18:02 bembelimen

I'm sorry, but in that case I rather pull this PR back and not have this in 5.1. I'm not going to take the chances of sites breaking because this is enabled by default. There are way to many sites out there which have weird code to just take the chance on this.

Hackwar avatar Feb 29 '24 10:02 Hackwar

I have noticed to late.

Can you please do not do redirect in parseRule? The rule should do "rule thing", nothing more. The same problem with https://github.com/joomla/joomla-cms/pull/42702 (that already merged and should be also fixed)

I not very concerned about extra Option, however the redirect is a bigger issue.

Fedik avatar Feb 29 '24 11:02 Fedik

Revert as it's a draft.


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

bembelimen avatar Mar 01 '24 16:03 bembelimen

In the CMS maintainer meeting we talked about this again and the team agreed to the current implementation. Since it is to late for 5.1, I rebased this to 5.2, but 5.2 is in a desperate need of an upmerge... Anyway, this is theoretically ready to test.

Hackwar avatar Mar 06 '24 21:03 Hackwar

Thank you, @SniperSister, I fixed that. @ceford, @nielsnuebel, @viocassel and of course anybody else who is intersted, would you be so kind to test this again, so that we can merge this for 5.2?

Hackwar avatar Jun 01 '24 20:06 Hackwar

I have tested this item :white_check_mark: successfully on 5900b39ef1985e6b5c29dd7befb62c6e746c00c6

I see #42692 has been merged. I am using a cms clone - so I did a git pull them npm ci, applied the patch and I get this:

An error has occurred.

0 Call to undefined method Joomla\Plugin\System\Sef\Extension\Sef::setSiteRouter() 

I needed to disable the sef plugin in the database to get admin working again.


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

ceford avatar Jun 02 '24 04:06 ceford

I have tested this item :red_circle: unsuccessfully on 5900b39ef1985e6b5c29dd7befb62c6e746c00c6

I see #42692 has been merged. I am using a cms clone - so I did a git pull them npm ci, applied the patch and I get this:

An error has occurred.

0 Call to undefined method Joomla\Plugin\System\Sef\Extension\Sef::setSiteRouter() 

I needed to disable the sef plugin in the database to get admin working again.


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

ceford avatar Jun 02 '24 04:06 ceford

@ceford The comment about #42692 actually isn't true anymore and that PR actually has been reverted before the release of 5.1. Since this PR also doesn't touch that code, your git clone most likely is very outdated and should be updated to latest 5.2-dev before testing this.

Hackwar avatar Jun 02 '24 20:06 Hackwar