Skip to content

Conversation

@michael-milette
Copy link

@michael-milette michael-milette commented Jul 25, 2025

Note: This patch is not required for Moodle 5.0+ as the functionality is now integrated. That said, if you prefer to keep it in for Moodle 5.0+, it won't hurt as the code includes a check for the Moodle version.

The following steps have been completed to ease the review process for the peer review team:

[X] link your issue in the PR title, using the keyword 'resolves #ISSUE-NUMBER', e.g. 'Feature: Provide the ultimate user experience, resolves #42'
[X] provide any further information that is relevant for peer review and not yet mentioned in the linked issue as a comment in the PR
[X] make sure that the 'Allow edits by maintainers' checkbox is checked when creating the PR. Otherwise, the peer reviewer would not be able to push any review changes to the PR and the communication overhead increases
[X] submit your PR in draft status to run the automated checks and review the results
[ ] in case any checks fail solve the mentioned errors by pushing the corrected code to your PR-branch
[X] if all checks pass (or if you are unable to resolve the failing steps without any help of the review team), mark the PR as 'ready for review'

Please let me know if you have any questions or concerns.

Best regards,

Michael

@michael-milette michael-milette marked this pull request as ready for review July 26, 2025 05:44
@abias
Copy link
Member

abias commented Jul 28, 2025

Hi @michael-milette ,

many thanks for submitting this PR!

So this is a cross-port of MDL-63219 which has been integrated into Moodle 5.0.
I see that there is a backport request on MDL-84559 as well, but this is still pending. Thus, it indeed would be interesting to backport this into Boost Union 4.5 if it would help the community.

Looking at your patch, I think it is quite straightforward. However, I would like to raise three change requests:

  1. As you surely now, the main design principle of Boost Union it that it does not change anything to Moodle at first and simply behaves as Boost from Moodle core does. Every change has to come with an admin setting (or it has to be added to https://github.com/moodle-an-hochschulen/moodle-theme_boost_union?tab=readme-ov-file#exceptions-to-our-main-design-principle which should only be rare edge cases).
    Against this background, may I ask you to implement an admin setting which controls the added behaviour? This setting would be added to the "Feel -> Navigation -> Primary navigation" settings section and would basically be the same kill switch as $CFG->navfilter which was added to Moodle core.

  2. Additionally, I would appreciate if you could add a Behat test which covers this setting and its results.
    Ideally, the test would work with a filter which is contained in Moodle core. If this is not possible at all, you can add the installation of filter_filtercodes into .github/workflows/moodle-plugin-ci.yml

  3. Looking at the changes of MDL-63219 in Moodle core, I am wondering why you process explicitely only string filters there, but in this patch here you just use format_string(). You are much more experienced with filters than I am, so I am quite sure that this patch here should be fine as well. But I would appreciate if you could explain the difference to us.

Many thanks in advance,
Alex


Note to myself:
When this PR is integrated, a follow-up issue should be created to evaluate its need again when MDL-84559 would be finally backported in Moodle core.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants