Skip to content

Comments

[6.1] Router: Enforce suffix by default if enabled#44480

Open
Hackwar wants to merge 5 commits intojoomla:6.1-devfrom
Hackwar:6.0-router-tainted-ref
Open

[6.1] Router: Enforce suffix by default if enabled#44480
Hackwar wants to merge 5 commits intojoomla:6.1-devfrom
Hackwar:6.0-router-tainted-ref

Conversation

@Hackwar
Copy link
Member

@Hackwar Hackwar commented Nov 19, 2024

Summary of Changes

This PR changes the feature introduced in 5.2.0 to be able to enforce a suffix ending if it is enabled and makes this the standard behavior. It removes the option from the SEF plugin and moves the code to the SiteRouter class, at the same time using the "tainted URL" feature from #44455 to prevent unnecessary redirects.

Testing Instructions

  1. Enable SEF URLs with suffix ending.
  2. Open a random URL of the site and remove the suffix
  3. Open a random URL of the site and replace the suffix with something else and add ?format=html to the URL
  4. Open a random URL of the site, remove the suffix and add ?format=html to the URL

Actual result BEFORE applying this Pull Request

Joomla loads all the URLs without any redirects.

Expected result AFTER applying this Pull Request

All URLs are redirected to the version with .html at the end.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-6.0-dev labels Nov 19, 2024
@HLeithner
Copy link
Member

I have tested this item 🔴 unsuccessfully on 9873b77

* install pr

  • install sample data
  • open website blog entry "Welcome to our blog" (/blog/welcome-to-your-blog) in new tab and let it stay open
  • change setting "Add Suffix to URL" to yes
  • reload page opened before: 0 Call to undefined method Joomla\CMS\Router\SiteRouter::setTainted()

Expected a smooths transition, at least not a fatal error


#	Function	Location
1	()	JROOT/libraries/src/Router/SiteRouter.php:203
2	Joomla\CMS\Router\SiteRouter->parseFormat()	JROOT/libraries/src/Router/Router.php:384
3	Joomla\CMS\Router\Router->processParseRules()	JROOT/libraries/src/Router/Router.php:144
4	Joomla\CMS\Router\Router->parse()	JROOT/libraries/src/Application/SiteApplication.php:738
5	Joomla\CMS\Application\SiteApplication->route()	JROOT/libraries/src/Application/SiteApplication.php:243
6	Joomla\CMS\Application\SiteApplication->doExecute()	JROOT/libraries/src/Application/CMSApplication.php:306
7	Joomla\CMS\Application\CMSApplication->execute()	JROOT/includes/app.php:58
8	require_once()	JROOT/index.php:32
```<hr /><sub>This comment was created with the <a href="https://github.com/joomla/jissues">J!Tracker Application</a> at <a href="https://issues.joomla.org/tracker/joomla-cms/44480">issues.joomla.org/tracker/joomla-cms/44480</a>.</sub>

@Hackwar
Copy link
Member Author

Hackwar commented Nov 21, 2024

That is why it says "tainted URL" feature from #44455 and since that PR is not merged yet, this PR indeed fails.

@HLeithner HLeithner marked this pull request as draft November 21, 2024 16:59
@HLeithner
Copy link
Member

I changed this to draft since it's not ready

@Hackwar Hackwar added Feature b/c break This item changes the behavior in an incompatible why. HEADS UP labels Nov 23, 2024
@Hackwar
Copy link
Member Author

Hackwar commented Nov 23, 2024

I added the b/c break label because technically it removes anoption and forces this behavior on everyone, even though it is the expected behavior.

@Hackwar Hackwar marked this pull request as ready for review February 14, 2025 15:10
@VaishnaviSidral
Copy link

I have tested this item ✅ successfully on 7950bcb


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

@exlemor exlemor moved this to Abandoned in PR Cleanup May 10, 2025
@MacJoom MacJoom moved this from Abandoned to Held for B/C break in PR Cleanup May 10, 2025
@ceford
Copy link
Contributor

ceford commented Jun 3, 2025

Could you clarify the testing instructions? Without the patch applied, and using this url: http://localhost/joomla-cms6/en/content-component/single-article (which is the Australian Parks article in the testing data) I get a 301 response - permanent 301 redirect. All the tests work the same without the patch applied. That is, I get the behaviour the patch is supposed to produce.

With the patch applied all pages produce an error:

     0 Call to undefined method Joomla\CMS\Router\SiteRouter::setTainted() 

Call Stack
# 	Function 	Location
1 	() 	JROOT/libraries/src/Router/SiteRouter.php:203
2 	Joomla\CMS\Router\SiteRouter->parseFormat() 	JROOT/libraries/src/Router/Router.php:384
3 	Joomla\CMS\Router\Router->processParseRules() 	JROOT/libraries/src/Router/Router.php:144
4 	Joomla\CMS\Router\Router->parse() 	JROOT/libraries/src/Application/SiteApplication.php:754
5 	Joomla\CMS\Application\SiteApplication->route() 	JROOT/libraries/src/Application/SiteApplication.php:244
6 	Joomla\CMS\Application\SiteApplication->doExecute() 	JROOT/libraries/src/Application/CMSApplication.php:306
7 	Joomla\CMS\Application\CMSApplication->execute() 	JROOT/includes/app.php:58
8 	require_once() 	JROOT/index.php:32 
```<hr /><sub>This comment was created with the <a href="https://github.com/joomla/jissues">J!Tracker Application</a> at <a href="https://issues.joomla.org/tracker/joomla-cms/44480">issues.joomla.org/tracker/joomla-cms/44480</a>.</sub>

@Hackwar
Copy link
Member Author

Hackwar commented Jun 3, 2025

@ceford with which Joomla version did you test this? The code you mentioned is only available in recent versions of Joomla 6.

@ceford
Copy link
Contributor

ceford commented Jun 3, 2025

@ceford with which Joomla version did you test this? The code you mentioned is only available in recent versions of Joomla 6.

I use a local clone and update it often, last time being a few days ago. And from time to time I throw it away and start again. The last commit message for this test was May 28:

[6.0] Workflow: use generic AbstractApplication in type hint (#43155)

@HLeithner HLeithner changed the base branch from 6.0-dev to 6.1-dev August 31, 2025 11:58
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 6.1-dev.

@HLeithner HLeithner changed the title [6.0] Router: Enforce suffix by default if enabled [6.1] Router: Enforce suffix by default if enabled Aug 31, 2025
@jikubiswal
Copy link

I have tested this item ✅ successfully on 08d5790

Yes Patch is working as per the description / test instrcution
Applied Commit SHA: 08d5790

All URLs are redirecting to the version with .html at the end.


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

@muhme
Copy link
Contributor

muhme commented Feb 6, 2026

Tried to test with 'Add Suffix to URL' and 'Use URL Rewriting' and .htaccess in place. But, already before the PR all use cases are redirected to .html page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

b/c break This item changes the behavior in an incompatible why. HEADS UP Feature Language Change This is for Translators PR-6.0-dev PR-6.1-dev

Projects

Status: Held for B/C break

Development

Successfully merging this pull request may close these issues.

8 participants