Feature RTC Unit/System Tests bug PR-5.2-dev Pending

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
9 Mar 2024

This fixes issues #43154, #40991, #43104, #35570, #30818, #42686, #41897.

Summary of Changes

Somewhere before Joomla 2.5.0 a person decided that we need to force an Itemid in each and every case of building a URL, defaulting to the home menu item if none is set. This currently results in the URLs for core components never being allowed to hit the NoMenu rule. The end result is, that these URLs are sometimes not SEFed at all.

This PR is depending on the switch in #43432. (has been merged)

This PR contains a few changed system tests. These tests were either broken, tested broken behavior or with the latest changes were unnecessary.

I'd like to thank Joomla Agentur for sponsoring this change.

Testing Instructions

  1. Install the testing sampledata
  2. create an article in the uncategorised category
  3. search for that article in smart search. Notice that the URL is not SEFed.
  4. switch the "Disallow non-SEF URLs" in the SEF system plugin to yes
  5. Reload the search result page. Notice that the URL now is at least partially SEFed with the URL starting with /component/content/article/[alias]?catid=someID

By using the switch, this is backwards compatible the way it is now. When Joomla 6.0 comes around, the code should be removed completely and the changed behavior would then be the new default.

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

I checked the "no documentation changes needed" for now. I understand that some documentation for this needs to happen, but I don't want to add this to the live documentation yet when the feature has not been accepted yet.

avatar Hackwar Hackwar - open - 9 Mar 2024
avatar Hackwar Hackwar - change - 9 Mar 2024
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 9 Mar 2024
Category Libraries
avatar brianteeman
brianteeman - comment - 9 Mar 2024

You need to carefully look at the previous attempts to change this which result in a clusterf*k requiring a full revert

avatar laoneo
laoneo - comment - 12 Mar 2024

This should go then into 6.0, or?

avatar Hackwar
Hackwar - comment - 12 Mar 2024

No, this can be made in a b/c way with adding a (temporary) option. This draft is here to allow us all to easily have an installable package and to test the implications. I'm not a fan of adding yet another switch, but I also don't want to throw a change like this onto everyone at once with 6.0. Adding it to 5.2 would give people a years time to give feedback and to test it and we can adjust if something unforeseen comes up.

avatar Hackwar Hackwar - change - 14 Mar 2024
Labels Added: bug RMDQ b/c break PR-5.2-dev
avatar Hackwar Hackwar - change - 14 Mar 2024
Title
[5.2] Routing: remove code to set Itemid to homepage by default
[5.2] SEF: Add option to don't set Itemid to homepage by default
avatar Hackwar Hackwar - edited - 14 Mar 2024
avatar Hackwar Hackwar - change - 14 Mar 2024
The description was changed
avatar Hackwar Hackwar - edited - 14 Mar 2024
avatar Hackwar Hackwar - change - 14 Mar 2024
Labels Removed: RMDQ b/c break
avatar brianteeman
brianteeman - comment - 14 Mar 2024

its not unforseen when it has already happened

avatar Hackwar
Hackwar - comment - 14 Mar 2024

Then please enlighten me what already has happened.

avatar Hackwar
Hackwar - comment - 14 Mar 2024

I looked this up and there were a few attempts with #19099, #20979 and #22229, all of which have varying degrees of flaws because other code wasn't taken into consideration. This PR does, so I'm asking people to test this.

avatar brianteeman
brianteeman - comment - 14 Mar 2024
avatar joomla-cms-bot joomla-cms-bot - change - 23 Mar 2024
Category Libraries com_tags Front End Libraries
avatar Hackwar Hackwar - change - 30 Mar 2024
The description was changed
avatar Hackwar Hackwar - edited - 30 Mar 2024
avatar SniperSister SniperSister - test_item - 7 Apr 2024 - Tested successfully
avatar SniperSister
SniperSister - comment - 7 Apr 2024

I have tested this item ✅ successfully on c1e9f0f


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

avatar brianteeman
brianteeman - comment - 8 Apr 2024

This entire pr and the approach to it is wrong. Shocked to think anyone considers that its ok.

avatar Hackwar
Hackwar - comment - 8 Apr 2024

Please enlighten me, why this PR and this approach should be wrong.

avatar brianteeman
brianteeman - comment - 8 Apr 2024

You are changing urls without any notice just because someone paid you to change the code without any consideration of the rest of the joomla base.

avatar SniperSister
SniperSister - comment - 8 Apr 2024

You are changing urls without any notice just because someone paid you to change the code without any consideration of the rest of the joomla base.

He fixes a long term bug in the URL generation that prevents the generation of partial SEF URLs. And this is done by binding the fix to a new, optional parameter, causing no changes on existing sites. So neither the given reason ("because someone paid") nor the statement about consequences ("without any consideration of the rest of the joomla base") is true.

avatar brianteeman
brianteeman - comment - 8 Apr 2024

and this is done by binding the fix to a new, optional parameter, causing no changes on existing sites.

which will be removed in j6 - thats my point

avatar SniperSister
SniperSister - comment - 8 Apr 2024

So you are suggesting to remove the parameters in 7.x instead, right?

avatar Hackwar
Hackwar - comment - 8 Apr 2024

The only correct word in your sentence is the You at the beginning.

As you can see at the top of this PR, this change adresses at least 7 issues, which were open at the time when this PR was made. There are several more which have been closed in the past as "Yeah, you misconfigured your system. It's your own fault." This is a genuine bug, which stems from Joomla 1.6 times.

The URLs are also NOT changing without any notice. We have a switch which defaults to the current behavior by default, so any update will not see any change at all until people flip that switch. For 5.1 we also are not enabling this by default for new installations. For 6.0 we are making this mandatory and will remove the switch again, which is perfectly fine considering it is a major release and "b/c" breaks are allowed at this point.

I am well aware of the implications this change has. Right now these lines of code mean that we can't say if an Itemid is a genuinely good fit for the current URL or if it is just the fallback to the default menu item. It requires extensive code to check if the values are correct and should match to the current URL later on in the build() call. At the same time, it disables the nomenu rule in several instances, resulting in URLs not being SEFed at all.

In terms of SEO these lines of code are the reason why we have lots of URLs which contain ?Itemid=[default], even though that query parameter is completely unnecessary since we automatically fall back to the default Itemid when none is given. With the about 2 dozen SEO people I spoke to, they unanimously expected URLs to not contain query parameters and try to get rid of those all the time. So this should improve the SEO results a good bit. Together with #42854 we also have an automatic redirect from the old URLs to the new ones, making the change simple and seamless for google and site owners.

I will of course also write magazine article(s) about this for 5.2 when hopefully all SEF PRs are processed. The documentation has been amended already in the past for this and I will extend on that further when the time comes. People will not be thrown into this without notice.

Last but not least: Yes, I got paid money for working on the router again. I was approached by community members who wanted to see improvements be made here and who asked me to propose changes which everyone would benefit from. I proposed among other things this PR and one of the people in the group has supported this with a small donation. Rest assured that I'm definitely not getting rich from this. You should also know that from the 13 PRs which I wrote for the routing in this year, so far 5 were sponsored. And that is 5 out of 72 in total, which I've created so far this year. If you have issues with other people sponsoring development work, please bring this up with OSM and ask OSM to regulate this properly if you feel that this is not done correctly right now.

avatar brianteeman
brianteeman - comment - 8 Apr 2024

So you are suggesting to remove the parameters in 7.x instead, right?

yes, that would be in line with the practice enforced with everything else.

This practice was adopted after considerable feedback and discussion involving large sections of the core and 3pd development community and site builders. I would not expect this practice to be changed without a similar period of discussion.

I am commenting about this change in practice in general terms and not specificaly about this PR, its just that this PR is the most blatant one to simply mark code for deletion without any form of deprecation at all. You would have to read every line of joomla to spot that these lines of code will be deleted.

avatar Hackwar
Hackwar - comment - 8 Apr 2024

So we aren't allowed to delete wrong code without at least 3 years of deprecation? Then I have to close a lot of other PRs, for example this one: #43230

Your perception of this is pretty skewed.

avatar brianteeman
brianteeman - comment - 8 Apr 2024

By using the switch, this is backwards compatible the way it is now. When Joomla 6.0 comes around, the code should be removed completely and the changed behavior would then be the new default.

I can only go by your own words. A change will be made today that is optional but in j6 the change will be compulsary. That is by your own statement confirmation that the current behaviour will be changed in j6 without any deprecation notice other than a comment and that it will not be backwards compatible with the behaviour in j5.

If I have misunderstood what you have stated then I apologise and you can do what you want - even if you are not following standards for marking something as deprecated.

If I have not misunderstood what you have stated then I stand 100% by my comments

avatar brianteeman
brianteeman - comment - 8 Apr 2024

Then I have to close a lot of other PRs, for example this one: #43230

I didnt make the rules on deprecation. But they exist and they are a contract with the userbase. So yes as that code has not been marked as deprecated you cannot just delete it. Same as a language string that isnt being used cannot be removed without first being marked as deprecated etc

avatar fgsw fgsw - test_item - 11 Apr 2024 - Tested successfully
avatar fgsw
fgsw - comment - 11 Apr 2024

I have tested this item ✅ successfully on c1e9f0f


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

avatar Hackwar
Hackwar - comment - 11 Apr 2024

I've brought the deprecation strategy up in the CMS maintainer meeting and the team was unanimous that my interpretation on this is correct.

avatar Hackwar Hackwar - change - 11 Apr 2024
Status Pending Ready to Commit
avatar Hackwar
Hackwar - comment - 11 Apr 2024

RTC


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

avatar brianteeman
brianteeman - comment - 11 Apr 2024

I find it hard to believe that the maintainers really considered the specifics of this PR or the documented policies as they apply to this specific PR. You have stated already that as a result og this pr j6 will have b/c breaks and yet you refuse to follow the policies regarding docblocks and periods of deprecation as clearly defined
https://developer.joomla.org/development-strategy.html
Sections 6,1,8 and Section 6,2

avatar Hackwar Hackwar - change - 12 Apr 2024
Labels Added: RTC
avatar brianteeman
brianteeman - comment - 12 Apr 2024

roflmao

avatar Hackwar
Hackwar - comment - 2 May 2024

I find it hard to believe that the maintainers really considered the specifics of this PR or the documented policies as they apply to this specific PR. You have stated already that as a result og this pr j6 will have b/c breaks and yet you refuse to follow the policies regarding docblocks and periods of deprecation as clearly defined developer.joomla.org/development-strategy.html Sections 6,1,8 and Section 6,2

Please read those sections again. While that development strategy is not how we handle it right now, this PR is in compliance both with the strategy you linked and the strategy how we apply it right now.

avatar brianteeman
brianteeman - comment - 2 May 2024

lol

avatar Hackwar
Hackwar - comment - 2 May 2024

6.1.1 PHP
All PHP code in the /libraries folder which is not flagged as private is considered to be part of the Joomla API and subject to backwards compatibility constraints. This may be extended to include other PHP code, such as component classes, in the future.

The code is marked as private.

6.1.8 URLs
Any change to a URL that will give a 404 (or some other error) where it previously gave a 200 is a break in backwards compatibility. However, if the change results in a redirect to a new URL (which gives a 200) then that is acceptable.
In general, if a URL is changed then provided the new URL delivers the exact same resource rendered in the same way then that is not considered to be a break in backwards compatibility. For example, changing the order of the arguments in the query part of a URL is not considered to be a break.

Old URLs will still be parsed and not throw a 404. New URLs will be generated without the unnecessary query parameter. With #42854 you also get an automatic redirect from the old to the new URL.

avatar Hackwar Hackwar - change - 11 May 2024
The description was changed
avatar Hackwar Hackwar - edited - 11 May 2024
avatar Hackwar
Hackwar - comment - 3 Jun 2024

@SniperSister @fgsw Since the last few changes, the RTC status unfortunately is not valid anymore. Could you test this again, so that we can merge this?

avatar Hackwar Hackwar - change - 3 Jun 2024
Status Ready to Commit Pending
avatar Hackwar Hackwar - change - 3 Jun 2024
Category Libraries com_tags Front End com_tags Front End Libraries Router / SEF
avatar SniperSister SniperSister - test_item - 3 Jun 2024 - Tested successfully
avatar SniperSister
SniperSister - comment - 3 Jun 2024

I have tested this item ✅ successfully on 534d8cf


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

avatar robbiejackson robbiejackson - test_item - 20 Jul 2024 - Tested successfully
avatar robbiejackson
robbiejackson - comment - 20 Jul 2024

I have tested this item ✅ successfully on 534d8cf

I tested this for a variety of /component routes (using com_content, com_contact and one of my own components) on both single language and multilanguage sites and all URLs were built and parsed ok when the strictrouting parameter was set. This improves the URL format in that the Itemid is no longer set as a URL query parameter.

I also tested /component routes for com_tags, using both the "tag" (with selected tags) and "tags" (with selected parent) views, and these were all routed to the correct pages.


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

avatar robbiejackson
robbiejackson - comment - 20 Jul 2024

Having looked at the code I wouldn't have a problem with removing these code sections in Joomla 6. In comparison with other changes without backward compatibility this change seems fairly small.

I suspect that, like me, people in general avoid /component routes because of the presentation issues which might arise with building routes on the home page, and are likely to set up a hidden menuitem instead.

I guess if you wanted to be ultra-cautious you could set the strictrouting switch on by default in 6 (which should flush out any problems but still leave an escape route back) and then remove all the code in 7.

avatar Hackwar Hackwar - change - 20 Jul 2024
Status Pending Ready to Commit
avatar Quy Quy - change - 15 Aug 2024
Labels Added: Feature
avatar brianteeman
brianteeman - comment - 17 Aug 2024

Not sure how this is RTC as the system tests are failing

avatar Hackwar
Hackwar - comment - 17 Aug 2024

We are aware that there is an issue here. That is the reason why it isn't merged yet. The problem most likely stems from the fact that for new installation this feature is now enabled by default and that again changes behavior which we are testing. I'm still debugging this.

avatar joomla-cms-bot joomla-cms-bot - change - 25 Aug 2024
Category Libraries com_tags Front End Router / SEF Front End com_contact com_content com_newsfeeds com_tags Libraries Router / SEF
avatar Hackwar Hackwar - change - 25 Aug 2024
Status Ready to Commit Pending
avatar Hackwar
Hackwar - comment - 25 Aug 2024

I had to fix a bug in the component routers for the no-menu-behavior. So back to pending. @robbiejackson, @SniperSister could you test this again?


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

avatar Hackwar Hackwar - change - 25 Aug 2024
Labels Removed: RTC
avatar joomla-cms-bot joomla-cms-bot - change - 25 Aug 2024
Category Libraries com_tags Front End Router / SEF com_contact com_content com_newsfeeds Front End com_contact com_content com_newsfeeds com_tags Libraries JavaScript Unit Tests Router / SEF
avatar Hackwar Hackwar - change - 26 Aug 2024
Labels Added: Unit/System Tests
avatar Hackwar Hackwar - change - 28 Aug 2024
The description was changed
avatar Hackwar Hackwar - edited - 28 Aug 2024
avatar Hackwar
Hackwar - comment - 28 Aug 2024

I edited the PR description to contain a paragraph about the changed system tests. I'm still looking for tests for this PR to add it to 5.2. Thank you!

avatar robbiejackson robbiejackson - test_item - 28 Aug 2024 - Tested successfully
avatar robbiejackson
robbiejackson - comment - 28 Aug 2024

I have tested this item ✅ successfully on b63b69b

I tested this for com_content using the site module to display list of items in a category using both menu and non-menu rules and it worked OK.

I also patched that module code so that the catid wasn't passed to getArticleRoute() and confirmed that the old code failed but the updated code worked OK.

So this fixes a genuine problem as it could be argued that you shouldn't need the catid to form the article url as all the info is in the article id:alias.


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

avatar Hackwar
Hackwar - comment - 29 Aug 2024

@robbiejackson Are you sure you commented the right PR? It sounds more like you meant to comment on #43992. This PR does improve some issues, but it doesn't add the right catid by looking it up in the DB. Don't get me wrong, I'm happy for every test I can get. ?

avatar robbiejackson
robbiejackson - comment - 29 Aug 2024

Yes I'm pretty sure. In this recent version in the com_content router getArticleId you check the id is present in the query params before you use it as the catid in the database query. I just engineered a test case where it wasn't present, there were 2 articles with the same alias, and not using ids in the URLs.

avatar Hackwar
Hackwar - comment - 29 Aug 2024

Thank you!

avatar coolcat-creations coolcat-creations - test_item - 29 Aug 2024 - Tested successfully
avatar coolcat-creations
coolcat-creations - comment - 29 Aug 2024

I have tested this item ✅ successfully on b63b69b

Tested successfully as described.

Two personal remarks:

  1. Some might disallowed component/content in their robots.txt so maybe this change or behaviour should be mentioned somewhere prominently

  2. It would be nice if the "slug" component/content/article would be adjustable in future


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

avatar Quy Quy - change - 29 Aug 2024
Status Pending Ready to Commit
avatar Quy
Quy - comment - 29 Aug 2024

RTC


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

avatar Quy Quy - change - 29 Aug 2024
Labels Added: RTC
avatar pe7er pe7er - change - 29 Aug 2024
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2024-08-29 15:41:52
Closed_By pe7er
avatar pe7er pe7er - close - 29 Aug 2024
avatar pe7er pe7er - merge - 29 Aug 2024
avatar pe7er
pe7er - comment - 29 Aug 2024

Thanks @Hackwar !

Add a Comment

Login with GitHub to post a comment