Skip to content
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

Merged

Conversation

SniperSister
Copy link
Contributor

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

  • Build rules not attached

Expected result AFTER applying this Pull Request

  • Build rules attached

@brianteeman
Copy link
Contributor

Perfect example of why I said

Without a test suite that will measure all the types of urls that can be exist now and that this, and any other future changes, can be measured against this should be reverted. Too risky especially as the "gains" are so minor and can be addressed without touching any code as already discussed.

@SniperSister
Copy link
Contributor Author

@brianteeman we are planing a dedicated “router test suite” sprint for that reason :) so your feedback has been heard and appreciated!

@brianteeman
Copy link
Contributor

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

@Fedik Fedik added the bug label Apr 17, 2024
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@richard67
Copy link
Member

I have tested this item ✅ successfully on 81b7afe

I've tested as follows:

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:

        if (
            $app->get('sef')
            && !$app->get('sef_suffix')
            && $this->params->get('trailingslash', -1) != -1
        ) {
            if ($this->params->get('trailingslash') == 0) {
                // Remove trailingslash
                $router->attachBuildRule([$this, 'removeTrailingSlash'], SiteRouter::PROCESS_AFTER);
                // DEBUG
                error_log('DEBUG: Rule "removeTrailingSlash" attached.');
            } elseif ($this->params->get('trailingslash') == 1) {
                // Add trailingslash
                $router->attachBuildRule([$this, 'addTrailingSlash'], SiteRouter::PROCESS_AFTER);
                 // DEBUG
                error_log('DEBUG: Rule "addTrailingSlash" attached.');
           }
        }

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 DEBUG: Rule "removeTrailingSlash" attached. can be found in the PHP error log when the value of the "trailingslash" parameter is 1, i.e. "Enforce URLs with trailing slash", or when it is -1, i.e. "No change", but not when it should be, and the DEBUG: Rule "addTrailingSlash" attached. can never be seen in the PHP error log.

With the changes from this PR applied, the debug logs appear as expected:

  • Nothing when the value of the "trailingslash" parameter is -1, i.e. "No change"
  • DEBUG: Rule "removeTrailingSlash" attached. when the value of the "trailingslash" parameter is 0, i.e. "Enforce URLs without trailing slash"
  • DEBUG: Rule "addTrailingSlash" attached. when the value of the "trailingslash" parameter is 1, i.e. "Enforce URLs with trailing slash"

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

@rfmjoe
Copy link

rfmjoe commented Apr 29, 2024

@richard67
Copy link
Member

@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.

@LadySolveig LadySolveig added this to the Joomla! 5.1.1 milestone May 11, 2024
@LadySolveig LadySolveig merged commit 7bfcd21 into joomla:5.1-dev May 11, 2024
2 of 3 checks passed
@LadySolveig
Copy link
Contributor

Thank you @SniperSister 👍🏼 and also for testing @richard67 and @rfmjoe !

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

Successfully merging this pull request may close these issues.

None yet

7 participants