🇺🇸United States @nicxvan

I took a look, in general it looks like this is going in an interesting direction. I ran the test only job and it fails as expected.

I did not review the test.

Had a few questions.

I've been eyeing 📌 Extension System, Part IV: Properly register all installed extensions/namespaces during container generation Needs work because it's likely on the road to getting oop hooks in themes which will let us clean up a fair bit around preprocess.

I've been eyeing 📌 Extension System, Part IV: Properly register all installed extensions/namespaces during container generation Needs work because it's likely on the road to getting oop hooks in themes which will let us clean up a fair bit around preprocess.

nicxvan → made their first commit to this issue’s fork.

I think this is ready again, I'll give @donquixote a chance to respond, but this cleans up the extensions and gives us a way to tie to documentation.

We can also add to it if we want to extend further.

This looks great! I have to review more closely, but this already covers the three I was reviewing.

Oh nice!

I think @godot is on to something. I pushed up a test case based on the original report and could not reproduce it manually or in the test.

nicxvan → made their first commit to this issue’s fork.

I found this: https://stackoverflow.com/questions/68667235/what-does-the-composer-prov... but no documentation on composer directly.

Can we close this then?

Do we still want this? How will it work when core modules are moved to contrib?

I think this is safe to close, there is no rollback method that is mentioned in the issue summary, a different solution was committed.

Is this safe to close now?

Which one is the one to review?

If I am not mistaken I think for batch we already can use methods as callbacks.

We could explicitly deprecate the procedural option or just let the .module deprecation take care of it and mention how to move them in that CR.

Still need to look at the other two.

This is so much cleaner! It allows the documentation to be declarative, removes the workaround in extending Hook attributes and will allow us to drop Implements hook_form_FORM_ID_alter while maintain the api links.

Are you able to provide a test case so we can reproduce and fix this?

There is something going on with the file system, all of the failures are related to that, I don't see what is missing in the conversion though.

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

If we do remove this do we want to remove it from all hooks?

If I am being honest the only novel thing it provides is the ability to specify another class and method as the implementation, but I'm not sure why you'd need to do that.

As for the main reason is so you can place the attribute on the class, but then you have to have an __invoke method. Since we already have that restriction, why not drop method entirely, then if someone does not want a specially named class they use __invoke which already have to do.

I really like the direction this is going!

Two comments on the MR that need to be addressed then I think we can mark this as ready.

There are a few other comments, but they have been responded it in a way I think we can consider closed.

This cleans up the extensions a lot and lets us still get the documentation for the api module.

I think there is something to be said for keeping them close so it's easier to switch between a and I'm not sure it's that much more complex, but I don't have a super strong opinion on this.

I'm not sure if we want to move this to the gathering stage.

Constructors do not need documentation if everything is typed I think.

I agree DI should not be a blocker.

I think this is ready, all of my comments have been addressed and this means work all of the changes the actual registry is smaller than 11.1!

Also wonder why we're marking child issues as related.

I've never been super clear on the difference so I usually default to related.

As for system_requirements, I'd love to get that in, but it's massive, and I feel like the template preprocess conversions will be easier to get in.

Probably needs sign off for the api change.

We just need to keep the api module in mind, see the linked issue ✨ support for OO hooks in classes Active we need some way to tie the sub attribute to the correct hook documentation.

yes, thank you!

PackageManagerRequirements has some minor duplication as well.

Applied! Thanks

Pretty straightforward

Ok, I think this is ready.

I tested manually both in 11.x and this branch locally.
I tested block and view contextual links and tested with both mouse and keyboard. As far as I can tell both act exactly the same.

I searched for references to backbone, the only references remaining are for toolbar. There are no more references in contextual.

Javascript is not my forte as mentioned, but I am familiar with it, reviewing the code directly I see nothing that looks amiss.

As far as I can tell both larowlan and _nod have worked on this to a non trivial extent.

The only thing I didn't check that may be worth it is performance. I'm not super familiar with JS performance testing so I'll leave that up to a committer to determine if it's necessary.

_nod's comment in 56 make me think the change has been addressed as far as I can tell.

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?

nicxvan → created an issue.

This looks great, test is more thorough and I see you managed to fix empty hook implementations not being cached at the same time!

Created 📌 Better document when to use setModuleList Active

nicxvan → created an issue.

✨ support for OO hooks in classes Active is where some of the discussion happened too

@donquixote pointed out the missing paramter.

Created 📌 [10.5.x] Add BC stubs for Hook ordering. Active because creating 10.5 from here was too out of date.

nicxvan → created an issue.

nicxvan → created an issue.

When composer is on a different server like ci you can run into issues.

I ran into an issue a while back where I installed search api and it had a php requirement I had locally in ddev and in the CI tooling but not production so the site broke, this key would have prevented it. Most modules use hook requirements though for this a key might be easier.

Some things are missing in the CR and the proxy class likely needs to be regenerated.

Also had a question on the new service.

I did a first pass here, nothing jumped out immediately, it's pretty clever, I definitely need to run through some things to better understand some bits though.

This looks great, I confirmed it deprecates what it claims, all messages look right.
Confirmed the CR outlines all deprecations.
All deprecation messages look correct.

Added a few questions, I could do suggestions for some reason so here is a patch with the changes I was testing.

📌 [Plan] Determine how to deprecate procedural hooks. Active is where we discussed procedural hook deprecation.

If I am being honest I'm not sure I see how we can directly deprecate procedural hooks, it will more likely be a side effect of deprecating .module files. There just isn't a clean way to know whether a function is a hook or not. We can throw a message runtime in LegacyHook, but that seems riskier than we would want to be.

Side note, hook_module_implements_alter and hook_hook_info are also strange deprecations because we are just removing them in 12.

I think Don meant set module list would not have an effect.

The code in the update is not important to test around either. It is explicitly to prevent views from loading way to many things during update before certain registries can be cleaned or set up.

Module weight still should matter, there is an issue for alters and module weight we still need tests for. But this is not the issue.

I can roll a 10.5 specific if someone doesn't before me.

Thanks for the credit!

larowlan → credited nicxvan → .

I think this is a duplicate 🐛 Order of alter hooks is not always respected by ModuleHandler Needs work

I think am easier way around this is to deprecate and replace the things inside the files.

.module has very few things that are required in it. We can make this oop then just deprecate .module entirely.

Well, hook_hook_info is now deprecated is there another way to do this?

I think we will likely deprecate the inclusion of .module first.

Is this eligible for the no test policy?

This can be closed as duplicate once credit is assigned.

ThemeHandler::rebuildThemeData()

is deprecated, can we close this?

Do we still want to do this?

I think this actually belongs in the devel module.

This definitely isn't an extension system.

Moving it to layout builder for now because the devel module doesn't support issues.

This one can be closed it no longer exists!

Since composer is the only recommended way to install can we close this?

It's in, I think we can close this, just not sure on credit.

I think we can close this.

There are no arrays that don't have the shape defined. After the hook ordering issue.

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

Also it's this a contributed module?

🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active is removing ProceduralCall entirely.

Ok I need to actually write the CR.

I'm not sure how I feel about making it protected, it is technically api now, not sure why we want to restrict this further.

I also am not sure about the deprecation message either.

The hook order issue and preprocess issue both got in and both affected module handler.

Would this be related 💬 Drupal 11.2 upgrade causes \Drupal::$container is not initialized yet error Active
We're having trouble reproducing the issue.

pdureau → credited nicxvan → .

@longwave indicated we might be able to do this without tests, but the work in 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active makes this unnecessary I think.

I wonder how much this will help when you have test module discovery on.

Is this eligible for backport to 10.x?

I updated who the CR targets and did a quick review of the 11.x MR

Nightwatch fails often, you can rerun it.

As for test only, you don't usually need a separate branch, there is a test only job, it's at the bottom of the list of tests.

If you run that it will run your tests without the fix or feature.

It should fail.

But in general, we should always use the phpstan-like array type doc for any new array property, parameter or return we introduce.

I do not agree, but this discussion should happen in a coding standards issue.

Feel free to link one or create one if one doesn't already exist.

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

I created an MR with the fix @godotislate suggested in slack.
I'm not sure that this is really testing the fix now though since it's a new moduleHandler instance.

To be honest I'm not sure if this fix needs a test for the new bugfix policy, this will be very hard to test, it's easy to see it's missing though.

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

It's in dev

I can merge it into a dev version

Sharing the modules you have installed is a good first step, we need to identify the cause so we can fix it properly.

Yes, I created that one for you to test with. That reverts the only change I made in this issue after your work.

No problem, everyone starts somewhere!

To find the actual version installed you want to look for that entry in composer.lock

            "name": "doctrine/deprecations",
            "version": "1.1.3",

composer require doctrine/deprecations:1.1.3 should work. The #support channel in drupal slack is probably a better place to get assistance.

Tests just started, but this should be our target, if there is a module like block_content that only has template_preprocess we can take care of it in that issue.