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

add Moodle 4.4 hooks #85

Merged
merged 1 commit into from
May 22, 2024
Merged

Conversation

usqfowlerj
Copy link
Contributor

This ought to be what's needed to fix issue #84 with Moodle 4.4+ superseding the 'before_standard_html_head' and 'before_http_headers' plugin callbacks.

@usqfowlerj
Copy link
Contributor Author

What's the strategy for situations like this, where the failure happens in the lint check on a PHP and Moodle version the code will never run under anyway?

@danmarsden
Copy link
Member

thanks @usqfowlerj - usually we generate a new branch, which I'm leaning towards doing here with a new 4.4 branch and we could drop the deprecated before_standard_html() code completely at the same time....

@bwalkerl
Copy link
Contributor

Thanks @usqfowlerj

I've had some luck getting around that issue for a similar hook by including it in a use statement, which shortens the line enough to meet the coding guidelines, i.e. https://github.com/catalyst/moodle-local_envbar/pull/210/files

I personally don't think we need a new 4.4 branch for this particular change as there's a clear workaround, and the hooks will work cleanly on all supported versions. Though it would be worth adding a note mentioning the deprecation to the old functions in lib.php, similar to what has been done in the linked example above.

If you can get that sorted and rebase onto MOODLE_401_STABLE we should be able to look at merging this in.

@usqfowlerj
Copy link
Contributor Author

I believe it should all be in order as suggested.

@bwalkerl bwalkerl merged commit 4e07ad3 into catalyst:MOODLE_401_STABLE May 22, 2024
19 checks passed
@usqfowlerj usqfowlerj deleted the m404-hooks branch May 22, 2024 23:04
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.

3 participants