-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[5.1] fix wrong parameter value of new trailingslash parameter in SEF plugin #43292
[5.1] fix wrong parameter value of new trailingslash parameter in SEF plugin #43292
Conversation
Perfect example of why I said
|
@brianteeman we are planing a dedicated “router test suite” sprint for that reason :) so your feedback has been heard and appreciated! |
great to here I was partialy listened to. Shame that it has taken precited issues taking place AFTER the merges. Would save a lot of hurt if these untested changes had not been merged until after the tests were created. After all at least one of them was addressing a multiple year old issue |
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
I have tested this item ✅ successfully on 81b7afe I've added some error logging to the code to see when which rule is attached and made sure that PHP errors are logged into a log file. E.g. here the code with the PR applied and with my additional error logs:
Then I have tested the 3 values for the "trailingslash" parameter of the SEF plugin by selecting that value, sabing the plugin settings and then calling the frontpage and watching my error log. Without the changes from this PR, only the With the changes from this PR applied, the debug logs appear as expected:
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43292. |
tested this (https://raw.githubusercontent.com/joomla/joomla-cms/81b7afeda08c2dce8aec9b34267220d086e6af0b/plugins/system/sef/src/Extension/Sef.php) successfully on a live-site. |
@rfmjoe Could you mark your test result in the issue tracker so it's properly counted? For doing this, go to this PR in the issue tracker here https://issues.joomla.org/tracker/joomla-cms/43292 , use the blue "Test this" button at the top left corner, select your test result and submit. Thanks in advance. |
Thank you @SniperSister 👍🏼 and also for testing @richard67 and @rfmjoe ! |
Summary of Changes
The new trailing slash parameter, introduced in #42702 had received another patch that changed the parameter values in the XML, see #43195. That second change was incomplete, leaving the onAfterInitialise function unpatched.
This PR fixes the issue.
Testing Instructions
See #42702 and/or code review (tbh it's a no-brainer as we are obviously comparing with a non-existing value...)
Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request