I figured out why I needed the empty baseline.
It was because during the conversion there were thousands of changes. A normal baseline process doesn't need that.
nicxvan → changed the visibility of the branch 3485896-donquixote-order-at-call-time-nicxvan-changes-BAD to hidden.
I think we will be consolidating on the new approach shortly. There is still some refinement to happen, but before I hide the attempt simplification branch and update the CRs and IS to remove mentions of extra_types I wanted to give some notice.
I created an issue to track that and removed the change from this issue:
🐛
Determine what to do about comment module form alters calling getFieldUIPageTitle
Active
I converted contact form alters instead so we can validate.
Sorry I was looking at the old MR, is this ready for review again?
I think this needs a clean rebase.
thanks!
Ok, huge WTF, I injected comment.manager and getFieldUIPageTitle does not exist, did a quick search, it was removed 10 years ago.
#2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form →
Can we safely just delete these? These alters can't have done anything in years.
Ok, huge WTF, I injected comment.manager and getFieldUIPageTitle does not exist, did a quick search, it was removed 10 years ago.
#2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form →
Can we safely just delete these? These alters can't have done anything in years.
We raised three specific concerns about that piece:
Speed, yes this is only for alters, but form alters can be called many times on a page.
Size/memory, in the beginning there are not many, but this api is much easier than HMIA so adoption will likely go up.
Security, you're hand waving this, but there are escalation attacks related to these functions and classes. why not avoid this concern entirely?
Looking at how you're using it is is so you can just call the ->apply method right?
Rebased, only conflict was with the helper function for update.install from this issue: 📌 Remove UI and routes for the ability to update modules and themes via update.module and authorize.php Active I copied the code for that function again to ensure the update was correct since this is a conversion issue.
I have attached a list of all of the preprocess functions in core.
Good question!
I rebased on head, the last comment was addressed upstream so this should be ready.
Updating credit thanks!
There is no issue with:
#[LegacyModuleImplementsAlter]
#[RemoveHook]
#[ReOrderHook]
They are just ignored.
The issue may be with the new enum and ComplexOrder object in the new order parameter for hook, I think it does get loaded so if contrib converts to the new ordering then is loaded on Drupal 11.1 then those do not exist so there is a fatal error. (The fact that the parameter is not parser does not matter because those missiles will have kept hmia around for ordering.)
I need to confirm this and look at a solution.
I will also look at the extra type tests you wrote.
RecursiveDirectoryIterator / DirectoryIterator does not produce a predictable order.
https://stackoverflow.com/questions/29102983/order-in-filesystemiterator
And the way we look for hooks, we mix procedural and oop in the same scan operation.
This means it is not clear whether OOP hooks are first or procedural hooks are first, if from the same module.
Why would this matter?
We key them by class hook and method.
Collection order should not have an effect.
Your tests look like they are failing due to deprecations.
I hid most of the branches.
nicxvan → changed the visibility of the branch 3485896-donquixote-tests-nicxvan to hidden.
nicxvan → changed the visibility of the branch 3485896-donquixote-tests-baseline to hidden.
nicxvan → changed the visibility of the branch 3485896-donquixote-tests-donquixote-2 to hidden.
nicxvan → changed the visibility of the branch 3485896-donquixote-suggested-changes-part2 to hidden.
nicxvan → changed the visibility of the branch 3485896-donquixote-tests-nicxvan-FAIL-side-effect to hidden.
nicxvan → changed the visibility of the branch 3485896-donquixote-suggested-changes-part3 to hidden.
nicxvan → changed the visibility of the branch 3485896-nicxvan-order-at-call-time to hidden.
Let us know what they say and we can start working towards this if there is agreement!
Created one off yours to fix the first stage just so we can see the full test suite.
I'll take a look at your tests on my branch in a bit.
There is no way to fix this within the current proposal, because naturally there won't always be a pre-ordered list for the current combination
I do not think that's true, but let me work it out, thanks for the tests!
So we add a #[ReOrderHook] that targets a non-existing implementation in a non-existing module.
This should have no effect on the order of the other implementations.
We can check that the implementation exists before adding to the order group.
We had considered that so i want to look deeper.
I suspect there is more, including possibly implementations disappearing
What mechanism are you proposing that does that?
Looks good to me.
Checked deprecation messages.
Read through the change record as well.
But for now I want to reduce disruption.
Too late for that lol.
Honestly I don't really care snake case vs camel.
Things seem to be trending toward camel so I generally lean towards camel. In an issue of this size where we are talking about a whole new approach this feels like noise and can be addressed when we settle on which approach.
More tests
Sounds good.
Order and alter hook implementations at call time, especially for multi hook alter calls.
I still feel like this would be a mistake. If you want predictability we want as much ordering during build as possible. This will also be more performant.
As mentioned earlier, ordering relative to a specific implementation easily breaks when that implementation gets renamed.
Ordering relative to all implementations of a module might have unintended effects if one of these as a "first" or "last" order applied.
Yes, this is a limitation, but in the interest of scope we should try to minimize changes.
In my older proposal, there was the possibility to order relative to a module's default odering position.
Feels out of scope and something that could be added later.
Out of curiosity why would you want this?
Another way to approach this is to define an order of ordering operations.
Same scope concerns and as you pointed out it's just punts down the road.
First impression on the further changes.
I'm on the fence about splitting hook collector out, but honestly pass is getting huge. I wonder if we have more files, one that does collection, one with parsing and ordering and we leave the registration in hcp?
No objection from me to remove that on your branch, let me know when it's ready for a deeper look.
Here is the link to the repo: https://github.com/donquixote/quick-attributes-parser/ the one in 18 404s for me.
I don't have a super strong opinion on this bit, it was more that we already have the reflection class so why not use it.
I know that @godotislate is experimenting with updating this elsewhere too.
What do you think of that?
Thanks for all of your help!
I think I can get behind dropping the ability to kill alter, but I think we need to handle invokeAllWith still
This looks great!
Gonna test it some now.
I took a look at your branch and just reviewed HCP directly.
If I read it right you can remove Hook priority entirely right? I'd have to take some more time to look at it, but it looks interesting.
I still think it's far more efficient to store legacyImplementationMap and remove the one or two that need to be removed rather than loop over all thousands of hooks just to build it after, but I wouldn't block the issue on that.
Just to make sure I am reviewing everything, I don't think you made any substantial changes beyond what you changed in HCP and the really minor optimization in ModuleHandler:alter right?
Can you rebase against the main branch so the fork mr is accurate so I can see your changes.
I reopened those two threads.
However, in the one I responded:
Once HMIA is gone this edge case is gone
Maybe edge case is the wrong word, it's more that once this ordering is the only way there won't be a way to order against unknown hooks so we don't have to fall back to legacy ordering. I closed it because of the comment about legacy ordering.
I would still consider that answered.
The other one I'm not sure is an issue either, the last one should win, just like with hmia now.
All of this has to work reliably and predictably, otherwise it should not be added.
It does, and far more that hmia it is easier to understand how to implement your item ordering. It doesn't have to be perfect we can't achieve that in a world where we need to be bc with hmia.
Form alters are not an edge case.
I never said they were, but ordering across multiple layers of form alter very much is an edge case.
If I could cut that out of this issue I would because 90% of all comments outside of typing and renaming are about extra types, and one hook uses extra types.
If you can find a contrib module doing this type of ordering I will gladly convert it to test this further.
Until we find other uses for it, this is all speculative. Modules can use legacy ordering and file an issue if we didn't meet their edge case, it's not the end of the world, we solved a fews issues since oop hooks came out that we missed.
Feel free to add more tests in your branch we have many tests and we have tons of implicit tests with the hmia conversions we did.
Feel free to experiment with refactoring too, but most of your refactoring I've seen are internal implementation details that would be far easier to get in and scope once this is in.
Other than your comment about module handler add i have not seen an architectural question you've raised we had not already considered which is one of the reasons I'm so confident. I'm not worried about module handler add because it has far deeper architectural issues and it's on its way out and this works just as well as it does in 11.1
Rebased.
Rebased
Rebased
@aaronrus catch already told you the planned release is this week, I'm not sure if that is still the case, but you can patch your site using the instructions here: https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... →
hook_module_implements_alter == HMIA
No worries! HMIA does need to be removed, I don't recall if we need Drush still, I think it hooks into only affect migrations run with Drush.
Happy to have someone reviewing, I only work on this for a personal project so I've not had time to dig in further.
PHPCS fixed
What version of drupal?
@ahmad khader yes please do!
We are designing a public API here, and anything we create will be very hard to remove later.
We can fix things before a release, but in general we should only commit what we are ready to release.
I am 100% confident in how we designed this, but as mention in the MR even though it is an API it is marked internal so we can tweak it. The extreme complexity is limited to only one parameter on a parameter, that gives us the ability to deprecate if we have to change it wholesale, we do not have that ability to do targeted updates now which is why it is so massive.
From what I remember, ckeditor5 would work just fine with "Order::Last" instead of "Order::After".
The main reason why we would try to support this case in core instead of replacing it with "Order::After" is that there are likely more such cases in contrib, and we keep this one as a canary.
I literally tried that multiple ways, it does not work and is the whole reason we created extra_types.
This makes me wonder: Order::First and Order::Last do not allow to specify additional hook names.
How will first and last work if alter is called with multiple hooks?
The same way it does now if you only move your implementation first without doing any array searching and slicing like ckeditor5 does.
Basic concepts
This section is generally correct.
(Consequence: When the class or method for a hook implementation get renamed, the ordering attributes that target the old name no longer work.)
Yes, but when doing relative ordering you have to know about the implementation you are ordering relative to.
Alternatively, a relative ordering can address module names, which will target all implementations by that module.
For the hook providing the ordering directive.
Conflict resolution
Yes, that is how it works here.
Extra hooks logic Tbh I find it hard to follow the "extra hooks" logic.
Yes, it is hard to follow.
I also suspect that developers who want to apply order attributes may not fully understand the consequences of adding or not adding hooks in the order extra types.
I can almost guarantee that almost no one will ever need this, it's needed in core so we had to provide it, this solves core's usage, proven by tests. That should be the bar for acceptance for this bit of the feature.
At the same time I had the suspicion that it might not behave as we expect in many scenarios.
But it is hard to point out possible flaws when I might just not correctly understand it.
I can also all but guarantee that anybody trying to use this will probably not need it and it will be wrong.
We can't build this feature for what might be or how people might use it, it's far, far too much of an edge case. If someone opens a bug against it I will happily chase it down and try to find a solution, but we can't make guesses in this area, if it were not for HMIA supporting something like it, we would not have built it in the first place. As it stands we do need it unfortunately.
The bar for this really should be does it work in core, can we address contrib if they need it? The truth is if there is a contrib module doing some ordering on extra types this doesn't work for they can keep their procedural implementation and we can work on solving their use case before Drupal 12.
Naming of methods and variables
Not going to address anything here beyond what I've said before, if other's agree we should rename anything I am happy to change to a consensus. I'm not going to rename anything further before that. See IS for current options.
Test coverage
There is extensive test coverage, further, we have converted all of core which has some of the most complex HMIA implementations. I'm confident we have enough tests. This is a large enough issue that there are gaps, but we cannot find them without contrib using it which requires merging this.
Follow-up issues?
To be fair my core contribution experience is more limited than yours, but from what I've seen everything I've called out as a follow up falls within standard usage. I mean look at the size of this issue, it's already at least 5 times larger than average and far far more complex. There are some edge cases we know about, the sooner we get this in the sooner we can track those down. Needing to rebase a 2000 line MR to try to solve theoretical edge cases is not the way to progress.
Alternative: Order at call time (cached)
Runtime ordering is where almost all of this complexity comes from, so I would be strongly against adding more of that in the future.
Simple enough change for a massive improvement:
Here is an intentional failure to show the output: https://git.drupalcode.org/issue/drupal-3510927/-/jobs/4556292
nicxvan → changed the visibility of the branch 3510927-test-misspelling2 to hidden.
nicxvan → changed the visibility of the branch 3510927-test-misspelling to hidden.
The comment was straightforward and addressed.
I've been following and reviewing this issue for a bit so I agree rtbc.
There is already a pull request with a fix.
@donquixote did a deep dive and has a lot of suggestions. Some are substantial that I need to review and give them due consideration. First review, I think one or two might be a gap that needs to be addressed before this is merged. I think most of them are just an alternate way of doing things, not sure if they are better, worse, or equivalent without more thought.
Many are still related to typing and naming, before going through that I'd like a consensus on a couple of questions:
1. A lot of the typing is on parameters that have not changed, I think that would be out of scope for this issue, should we avoid that?
2. Same for constructors being moved to multiline, I prefer this myself and I know coding standards allow this, but I'm not sure it is in scope here.
3. Suggesting renaming ComplexOrder with RelativeOrder or RelativeOrderBase, I'd lean towards RelativeOrderBase.
4. Rename value for ordering to direction or position, I'd lean toward position.
If I get consensus on 1-4 I'm happy to apply them.
We started using this in the hook ordering issue here: 📌 Hook ordering across OOP, procedural and with extra types Active
Looks great! Not sure if this needs a change record or not.
Please update the mr
Can you help me understand how that is related?
Is that answer acceptable?
There were two very long discussions in slack relevant to this.
The first regarding naming of extra types and other variables: https://drupal.slack.com/archives/C079NQPQUEN/p1740408731650039
All variables have been renamed.
There is still a point of contention about extraTypes on the order object. I have suggested relatedAlters. Since this is the second rename I will not do this unless there is more consensus. I will post more thoughts on this later.
The second discussion was around higher level feedback most recently about ordering around extraTypes and ordering multiple implementations: https://drupal.slack.com/archives/C079NQPQUEN/p1740590792125979
Before I dive into that further I want to highlight the purpose of this issue and why it is so large.
The purpose of this was to provide the ability to order hooks and deprecate hook_module_implements_alter. In order to deprecate HMIA we need to replace all of it's functionality:
- set a hook first
- set a hook last
- order a hook before another module
- order a hook after another module
- remove another module's hook
- reorder another module's hook
- reorder a hook relative to alter hooks with extra types
That is a huge ask, especially when there is the constraint of handling this in a BC compatible way so current modules that have not converted yet can still order hooks relatively. This issue has been able to achieve that.
I want to also highlight that an inordinate amount of attention has been given to ordering relative to extraTypes (formerly orderGroups). We have to support this to deprecate HMIA because core does do this, and inevitably some module somewhere does to. However, I cannot stress enough how rare this should be. However, as complex as this is, the situation is significantly better because now it is part of a discreet object and parameter so if we need to change it in the future, we can deprecate. As part of HMIA it's all or nothing deprecation so for this stage we must provide that option. I will likely update the IS and CR later highlighting how rare this should be, and if you find yourself using extraTypes, you should take a step back and consider deeply if you really need the approach you are taking. It did take the most time since it's the most complex bit, and it has faced the most scrutiny, but we know it works with ckeditor5 and the tests, and it really should be the least used bit of this. This is also why I will hold off on renaming again unless there is wider consensus, it's a lot of thought and attention something that will almost never be used, and it is named after how it is used in ModuleHandler::alter.
The other point of contention in this thread and slack is that if a module has multiple implementations they want to order that order is lost. However, this should be a follow up, this is a current gap in implementation, and HMIA cannot order against multiple implementations anyway. The scope of this issue is so large we should not take on pre-existing bugs that are for a feature that is brand new and will likely not be used. Again, it is an edge case of an edge case and we have a follow up already for it.
Now to address some specific points in the previous comment:
We should avoid adding features and extension points that we later remove.
We don't need to do everything in one issue, but we should know in which direction we are moving.
In general I agree, however I don't think that applies here for two reasons. 1. I strongly doubt we will remove this unless altering with extra types goes away and that's not happening anytime soon. and 2. this situation is much, much better because if we do change it we can deprecate just that one piece of ordering.
Can you remind me what is the "initial OOP hook issue" and how it failed? Maybe I missed it.
The one that is in the IS is where we discuss how it blew up
🐛
Ordering of alter "extra hooks" is a gigantic mess
Active
A couple of final comments on the approach you mentioned using priority in the slack message and why it blows up. If you use priority then you have to exclude that hook implementation from hook ordering in HMIA which means there is a line in the sand you cannot legacy order hooks using priority so you force other modules to update if they need to order relative to you. With our relative ordering approach it is far less disruptive.
My point is the more we diverge and the less we reuse from symfony events, the more we have to ask what we gain from using the symfony system, or if it would have been better to do something custom.
I didn't want to add too much here since there is already information overload, but I suspect we'll likely go in this direction 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active But we can't start that until this gets in.
To me this MR is incomplete if we don't properly support multiple implementations per module. It could even reveal shortcomings in the approach.
This is out of scope for an issue already this big. This already cannot happen, it remains to be a problem, but once this is in it will be a smaller issue to fix.
I really think this issue is close to being ready, it has been looked at by several people in depth. We converted all core hmia implementations so we know they are covered, and we have significant test coverage. I am starting to worry about attrition for this issue, let's not perfect be the enemy of an issue that replaces one of the most complex and problematic hooks.
Read through the CR, it looks clear to me too!
Created two follow ups
Thank you so much for your review and this comment, you've obviously put a lot of thought into this topic over the years and this issue specifically.
At a high level, I don't see anything here that should block this issue, once this is in there is some opportunity for clean up / expansion, but this is already far broader than a typical issue due to the nature of HMIA. I truly think for this issue the thought should be
Most of your comments could be addressed in follow ups that are far more narrowly scoped.
I will create those follow ups shortly with some details to come later.
We are using the symfony event system, but then we are reinventing the way that these events are declared, how we specify ordering, and how we invoke them.
Yes, hooks are different so the divergence is expected
Explicit vs calculated numeric priorities
These are not subscribers, so again them being different is expected. The initial OOP hook issue did use explicit priorities and that blew up which is why it was pulled out, this issue was specifically borne out of a BC way to allow ordering relatively.
Relative ordering
This section only applies when it goes through the legacy ordering, and HMIA can't handle multiple implementations so them getting flattened when run through legacy hmia runtime is acceptable, once all modules convert to this system there will not be an issue.
Module name per implementation
Yes, we do that.
Order lost when grouping by module
While true, let's handle it in a follow up, multiple implementations are new, going to be rare, and ordering them as a separate entity will be even rarer. That follow up scope will be much more narrow and easier to handle. Let's not hold this up for an edge case of an edge case of a new feature.
Ordering conflicts
I agree with your assessment, this is not a blocker, this can also be solved with module weights, or by using ReOrderHook which was not previously available.
Alternative
I think this is pure follow up material, again I and Ghost have ideas for further clean up too, but as I've mentioned, this issue scope is already huge, and we know it works, let's not get bogged down in perfection.
Took a pretty good look at this, there are two comments to address still.
I think this needs a change record to let contrib know they can use this too I would think.
Feels pretty close otherwise.
Is the inverse also true where users without permissions can create menu items through the node edit page?
I've been thinking about this, I wonder if the breakdown in 14 is the best way to do it?
The issue I have is for less experienced developers, the breakdown I wrote up in the IS is a pattern even less experienced devs can follow The edge cases like language_fallback_candidates_entity_view_alter really make it more difficult.
Repostponing for the reason in 14.
Nothing preventing me from working on the plan while postponed.
All comments have been addressed, I've also manually tested.
Test only fails as expected.
I think this one is good to go!
Thank you for that write up! I ran through A and B on both 11.x and this branch and confirmed the expected behavior on both the front end and the db level.
There is one remaining issue which is out of scope here, and exists on 11.x too.
If you publish a node with a draft menu item using the content moderation form instead of with a node save the menu item isn't published.
There are two action items on the MR still, but once those are done I think this is ready.
Rebased and also addressed @berdir's last concern.
Looks great!
Couple of comments, I didn't provide a lot of detail feel free to ping me on slack if you need clarification.
Sorry which branch? Can you hide the ones you're not using?
Personally I never want to see whitespace aligned.
It is different in an actual table cause there are lines for cell borders which is not the case here.
I really struggle with whitepace aligned assignments.
Ok that has been pushed up
nicxvan → changed the visibility of the branch deprecateHMIAFresh to hidden.
Great thanks! I'll pull that in and then update the IS