User tests: Successful: Unsuccessful:
This fixes issues #43154, #40991, #43104, #35570, #30818, #42686, #41897.
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.
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.
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.
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
This should go then into 6.0, or?
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.
Labels |
Added:
bug
RMDQ
b/c break
PR-5.2-dev
|
Title |
|
Labels |
Removed:
RMDQ
b/c break
|
its not unforseen when it has already happened
Then please enlighten me what already has happened.
Category | Libraries | ⇒ | com_tags Front End Libraries |
I have tested this item ✅ successfully on c1e9f0f
This entire pr and the approach to it is wrong. Shocked to think anyone considers that its ok.
Please enlighten me, why this PR and this approach should be wrong.
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
So you are suggesting to remove the parameters in 7.x instead, right?
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.
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: #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 ✅ successfully on c1e9f0f
I've brought the deprecation strategy up in the CMS maintainer meeting and the team was unanimous that my interpretation on this is correct.
Status | Pending | ⇒ | Ready to Commit |
RTC
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
Labels |
Added:
RTC
|
roflmao
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.
lol
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?
Status | Ready to Commit | ⇒ | Pending |
Category | Libraries com_tags Front End | ⇒ | com_tags Front End Libraries Router / SEF |
I have tested this item ✅ successfully on 534d8cf
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.
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.
Status | Pending | ⇒ | Ready to Commit |
Labels |
Added:
Feature
|
Not sure how this is RTC as the system tests are failing
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.
Category | Libraries com_tags Front End Router / SEF | ⇒ | Front End com_contact com_content com_newsfeeds com_tags Libraries Router / SEF |
Status | Ready to Commit | ⇒ | Pending |
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?
Labels |
Removed:
RTC
|
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 |
Labels |
Added:
Unit/System Tests
|
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!
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.
@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. ?
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.
Thank you!
I have tested this item ✅ successfully on b63b69b
Tested successfully as described.
Two personal remarks:
Some might disallowed component/content in their robots.txt so maybe this change or behaviour should be mentioned somewhere prominently
It would be nice if the "slug" component/content/article would be adjustable in future
Status | Pending | ⇒ | Ready to Commit |
RTC
Labels |
Added:
RTC
|
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 |
You need to carefully look at the previous attempts to change this which result in a clusterf*k requiring a full revert