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

Issue #71: Use new 'after_config' hook for Moodle 4.5 #72

Merged
merged 4 commits into from
Dec 2, 2024

Conversation

djarran
Copy link
Contributor

@djarran djarran commented Oct 18, 2024

Description: Adds hook for deprecated callback after_config. Closes #71

@leonstr
Copy link
Contributor

leonstr commented Nov 13, 2024

This change causes an error if admin/tool/abconfig is present when installing a new Moodle site. I think we need to additionally check during_initial_install() in after_config():

 64         if (during_initial_install() || !get_config('tool_abconfig', 'version')) {

* @return void|null
*/
public static function after_config(\core\hook\after_config $hook) {
if (!get_config('tool_abconfig', 'version')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like you copied the code from tool_abconfig_after_config. Is there any way to abstract the to avoid code duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right - it is just copy and pasted. As we're not creating separate stable branches and the function of the callback remains the same, we could just call the original tool_abconfig_after_config function from the new hook.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @djarran ,

Please update the PR with the call to existing tool_abconfig_after_config, I will review and merge the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tuanngocnguyen, this has been done now. Thank you

@djarran
Copy link
Contributor Author

djarran commented Nov 21, 2024

Hi @leonstr, thank you for testing this. I have added the check you suggested and the install now continues as usual.

/**
* Runs after config has been set.
*
* @param \core\hook\before_http_headers $hook
Copy link
Contributor

@tuanngocnguyen tuanngocnguyen Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please fix this @param type, I think ci complains about this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that, updated this one.

@tuanngocnguyen
Copy link
Contributor

lgtm

@tuanngocnguyen tuanngocnguyen merged commit fb793c4 into MOODLE_38_STABLE Dec 2, 2024
35 checks passed
@tuanngocnguyen tuanngocnguyen deleted the issue71-update_for_405 branch December 2, 2024 03:12
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.

Add hook callback for deprecated after_config callback
4 participants