joomla-cms
joomla-cms copied to clipboard
joomla
[5.2] SEF: Add option to don't set Itemid to homepage by default
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
- Install the testing sampledata
- create an article in the uncategorised category
- search for that article in smart search. Notice that the URL is not SEFed.
- switch the "Disallow non-SEF URLs" in the SEF system plugin to yes
- 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:
-
[X] No documentation changes for docs.joomla.org needed
-
[ ] Pull Request link for manual.joomla.org:
-
[X] 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.
You need to carefully look at the previous attempts to change this which result in a clusterf*k requiring a full revert
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.
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.
I have tested this item :white_check_mark: successfully on c1e9f0f33245270af5cf65843cddd38a989a5db5
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42989.
This entire pr and the approach to it is wrong. Shocked to think anyone considers that its ok.
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.
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.
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
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.
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.
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: https://github.com/joomla/joomla-cms/pull/43230
Your perception of this is pretty skewed.
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
Then I have to close a lot of other PRs, for example this one: https://github.com/joomla/joomla-cms/pull/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
I have tested this item :white_check_mark: successfully on c1e9f0f33245270af5cf65843cddd38a989a5db5
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42989.
I've brought the deprecation strategy up in the CMS maintainer meeting and the team was unanimous that my interpretation on this is correct.
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42989.
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
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.
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.
@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?
I have tested this item :white_check_mark: successfully on 534d8cf73d42ae6189f878b95a9515677c317bcb
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42989.
I have tested this item :white_check_mark: successfully on 534d8cf73d42ae6189f878b95a9515677c317bcb
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.
