Looks great!
Took a minute to look up the callback, it reads like a hook but it's not.
Sorry about the noise, I was just trying to link where it looked like the failures started and where I was seeing them.
I should have realized it wasn't this issue since this issue introduced those tests and would have been failing so it wouldn't have gotten in.
Also that is impressive on the klaro changes!
Took a couple of reads to see how it was working, but I'll keep it in mind if I ever need this.
That's a tricky test.
Coverage looks good and fix looks good.
I'm running test only just to be sure.
Our to help our future selves hook_entity_duplicate and hook_entity_ENTITY_TYPE_duplicate
Love to see new hooks!
While I appreciate @godotislate's idea of discouraging procedural hooks, @berdir is correct we need to do them all at once.
Only question at the moment is semantic, duplicate is already a verb, shouldn't the hook just be hook_duplicate, and hook_ENTITY_TYPE_duplicate?
@frank.dev just to clarify in case you only deleted that one file you should follow the instructions posted including step 4 in the link @longwave provided. Otherwise there is no way to know how many removed files still remain.
You should also consider using composer every where because soon that is going to be the only way to update.
My comments have been addressed and this looks great!
I would say for the future for these types of issues I'm not sure if i would introduce DI like this, it did make it a little harder to compare and since you have to pass it in from the container it is not super helpful on the DX side either.
Since it's just tests and opinion I don't think it should block the issue, but it will make future reviews easier if you with in other modules from the meta.
@catch thanks!
@heddn in principle yes, however in this case if you see the fix the value for the performance test was not correct so every test on HEAD was failing. If you check the referenced issue @nikolay only move code from a procedural function to a helper in a test module, there was no functional change, so no way for that to affect this performance test.
Can you please put that code on a merge request? Tests don't run against patches anymore.
This is causing other issues to fail the pipeline due to performance:
📌
Move helpers in node_access_test.module and delete it
Needs work
https://git.drupalcode.org/project/drupal/-/merge_requests/10633
#3493406-26: Add render caching for the navigation render array →
That probably needs to be addressed elsewhere. I'll keep an eye on this though.
Are you able to add a test for this?
This is great thanks!
One comment on the MR
Also even if they seem random tests do need to pass so it's best to rerun them.
It's so the api project can find them and document implementations, there is an issue to replace that.
Thank you for commenting.
Re 6 I think there are a bunch, especially if you have a shared helper method.
For 7, I agree, but what would you suggest for comments then? A description of what it is doing?
I think it's funny that issues I created were used for pro and against increasing the patch size.
I think it should be increased a bit from where it is, and while it can get hard to review after a certain size, tools like hiding sections do make it easier.
Here is a third example.
I am working through an issue that it was requested I break up now too.
📌
Mark core modules as converted to OOP hooks
Active
📌
[pp-1] Mark several more modules as converted
Active
I think the first break up makes sense because the first issue was an attempt to mitigate the memory leak and the issue was really doing two separate things to that approach.
The second issue is being requested to be broken up into 3 new issues.
With the tools in gitlab it feels possible to review that as one issue especially since it was rtbc already in the parent issue.
I'll likely break it up when I get some time just because it was requested, but I think that issue should be considered acceptable.
As for the meta smustgrave mentioned I did combine a couple but quickly realized the remaining had too much to do in each so I will end up splitting them up as I work on them as well.
@chi please provide that feedback here 📌 [meta] Clean up hook classes in core Active
Thank you for sharing your thoughts. I think there are a few approaches we can take and we just need to choose one and work on it.
Before I lay them out, just to clarify FileRuntimeRequirements could have been named anything, it's a hook class so the class name is arbitrary, it's the #[Hook('runtime_requirements')] that makes it work in the current iteration.
Some of these points are duplicated because they are relevant to more than one approach.
Approach 1
Special class for Install time requirements, new separate hooks for runtime and update requirements.
Pros:
- It's already done
- It's straightforward to convert from current hook_requirements to this system even in complex places like system_requirements as evidenced by @berdir's exploration there
- Install is no longer a hook so you get separation to call out it's unique
- We can document limitations of each phase better than we have in the past
- We can eliminate hook_requirements
Cons:
- It is a little confusing that one is not a hook and two are
- It is harder to share code between phases (especially if one of those phases is Install)
Approach 2
Special class for Install time requirements, new hook that combines runtime and update.
Pros:
- It is easy to convert the current MRs
- It keeps update and runtime closer to what they are today
- It's straightforward to convert since any logic you have for both runtime and update can be copy pasted
- We can document limitations of each phase better than we have in the past
- We can eliminate hook_requirements
Cons:
- It is a little confusing that one is not a hook and two are a new hook
- It is harder to share code between install and the hook
- I think it is confusing to have multiple phases in one hook
Approach 3
Special class that has a different method for each phase. Documented way to do injection that is safe for install time.
Pros:
- All requirements are in a consistent place
- We can document limitations of each phase better than we have in the past
- We can share code between phases easily (unless we share something using services with install)
- We can eliminate hook_requirements
Cons:
- It needs work
- It will be far more complex to set up dependency injection for update and runtime because you always need to be safe for install even if you don't use it
- Integration where update and runtime are called is far more complex
- We can share code between phases easily, this means it's more easy to add a service that isn't available install time
Approach 4
Something else
Additional thoughts
A few other points, in most core modules I noticed that modules use one phase.
That means any of these approaches will work equally well for those.
The more I think about this the more I think that these really should have never been combined. The things you can do with requirements is very different in install than the other two.
I still think Approach 1 is the clear winner with approach 2 following close behind.
I think Approach 3 is only viable if we're using it as a bridge to completely overhaul the requirements api. I think the complexity it forces on runtime and update along with the complexity it adds to where we invoke them makes it not worth it here.
If I'm missing something let me know, while I do think approach 1 is the way to go, my main goal is to deprecate hook_requirements and I don't use hook_requirements that often myself so if others think one of the other approaches is better we can work towards that.
I'll split media and library and then system to their own
Yeah I had left this in needs work because I knew this needed the test fixes in the other issue.
This has been merged.
Looks good now!
Not really a follow up, but I created the profile issue here: 📌 Convert profile hooks Active
Also don't convert here most of these are intentional.
I converted one of these so there is a conflict.
Also I need to create an issue to convert the profiles.
I think that's most of them!
Thanks! I added the other issue as a child and the other follow ups are already referenced.
nicxvan → changed the visibility of the branch 3491275-more-complex-mark to hidden.
Both branches need review now that they are split up: 10612 is green and simple. 10613 is more complex. It will likely fail because it will need the test changes in 10612.
MR !10612 includes the following:
It is all the simple changes that only include the services.yml
It includes the HookCollectorPass change to read the value.
It includes two tests that need updating
MR !10613 includes the following:
All other changes including moving / converting the hooks that need to be moved.
nicxvan → changed the visibility of the branch 3491275-mark-core-modules to hidden.
nicxvan → changed the visibility of the branch 3492391-early-fixed-event-dispatcher to hidden.
Please take a look at MR 10598
Yeah this should be closed works as designed.
This is required in some places.
Yeah I found these recently, but the Registry needs work to convert theme or theme engine hooks.
I'll follow that other issue too, I hadn't seen it.
Pretty sure this is intentional and required: #8-75: Let users cancel their accounts →
What I mean is you're jumping through all these hoops to get the invent dispatcher in place. But you're only calling a fixed output for that.
You should just replace this with a utility class that before bootstrap calls the static helper you're creating preBootstrapSubscribers to handle.
So the helper would call the static preBootstrapSubscribers if you are prebootstrap and then call the event dispatcher as it does now after.
You get the benefits you're looking for with way less code.
If you are trying to make this unified post bootstrap then why not have a utility class which you can call instead of the event dispatcher and does a fixed call before bootstrap and dispatches an event once the container is available?
Went through it a couple of times.
Just one question in the mr.
The comment has been removed but now it's part of the array.
That's actually better in my opinion cause now it's data.
If you added the needs tests tag to test the deprecation we don't need tests for that.
https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →
If it's for something else let me know.
Ok I hope it is ok to still mark this, I only updated test variables to the ones they were clearly meant to be.
I went through it and it looks right.
I manually tested it too.
The cache contexts look like they were updated properly
I did change the variables in the test because they were named wrong.
Tests pass and the test only job fails.
Discussion and questions are welcome here: 📌 [meta] Clean up hook classes in core Active
I'm going to see if I can refresh this and take a look if the feedback has been addressed.
I'm hiding the patches so we can make sure the work stays in the same MR the patches seem to have diverged from the MR so I created a new one starting with patch in 50.
nicxvan → changed the visibility of the branch 3023527-add-view-any to hidden.
My thinking in these cases is that it would move to a helper.
If it's in the update or requirements then it can just be a helper function in the hook class.
If install is one of the shared phases then it can be a static helper function.
In your example I would create a static helper method that would call this:
if (!extension_loaded('bcmath')) {
$requirements['commerce_bcmath'] = [
'title' => t('BC Math'),
'description' => t('Commerce requires the BC Math PHP extension.'),
'severity' => REQUIREMENT_ERROR,
];
}
Then just call that static helper in each one.
Another option is to move them all to a requirements class and remove the whole hook_requirements, then we have a method for install, update, and runtime, then any shared functionality like this is an internal helper.
We likely wouldn't be able to inject anything, but that is no worse than what we have now.
nicxvan → created an issue. See original summary → .
First let me start with the main driver of this.
If we ever want to deprecate procedural hooks we need to do something about hook_requirements.
I initially split it like this because modulehandler already invokes requirements('update') and requirements('runtime').
Then I split install into a special class because it was called directly as a magic function in the install processes.
Next I reviewed core requirements and noticed almost all interact with one phase, system requirements is a beast, but berdir did a naive conversion to this structure and it passes:
📌
[pp-3] Split up and refactor system_requirements() into OOP hooks
Active
Now to address the comment here:
I think we need to make it possible for the same requirements code to be run on install, update and runtime phases so, for example, we don't have three different hook implementations that do the PHP extension checking that's done currently in system_requirements().
I don't think that's possible, this gets called during profile dependency checking as well, this is another reason I split them, they have been combined, but they do have different purposes.
If they are separate we can begin to determine what things are safe to inject for each phase.
The idea of the phase being runtime and there's not always a kernel service is mind boggling.
That can't be right, but I bet it's due to being mixed in with system requirements.
I didn't see why that was added, but it was done as part of this
#2384675: Deprecate conf_path() →
I think hook_requirements should be split just to remove the questions of what is safe to inject per phase and to remove confusion on what is happening in a given hook_requirements.
Personally I'd postpone the rearchitecture to followups once we have the scope of what is available to each phase.
If there are existing issues discussing long term updates I'd love to read them and see if there is a way to incorporate those here.
I'll reply on the meta.
The short answer is the different phases have different requirements for what can be injected. Also almost all modules only interact with one phase so this would be a clean move.
I'll reply more fully on the meta.
I went through and confirmed these are all supposed to be void.
It's also been added to the docs in the *.api.php files.
Since you went through updating this module I'd love your insights here 📌 [meta] Clean up hook classes in core Active any thoughts on ways to group them etc or other thoughts or observations would be great.
Since you went through updating this module I'd love your insights here 📌 [meta] Clean up hook classes in core Active any thoughts on ways to group them etc or other thoughts or observations would be great.