Problem/Motivation

In #3512835: [11.1.x] Add BC stubs for Hook ordering. we added BC stubs for hook attributes and order classes.
The goal was that contrib modules that use the new hook order features can still remain compatible with older versions of Drupal.

From comment #37 in #3512835: [11.1.x] Add BC stubs for Hook ordering.:

We have a grace period before 11.2.0 to tweak api details of the new attributes and API features we are adding.
But whatever we put into older versions of Drupal may get released before 11.2.0, as part of a minor release. At that point we can no longer change these, so we are cutting that grace period short.

Also for disruptive future changes to the new attributes, we now will need to cater to the 10.5.x, 11.0.x and 11.1.x as well.
Worse, we could have contrib modules written for 11.2.x that are compatible with 10.5.x but not with 10.6.x, or vice versa, because we just backported another change.

Steps to reproduce

Proposed resolution

Revert changes from 10.5.x and 11.5.x that were backported as hook order BC stubs, have not been released yet, and which are not necessary for the main purpose that they were added for.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3523385

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:

Comments

donquixote created an issue. See original summary.

donquixote’s picture

I am not saying that full revert is what we need to do, but now we have the option :) (draft MR)

donquixote’s picture

We have to think a little bit what exactly we want to support:

Scenario 1:
A contrib module adds OOP hooks with #[Hook] attribute and using the order parameter.
It still wants to support older Drupal versions using by hook_module_implements_alter() and procedural hooks with the legacy attributes.
-> For this purpose, it is sufficient to backport only the order classes and the new parameter in Hook.

Scenario 2:
A contrib module introduces its own attribute classes that extend Hook.
At the same time it wants to be compatible with 10.5.x and/or 11.1.x.
-> It seems this would work fine with minimal BC stubs.

Scenario 3:
A contrib module uses the new FormAlter attribute.
At the same time it wants to be compatible with 10.5.x and/or 11.1.x.
-> This would require backport of that FormAlter attribute.

Scenario 4:
A contrib module uses the new Preprocess attribute.
At the same time it wants to be compatible with 10.5.x and/or 11.1.x.
-> This would require backport of that Preprocess attribute, AND all the new logic to register the theme hooks. So it is quite hopeless.

Scenario 5:
A contrib module or package wants to provide a functional polyfill for hook attributes, so that they work in 11.1.x and/or 10.x.
-> This would clash with any incomplete BC stubs for order classes and attributes.
-> I don't know if we care about this case.

donquixote’s picture

I am adding another branch where I add what seems to be the minimum we need.
This is mostly to see our options, I am undecided myself what is the best solution here.

nicxvan’s picture

Which one is the one to review?

catch’s picture

We originally backported Hook to 10.5.x so that phpstan wouldn't complain it was missing, and for no other reason, but I think slack discussion showed that's not really useful, and I reverted that commit already. The only new attribute backported to 10.5.x at this point is LegacyHook which is used in 10.5 runtime.

So... I think the remaining question is whether we want to backport any stubs to 11.1 patch releases. The reason to do this would be so that contrib can adopt e.g. FormAlter before it drops compatibility with 11.1, and not have to stay on either procedural hooks or change a Hook attribute later. It is very tempting to want to do this, but to be honest it feels like a lot of work to figure out, and every new attribute we add later we run into the same problem again.

The last patch release of 11.1 is in a couple of weeks, so I'm starting to think we should not worry about this at all, and just continue on 11.2 and later. People writing brand new modules can require >= 11.2. People who want to convert existing modules to OOP hooks can require 10.5 | >= 11.2 (at least in 2-7 months). Having to add > 11.1.7 in .info.yml already limits the feasibility of using this and maintaining 11.1 compatibility.

donquixote’s picture

Which one is the one to review?

For now the idea was to put some options on the table so that we can think about how these could go wrong.

The last patch release of 11.1 is in a couple of weeks, so I'm starting to think we should not worry about this at all, and just continue on 11.2 and later. People writing brand new modules can require >= 11.2. People who want to convert existing modules to OOP hooks can require 10.5 | >= 11.2 (at least in 2-7 months). Having to add > 11.1.7 in .info.yml already limits the feasibility of using this and maintaining 11.1 compatibility.

Maybe the problem here is the deprecation of hook_module_implements_alter().
Now if you want to ship a module that supports 11.1.x and 11.2.x, and which needs to order hook implementations, you basically have to keep the HMIA without the #[LegacyHookModuleImplementsAlter], and eat up the deprecation warnings.

You can drop support for 11.1.x, but maybe there are websites which use that module but which also use other modules that are not yet ready for 11.2.x, for whichever reason.

Personally I think we went too far in that hook ordering issue. Or rather we started things in the wrong order.
The correct order would have been this:

  1. Fix ModuleHandler to not group implementations by module, and thus allow proper ordering.
  2. Provide an alternative to hook_module_implements_alter(), and a way to suppress specific hook_module_implements_alter() implementations in newer Drupal versions.
  3. (future) Deprecate hook_module_implements_alter().

So now I see these options:

  • Introduce minimal BC stubs for 11.1.x.
  • Un-deprecate hook_module_implements_alter().
    This raises the question when we are ready to deprecate it. Maybe in 11.3.0?
  • Do nothing, and force modules to choose or to eat up the deprecation.
nicxvan’s picture

Now if you want to ship a module that supports 11.1.x and 11.2.x, and which needs to order hook implementations, you basically have to keep the HMIA without the #[LegacyHookModuleImplementsAlter], and eat up the deprecation warnings.

This is just flat out wrong. LegacyHookModuleImplementsAlter does nothing in 11.1 so hmia will run as expected, and in 11.2 where it was deprecated if you have the attribute you won't get the deprecation.

Introduce minimal BC stubs for 11.1.x.
Yes, this is the only thing we need to do to handle the named order parameter.

Un-deprecate hook_module_implements_alter().
This raises the question when we are ready to deprecate it. Maybe in 11.3.0? 

Absolutely no need to do that.

Do nothing, and force modules to choose or to eat up the deprecation.
As mentioned if you have LegacyHookModuleImplementsAlter no deprecation will be thrown.

nicxvan’s picture

All we need to do is add the order parameter to 11.1, and the minimum we need of the Order objects.

The question for core committers is what is the minimum that is the least disruptive?

donquixote’s picture

Now if you want to ship a module that supports 11.1.x and 11.2.x, and which needs to order hook implementations, you basically have to keep the HMIA without the #[LegacyHookModuleImplementsAlter], and eat up the deprecation warnings.

This is just flat out wrong. LegacyHookModuleImplementsAlter does nothing in 11.1 so hmia will run as expected, and in 11.2 where it was deprecated if you have the attribute you won't get the deprecation.

The point is, if we don't have the BC stubs in 11.1.x, then you cannot add the order parameter in Hook for 11.2.x without breaking 11.1.x compatibility.
As a consequence, you have to keep your HMIA working in 11.2.x, which means you cannot add the #[LegacyHookModuleImplementsAlter] to it.

Introduce minimal BC stubs for 11.1.x.
Yes, this is the only thing we need to do to handle the named order parameter.

Correct.
If we do that, the problem goes away.

The above comment was directed at the proposal to do nothing, from catch in #8:

The last patch release of 11.1 is in a couple of weeks, so I'm starting to think we should not worry about this at all, and just continue on 11.2 and later. People writing brand new modules can require >= 11.2. People who want to convert existing modules to OOP hooks can require 10.5 | >= 11.2 (at least in 2-7 months). Having to add > 11.1.7 in .info.yml already limits the feasibility of using this and maintaining 11.1 compatibility.

This.
The main problem here would be

People writing brand new modules

because in fact it would not apply just to "brand new modules" but to any module with HMIA that wants to dodge the deprecation by using the order parameter in #[Hook].

But again, the problem goes away if we introduce the BC stubs.

nicxvan’s picture

The above comment was directed at the proposal to do nothing, from catch in #8:

Ah I missed that sorry!

In that case I think we're in agreement that we just need the stubs.

catch’s picture

It is quite common for core to introduce deprecations in minor branches that contrib can't immediately update to, because the new API isn't in the previous minor version, so I'm not sure why hook_module_implements_alter() being deprecated is a showstopper when many other deprecations aren't? It would still be possible to use it with all supported versions from December 2025, which is still six months before the earliest possible Drupal 12 release.

I'm not saying we shouldn't backport some stubs, it probably won't hurt, but not really seeing what's unique about this situation.

nicxvan changed the visibility of the branch 11.x to hidden.

nicxvan changed the visibility of the branch 11.1.x to hidden.

nicxvan changed the visibility of the branch 3523385-11.1.x-clean to hidden.

nicxvan changed the visibility of the branch 3523385-cleanv2 to hidden.

nicxvan changed the visibility of the branch 3523385-11.1.x-revert-bc-stubs to hidden.

nicxvan’s picture

We definitely want stubs, it blows up entirely without them, you can see it works with them here: https://www.drupal.org/project/test_hook_order_parameters/issues/3524252
There are two branches, one with the patch here and one without.

donquixote’s picture

Priority: Normal » Critical

This is critical, we have to avoid releasing a version of 11.1.x that has a bad version of these stubs.

Perhaps we should fully revert the BC stubs now, and then do a follow-up where we re-add what we need.
That is, if we cannot quickly agree on something else.

donquixote changed the visibility of the branch 3523385-11.1.x-revert-bc-stubs to active.

donquixote’s picture

Actually it is kind of too late, it is all in 11.1.7.

nicxvan’s picture

It's not too late, we can fix 11.1.8

nicxvan changed the visibility of the branch 3523385-11.1.x-revert-bc-stubs to hidden.

nicxvan’s picture

Status: Active » Needs work

This needs work, tests are failing

donquixote’s picture

The same test is failing in 11.1.x for me.

donquixote changed the visibility of the branch 3523385-11.1.x-upstream to hidden.

nicxvan’s picture

The two failed are known failing tests on HEAD 11.1.x

nicxvan’s picture

Component: base system » extension system
quietone’s picture

Version: 11.1.x-dev » 11.x-dev
nicxvan’s picture

Title: Reduce hook attribute order BC stubs to the minimum necessary » [11.1] Reduce hook attribute order BC stubs to the minimum necessary
Version: 11.x-dev » 11.1.x-dev

This is specific to 11.1

mxh’s picture

I don't know what's going on with 10.5 and the missing Hook and LegacyHook attribute there. I thought the whole point of that one is to declare an existing function to be ignored once a core version is in use that is capable of real OOP hooks. I was able to implement hooks using this attribute in 10.3 / 10.4 but suddenly it's gone and cannot simply add it that way anymore.

nicxvan’s picture

You can still use the attributes, you just need to phpstan ignore next line to ignore the missing attribute.

Otherwise with the ordering classes there is a fatal and nothing will load.

nicxvan’s picture

Priority: Critical » Major
Status: Needs review » Needs work

Tests are not passing, I'm also going to reduce the priority since this has been out for a bit.

donquixote’s picture

You can still use the attributes, you just need to phpstan ignore next line to ignore the missing attribute.

If we do this, phpstan will complain about "No error to ignore is reported on line **" when you run it with Drupal 11.
So maybe use a different baseline file per core version? Is this possible for a contrib module, in a way that Drupal CI will understand it?

nicxvan’s picture

If we do this, phpstan will complain about "No error to ignore is reported on line **" when you run it with Drupal 11.

Yes, I've seen people deal with this in two ways.

1: add a second rule to ignore that error.
2: add a pre test script for d10 tests to delete all hook attributes legacy hook attributes.

I'm not a fan of either to be honest.

So maybe use a different baseline file per core version? Is this possible for a contrib module, in a way that Drupal CI will understand it?

There is the ignore deprecations so maybe we call globally ignore this specific rule for defined attribute classes?

nicxvan’s picture

Status: Needs work » Closed (outdated)

I'm going to close this as outdated since we only needed this for 11.1 and it no longer gets updates.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

donquixote’s picture

Yeah it would not be useful to try to change this now.
Thanks for cleaning up @nicxvan!