- Issue created by @nicxvan
- π¬π§United Kingdom catch
This seems critical for 11.1.x and very nice to have for 11.0.x and lower.
- πΊπΈUnited States nicxvan
nicxvan β changed the visibility of the branch 3512835-add-bc-stubs to hidden.
- πΊπΈUnited States nicxvan
nicxvan β changed the visibility of the branch 11.x to hidden.
- πΊπΈUnited States nicxvan
nicxvan β changed the visibility of the branch 3512835-bc-stubs to hidden.
The Needs Review Queue Bot β tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- πΊπΈUnited States nicxvan
Gitlab is not picking up my change, I fixed this 5 hours ago, and now I cannot apply a suggestion to it.
- πΊπΈUnited States smustgrave
I tried too and got a βfile has been changedβ
Wonder if 11.1x branch permission?
- πΊπΈUnited States smustgrave
Moving to NW because all the tests appear to be failing
- πΊπΈUnited States dww
PHP Fatal error: Uncaught Error: Call to undefined method Drupal\Core\Hook\Attribute\Hook::setMethod() in /builds/issue/drupal-3512835/core/lib/Drupal/Core/Hook/HookCollectorPass.php:289
- πΊπΈUnited States nicxvan
Yeah I forgot a method, I'll move it in later tonight.
-
larowlan β
committed 81f5ac29 on 10.5.x
Issue #3512835 by nicxvan: [11.1.x] Add BC stubs for Hook ordering (...
-
larowlan β
committed 81f5ac29 on 10.5.x
-
larowlan β
committed 55dfb3cd on 11.0.x
Issue #3512835 by nicxvan: [11.1.x] Add BC stubs for Hook ordering (...
-
larowlan β
committed 55dfb3cd on 11.0.x
-
larowlan β
committed 64bfef04 on 11.1.x
Issue #3512835 by nicxvan: [11.1.x] Add BC stubs for Hook ordering
-
larowlan β
committed 64bfef04 on 11.1.x
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Committed to 11.1.x and backported to 10.5.x and 11.0.x
- π©πͺGermany donquixote
Did the PREFIX and SUFFIX need backporting?
-
larowlan β
committed 7800942b on 10.5.x
Revert "Issue #3512835 by nicxvan: [11.1.x] Add BC stubs for Hook...
-
larowlan β
committed 7800942b on 10.5.x
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
The 10.5.x backport failed - we're using readonly properties here which aren't allowed on the versions of PHP 10.x supports
- πΊπΈUnited States nicxvan
I can roll a 10.5 specific if someone doesn't before me.
- πΊπΈUnited States nicxvan
Created π [10.5.x] Add BC stubs for Hook ordering. Active because creating 10.5 from here was too out of date.
- π©πͺGermany donquixote
This MR did a lot more than what we need to for the BC support.
We absolutely don't need all these new attributes.
We only need to make the existing attributes compatible with new modules.
We did have a very clear plan for this, which we somehow forgot.Missing attributes are ok, because we call ->getAttributes() with a type parameter. Only attributes classes with incompatible constructor signature are a problem.
We should either revert this and do again, or remove all the changes we don't need.
And before the next release for 11.1.x or 11.0.x. (if that has not already happened, I am not following). - πΊπΈUnited States nicxvan
That is not true, we backported Hook to 10.4 as well so phpstan would not complain, we backported the attributes here too.
In this case the Order classes are required otherwise there would be a fatal, and since we backported Hook already we need to backport those as well.
None of the plumbing for collection or ordering was backported, why are you concerned about backporting these classes?
- π©πͺGermany donquixote
(recap of what I said in slack)
My concern is this:
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.You are correct about order classes though, we do need these in any branch that collects hook attributes.
(more below)I'm not sure what plan you are referring to
I remember a discussion where we determined that missing attributes are not a problem, only incompatible constructors.
The conclusion was that we update existing attributes in older versions where they exist, to allow alternative parameters, but that we don't backport all new attribute classes.For concerns with phpstan, I am sure this can be handled in some way.
we backported Hook and LegacyHook to 10.4 as part of the original OOP hook initiative
I would have complained about backporting hook attribute to 10.x, but I probably did not pay attention at the time.
In this case the Order classes are required otherwise there would be a fatal, and since we backported Hook already we need to backport those as well to all branches.
This is correct, we do have to backport the order classes to any branch which has the Hook attribute class, OR any branch where we actually collect hook attributes.
But this also means: If we had not backported the Hook class to 10.x, we now would not have to create BC stubs for 10.x.None of the plumbing for collection or ordering was backported, why are you concerned about backporting these classes, what could it possibly affect, they are not used.
The attribute classes are part of public API, the plumbing is not.
In general, releasing public API features is easy, the problem is that we cannot easily remove or change them later. - π©πͺGermany donquixote
Follow-up here: π Reduce hook attribute order BC stubs to the minimum necessary Active
- Status changed to Fixed
about 1 month ago 1:59pm 22 May 2025 Automatically closed - issue fixed for 2 weeks with no activity.