[ignore] Convert everything everywhere all at once

I think we have a path forward here:

apply patch to convert some that need manual conversion e.g module implemented alter and the alter deprecation
Then run conversion
Then apply test fix patches in test conversion issue
Then apply patches that revert conversions that can't be converted yet not created yet but this includes tests like the one for .inc loading

Wrangle coding standards, initial testing this is probably a few minor things.

Check for any failures that arise from all things being converted and integrate into above step.

Run process during code freeze if possible.

All tests have resolutions except the functional htaccess test.

nicxvan → changed the visibility of the branch 3483599-conversion2 to hidden.

Conversion 3 patches used attached.
Order matters review the gist.

Ok after the automated conversion, patch application, and new baseline it looks like there are three functions in the baseline that shouldn't be there, not sure why, but that's pretty minor, let's see if there are any new test failures.

Many, many failures, mostly due to function calls missing, it seems that tests call hooks directly more than expected. I'll see if we can address this with rector or identify another pattern.

The test conversion is working though so we can always fall back to one bulk update of the tests, then group the modules as needed.

ghost of drupal past → made their first commit to this issue’s fork.

Good news I unintentionally deleted some proxy files.

We have some patches and a follow up.

Another patch FileTestHooks

nicxvan → changed the visibility of the branch 3483599-ignore-convert-everything to hidden.

ok I have converted everything.

Patches are out of date

nicxvan → changed the visibility of the branch 3483599-conversion3 to hidden.

Great news, only minor codestyle issues.

There are another 25 functions that were converted, but those fixes are minor I can add patches for them.

nicxvan → changed the visibility of the branch 3483599-conversion4 to hidden.

Tests are failing now mostly due to how I incorrectly fixed the missing hook functions now that they are methods on classes.

I need to call the actual hooks directly still. I'll update my local patch, that will take care of the majority of the issues.

nicxvan → changed the visibility of the branch 3483599-conversion5 to hidden.

Ok missing functions should be all fixed now, I also have a bigpipe patch so that is no longer manual.

0003 has an update coming, but in order to not lose anything here are the patches used in conversion7

Also 8 doesn't apply, but it's super easy to just fix the bigpipehooks arrays manually.

nicxvan → changed the visibility of the branch 3483599-conversion6 to hidden.

nicxvan → changed the visibility of the branch 3483599-conversion7 to hidden.

Patches are out of date.

nicxvan → changed the visibility of the branch 3483599-conversion8 to hidden.

nicxvan → changed the visibility of the branch 3483599-NOCONVERSIONS to hidden.

nicxvan → changed the visibility of the branch 3483599-conversion10 to hidden.

nicxvan → changed the visibility of the branch 3483599-conversionPatchReroll to hidden.

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

nicxvan → changed the visibility of the branch 3483599-patchReroll1 to hidden.

nicxvan → changed the visibility of the branch 3483599-nov1-systemOnlyNoPatches to hidden.

nicxvan → changed the visibility of the branch 3483599-conversion9 to hidden.

nicxvan → changed the visibility of the branch 3483599-nov4-fullAutomated1NewHMIA to hidden.

nicxvan → changed the visibility of the branch 3483599-nov1-fullAutomated1 to hidden.

nicxvan → changed the visibility of the branch 3483599-nov4-fullAutomated2 to hidden.

nicxvan → changed the visibility of the branch 3483599-nov4-fullAutomated3 to hidden.

This looks amazing, well done.

A couple of thoughts/questions:

1. With things like

-/**
- * Implements hook_rebuild().
- */
-function block_rebuild() {

This shouldn't be considered API, but I would bet that someone somewhere is calling things like this directly. Should we leave the existing code in place and apply the #[LegacyHook] attribute to it?

2. In turn this made me consider BC. If you were hook_module_implements_alter'ing to remove or reorder core hooks before, now we have broken this I think? But if we have the #[LegacyHook] attribute could we add metadata to point to the new OO hook name - and then we can provide a BC layer based on this information?

Thank you for taking a look!

1. I really, really would prefer not to globally for core keep the Legacy, However, it may make sense to keep those for a select few we thing might be problematic like this one. Would it be acceptable to move that to a follow up so we can highlight them?

2. We had to circle back to handle BC for hook_module_implements_alter, that ticket is ready for review with tests here 🐛 Ordering of alter "extra hooks" is a gigantic mess Active (that is merged into this branch, it's required to make things like ckeditor work.

I think the problem with #75.1 is that we don't really know which procedural hooks someone might be calling directly; we could search contrib but we have no idea about custom code. The BC policy → says that hook implementations are not part of the BC promise so perhaps we just hard break by removing all these, unless there are a small number that are widely used - but again I wouldn't know how to find that out.

The "gigantic mess" has put me off 🐛 Ordering of alter "extra hooks" is a gigantic mess Active :D but I will try to look soon!

I think it is OK for us to remove the procedural hook implementations - we do that for individual hook implementations every so often, and it's very clearly documented that you shouldn't call them directly. Also it will be an easy thing for a module to adapt to. My concern with leaving all the procedural hook implementations in is that we'll be doing hook discovery on those files until Drupal 12 then, which anecdotally has put our memory requirements through the roof. Core is big enough that fully converting core might mitigate some of this, got another issue open to hopefully mitigate it further but it's just an idea with no code still.

I'd love to just do a hard break, we can add examples in the CR that this will inevitably need.

I've been meaning to update that title, it has now been changed, but it is fully ready for review.

There are some optional follow ups there as well, one of which is RTBC as well.

If you were hook_module_implements_alter'ing to remove or reorder core hooks before, now we have broken this I think?

I think this will actually be OK after 🐛 Ordering of alter "extra hooks" is a gigantic mess Active .

hook_module_implements_alter() works on an array keyed by the module name, so as long as a module only implements one hook, the alter will continue to work the same way.

Once modules start having two implementations of the same hook, then it won't be possible to use h_m_i_a() to put an implementation between the two implementation, but that will be a result of refactoring of hook implementations (in 11.2 or later, or contrib) rather than the conversion itself. At that point we'll need a different ordering mechanism which will eventually replace h_m_i_a().

I should also note, that while 🐛 Ordering of alter "extra hooks" is a gigantic mess Active has it's own tests, it is applied in this issue so we can see it works with the alters ckeditor needs to work with editor and media.

All remaining tasks are now rtbc.

nicxvan → changed the visibility of the branch 3483599-nov5-fullAutomated1 to hidden.

nicxvan → changed the visibility of the branch 3483599-nov7-fullAutomated2 to hidden.

nicxvan → changed the visibility of the branch 3483599-nov7-fullAutomated1 to hidden.

📌 Fix invocation of hook_module(s)_(pre(un))install(ed) so they support OOP hooks Active is ready for review and can be rolled into this issue if we want. ( tested here 📌 [ignore] Test some install time hooks Active )

This also includes: 🐛 Add hook_cache_flush to procedural only Active so that would become a duplicate.

nicxvan → changed the visibility of the branch 3483599-nov7-fullAutomated3 to hidden.

nicxvan → changed the visibility of the branch 3483599-nov1-fullAutomated1Installer to hidden.

nicxvan → changed the visibility of the branch 3483599-nov11-fullAutomated1Installer to hidden.

nicxvan → changed the visibility of the branch 3483599-nov10fullAutomated1 to hidden.

I spot checked the first handful of converted modules and the conversion looks really great - all comments and formatting preserved where it needs to be. Overall I agree committing this in one go will be less disruptive than module-by-module, even if a few more MRs have to be re-rolled at least it's one hit instead of over and over again. Tagging with beta target (in the sense of good to do during the beta).

I'm regenerating since 🐛 Ordering of alter "extra hooks" is a gigantic mess Active got merged!

nicxvan → changed the visibility of the branch 3483599-nov11-fullAutomated2 to hidden.

Ok is been updated.

The two remaining tasks were merged, I'm gonna update again.

nicxvan → changed the visibility of the branch 3483599-nov11-fullAutomated3 to hidden.

It has been updated again since the last two remaining issues have been merged.

RTBC - I did not look at every single line of code, but performed isolated spot checks and the conversion seems fully automatic to me.

I am also +1 to do this in one full sweep, so that we can get off .module files ASAP.

This MR also clearly shows that hooks are as easy to implement as before the conversion, so that’s a huge plus.

This will need to be reconverted since some upstream issues updated baseline.

Currently working through getting some of the return type issues in that are converted so I will wait for those to reconvert.

Gonna regenerate now since the two hook return type issues got in that we convert:

📌 Add void return type to all hook_form_alter, hook_form_FORM_ID_alter and hook_form_BASE_FORM_ID_alter implementations Active
📌 Add void return type to all hook_entity_type_alter implementations Active

nicxvan → changed the visibility of the branch 3483599-nov13-fullAutomated1 to hidden.

nicxvan → changed the visibility of the branch 3483599-nov11-fullAutomated4 to hidden.

I've rerolled, all tests are still green.

I also reviewed the snippets of patches applied after the automatic conversion manually.

They all also look good to me.

The change record makes sense and even in D7 it was the right way to use module handler to invoke a single hook in another module.

Still RTBC

Just leaving a note here that @longwave confirmed in slack that his questions have been answered as well.

Thanks again for keeping on top of this.

Discussed this with @catch and @nod_ and we agree it would be good to get this into beta1. As it's such a big change and removes a lot of procedural code it would be good to shake out any issues as early as possible.

I have some nits, but they can wait for followups:

• Return types have a leading space which doesn't follow standards
• Not all methods have return types, this is an ideal time to add them

I also realised that we can infer the hook name from the method name to avoid repetition:

  #[Hook('help')]
  public function help($route_name, RouteMatchInterface $route_match) {

could just be

  #[Hook]
  public function help($route_name, RouteMatchInterface $route_match) {

Even for e.g.

  #[Hook('page_attachments')]
  public function pageAttachments(array &$page) {

we can swap camelCase for snake_case automatically. But again this easily can wait for a followup.

Committed and pushed 8aeb2ca5992 to 11.x and 059e256c285 to 11.1.x. Thanks!

Tagging as a release highlight and also for the release notes as it's such a big change.

Thank you so much!

I can't thank @ghost of drupal past enough for all of his help working through some of the tests and regressions we discovered working on this. I learned so much working through the myriad of issues we uncovered.

I'd also like to thank @catch for his advice and attention on several follow up issues and process for an issue like this.

@godotislate, @lendude, @berdir, @smustgrave and @bbrala all helped me uncover some test solutions, reviewed related issues in a way that was pertinent to this issue or helped with rector.

And of course thank you @longwave and @fabianx for your reviews here.

👏 great advancement!

longwave → credited bbrala → .

longwave → credited berdir → .

longwave → credited godotislate → .

longwave → credited lendude → .

longwave → credited smustgrave → .

I'm happy to work on some follow ups, however:

I also realized that we can infer the hook name from the method name to avoid repetition

I don't think we want to do that for a few reasons:

1. It does not allow multiple implementations which is one of the goals of this initiative
2. It does not allow using the same method for multiple hooks which we are already using
3. It contradicts with point 1 in 📌 OOP hooks using event dispatcher Needs review "No magic naming in the implementations."

Unless you mean infer only if we're missing the hook parameter in the hook attribute, but I think point 3 covers that, it means if it's missing you have to magically name you're method.
We also probably are going to use that for documentation similar to implements so it helps there too.

Well. I think an optional empty hook attribute with the method name converted to snake case as the default hook is okay. It just shouldn't be mandatory. And yeah, it's not usable for either angle of multiples but that's also ok -- it's a sensible default after all, nothing more.. It'll cover a lot of cases and when it doesn't it's very easy to do more. Change $hook to optional in Hook and in HookCollectorPass change:

  protected function addFromAttribute(Hook $hook, $class, $module) {
    if ($hook->module) {
      $module = $hook->module;
    }
    $hookValue = $hook->hook ?: strtolower(preg_replace('/(?<=.)[A-Z0-9]/', '_$0',  $hook->method));
    $this->moduleImplements[$hookValue][$module] = '';
    $this->implementations[$hookValue][$module][$class][] = $hook->method;
  }

So simple ;) but really, this is just 1 line change besides factoring out $hook->hook into $hookValue.

I could create a follow up for that, are you suggesting we do a second pass to update core to use this?

Does this complicate hooks with extra types more?

That's what I am suggesting, it should be a whole lot easier since the rule would only remove superfluous strings from Hook attributes. It will not change anything behavior wise, it's free, no test failures, no nothing. It's just less strings. Not to mention how easy it is to write the rule. It doesn't matter whether it's extra types or not. It could look at fieldWidgetCompleteTestFieldWidgetMultipleSingleValueFormAlter, convert it to field_widget_complete_test_field_widget_multiple_single_value_form_alter check the first arg to Hook and see it's the same, so it deletes it. If it is not the same, the rule does nothing.

It would also skip ckeditor5PluginInfoAlter for reasons outlined above, it's the only eligible Hook implementation which is followed by a number.

Lets discuss that in a new issue? That said, -1 from me on that. It will make it harder to search/grep for implementations and understand for a human what on earth fieldWidgetCompleteTestFieldWidgetMultipleSingleValueFormAlter is exactly. I'd rather do the opposite and have the full Hook attribute and possibly use a shorter method name.

Certainly. I said "if core wants to go that way" and outlined how it could look, what would it take in core and in rector. I am not calling any shots.

It will make it harder to search/grep for implementations

Yes as someone who constantly greps looking for code and who doesn't use an IDE, it would be nice to retain this ability.

quietone → closed merge request !10178

quietone → closed merge request !10032

quietone → closed merge request !10030

quietone → closed merge request !10031

Automatically closed - issue fixed for 2 weeks with no activity.

drush gen hook appears to have been forgotten for this refactor. Still no OOP option for drush gen hook !

screenshot of drush gen hook when using Drupal 10.3.6

I also tried this with Drupal 11.x and it did the same thing as Drupal 10.3.

Drush Commandline Tool 13.3.3.0

https://github.com/drush-ops/drush/issues/6175

It's actually higher than drush, it's the Chi-teck drupal-code-generator
https://github.com/Chi-teck/drupal-code-generator/issues/194