Problem/Motivation

It's seems odd that hook_system_info_alter has the word "system" in it. It should really have the word "extension" instead. Furthermore, it makes it sound like the hook is provided by the system module, but it is not.

Steps to reproduce

Proposed resolution

Introduce hook_extension_info_alter and deprecate hook_system_info_alter.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3623862

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mstrelan created an issue. See original summary.

nicxvan’s picture

nicxvan commented

Makes sense to me!

chandansha made their first commit to this issue’s fork.

mstrelan opened merge request !17202

mstrelan’s picture

mstrelan commented
Status: Active » Needs review

mstrelan changed the visibility of the branch 3623862-rename-hooksysteminfoalter-to to hidden.

daffie’s picture

daffie commented

Just a question: Should we create a BC layer instead of just removing the old hook_system_info_alter? The current MR just removes the old hook.

mstrelan’s picture

mstrelan commented

It doesn't just remove the old hook, it invokes it with $this->moduleHandler->alterDeprecated()

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All code changes look good to me.
The new hook has replaced the old hook.
The old is still called as deprecated.
For me it is RTBC.

nicxvan’s picture

nicxvan commented

I tweaked the CR.

I always wonder in these cases, which do we call first? The new hook or the old one?
Is there a way to detect that something changed in the new hook and only call the legacy if nothing changes?

mstrelan’s picture

mstrelan commented

I can't imagine it matters too much here. If it does, implementors could always guard against it with a class property or even a conditional on the values in the info that has been altered.

nicxvan’s picture

nicxvan commented

Fair enough, can we add a note to that effect to the CR?

catch’s picture

catch
he/him
Primary language English
commented

On the issue summary, this is because system module used to do all this, we only fully removed system_rebuild_module_data() in 2020.

mstrelan’s picture

mstrelan commented

#12: added a couple sentences

#13: understood, I meant the current state seems odd, because of this.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

@mstrelan yeah the original state was odd too IMO - the module system should never have lived in system module, installer used to have to call out to it etc. Only just got to the point where it doesn't in #3614153: Install system module alongside other modules in the installer.

Now that 12.0.x is open I think we should probably target this for 12.1.x for removal in 13.x - left suggestions on the MR.