OOP hooks using event dispatcher

Created on 19 April 2024, 8 months ago

Problem/Motivation

For a very long time we wanted to introduce hooks in some OOP manner. The following goals altogether however have proven elusive

  1. No magic naming in the implementations.
  2. A hook implementation should be simple and have minimal boilerplate.
  3. Calling any hook without defining anything should be possible as it is now.
  4. Reordering hooks should be doable.
  5. Relative reordering should be easier. ckeditor5_module_implements_alter is ouch.
  6. Minimal added code to core.

Proposed resolution

Big kudos to EclipseGc for realizing the Symfony EventDispatcherInterface has a getListeners method which allows us to call listeners any way we want. Unlike dispatch this allows us to call listeners with any arguments we want without defining an event object.

Based on this:

  1. Hook implementations go into Drupal\modulename\Hook namespace. Familiar pattern from plugins. These are automatically registered as autowired services. This makes for minimal boilerplate. If autowire doesn't suffice -- should be very rare -- they can be registered manually as well, the service id is the class name.
  2. Hook implementations needs to be marked with a Hook attribute. This is new. Either on a method #[Hook(hook: 'user_cancel')] or on the class #[Hook(hook: 'user_cancel', method: 'UserCancel')]. The attribute supports a priority as well: #[Hook(hook: 'user_cancel', priority: -20)]. This is patterned on the Symfony AsEventListener attribute which is shipped with the event dispatcher but it is only used in the full Symfony framework.
  3. Priority defaults to negative module weight. This was already at hand in DrupalKernel at normal rebuild, for install/uninstall it needed a tiny change to pass it in.
  4. Multiple implementations are totally allowed on multiple axis: one method can implement multiple hooks, one module can implement a hook as many times as it wants in as many classes as it wants. This allows, for example, adding form_alter implementations firing on other conditions than form id without touching any existing implementation. For now I exempted the hooks fired via ModuleHandler::invoke from this, documentation would be needed for those: library_build_info, mail and help runtime. donquixote recommended firing multiple ones and using NestedArray:mergeDeep on the results. We could do that if we wanted for sure. Also install/uninstall/scheme/last_update_removed on install time but I do not know what happens to install time hooks. Might need to remain procedural, making them event subscribers like this makes my brain hurt.
  5. All the implementations are stored in a container parameter. This is a multidimensional array with keys for hook, class, method and the values are module and priority. To reorder, use a service provider alter and change the priority. A proof of concept of how ckeditor5_module_implements_alter will look after is provided. I also added a helper method which retrieves all the priorities for a given hook to make absolute ordering easier. min() and max() will be new friends for Drupal module developers when ordering.
  6. There's little to no concern given to backwards compatibility. New hook implementations fire after the old ones and they are altered and fired completely separately. It is not possible to mix old and new. This shouldn't be a problem as conversion automatically should be possible and easy especially if a hook implementation is marked with the doxygen "Implements hook_foo".
  7. The edge case of "implementing a hook on behalf of another module" is also supported by simple specifying the module in the attribute.
  8. Minimal code is added: less than 200 lines of new code. This handles registration of new services, gathering all the implementation data, registering them as event subscribers and firing them as needed.
  9. If loading all hook classes at build time becomes a problem we can move nikic/php-parser from dev dependency to normal and swap out HookHelper::getHookImplementationsInClass to something like this.

Remaining tasks

Write tests. Probably separate out the module list changes to ModuleInstaller in a separate issue.

User interface changes

API changes

Oh my.

Data model changes

Release notes snippet

📌 Task
Status

Needs review

Version

11.0 🔥

Component
Base 

Last updated about 1 hour ago

Created by

🇨🇦Canada Charlie ChX Negyesi 🍁Canada

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Charlie ChX Negyesi
  • Merge request !7604hook attempt → (Closed) created by Unnamed author
  • Pipeline finished with Failed
    8 months ago
    Total: 188s
    #150964
  • Pipeline finished with Canceled
    8 months ago
    Total: 53s
    #151036
  • Pipeline finished with Canceled
    8 months ago
    Total: 80s
    #151040
  • Pipeline finished with Canceled
    8 months ago
    #151042
  • Pipeline finished with Failed
    8 months ago
    Total: 208s
    #151044
  • 🇬🇧United Kingdom joachim

    > Hook implementations go into Drupal\modulename\Hook namespace. Familiar pattern from plugins.

    +1, good DX.

    > These are automatically registered as autowired services

    Do they specifically need to be registered as services? They don't need to be used as services, and that's going to add a LOT of bloat to the container.

    > All the implementations are stored in a container parameter

    Likewise - that's a lot of bloat, surely?

    > The attribute supports a priority as well: #[Hook(hook: 'user_cancel', priority: -20)]

    I'm torn between using priority to be consistent with Symfony, and suggesting we use weights to be consistent with Drupal-land.

    > There's little to no concern given to backwards compatibility. New hook implementations fire after the old ones and they are altered and fired completely separately. It is not possible to mix old and new

    That would be doable if we wanted to, though, I think? Have something that discovers procedural hooks, and registers them with the OOP hook handler using the module weight.

    We could even smooth the path to conversion by allowing #[Hook] to be used on a procedural function if we wanted, no?

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    > Do they specifically need to be registered as services?

    Well, yes, we use the event dispatcher to fire them and the dispatcher uses services.

    Note the full Symfony framework will also register every class it finds under a specified directory which is usually just App\ so that system is designed to take it. Surely by now we would if our array dumper/loader couldn't but can't see why not.

    > I'm torn between using priority to be consistent with Symfony, and suggesting we use weights to be consistent with Drupal-land.

    You and me, brother. I have no idea what to do here. This is up to community to decide. Code wise, it's trivial to change: change string replace priority to weight and a - sign in HookCompilerPass because the event dispatcher does use priority but that's an internal detail, really.

    I neither know nor do I want to figure out how to convert procedural functions into something that the event dispatcher can fire. The entire system is designed around hook implementations being methods on objects. But I have a suggestion. Let's pretend we have a dumper which can dump the service definitions written by autowire. Then:

    1. Convert the hook code to the new system
    2. Dump the service definition generated and add it to service yml
    3. Change the procedural call to manually call the converted code
    4. Add a [#HookConverted] attribute to the procedural code. This would be ignored completely by pre-patch Drupals and this patch could change ModuleHandler to skip functions marked so.

    This would allow modules to convert to the new system yet remain compatible with old Drupals without maintaining the same code twice. Ordering would need to be doubled up if this is what the community wants to do. Not my call.

  • Pipeline finished with Failed
    8 months ago
    Total: 243s
    #151203
  • Pipeline finished with Canceled
    8 months ago
    Total: 36s
    #151207
  • 🇬🇧United Kingdom longwave UK

    >> These are automatically registered as autowired services

    > Do they specifically need to be registered as services? They don't need to be used as services, and that's going to add a LOT of bloat to the container.

    If they are registered as private services they don't bloat the container as much; during container build they should be inlined into the event dispatcher definition as that is the only place they are needed, they don't need to be publically accessible by any name.

  • 🇬🇧United Kingdom longwave UK

    > Note the full Symfony framework will also register every class it finds under a specified directory which is usually just App\ so that system is designed to take it. Surely by now we would if our array dumper/loader couldn't but can't see why not.

    Symfony does do this but also services are private by default, and unused private services are then optimised away - so if you don't refer to an automatically discovered class somewhere, it doesn't even exist in the final container. Drupal is still public-by-default and we need to unwind a lot of things to be able to change that.

  • Pipeline finished with Failed
    8 months ago
    Total: 200s
    #151339
  • Pipeline finished with Canceled
    8 months ago
    Total: 88s
    #151354
  • Pipeline finished with Failed
    8 months ago
    Total: 178s
    #151364
  • Pipeline finished with Failed
    8 months ago
    #151377
  • Pipeline finished with Failed
    8 months ago
    Total: 189s
    #151510
  • Pipeline finished with Canceled
    8 months ago
    Total: 780s
    #152862
  • Pipeline finished with Failed
    8 months ago
    Total: 654s
    #153030
  • Pipeline finished with Success
    8 months ago
    Total: 988s
    #153057
  • 🇬🇧United Kingdom joachim

    > A method can have multiple Hook attributes if it implements multiple hooks. For example, node_comment_insert and node_comment_update have the exact same implementation and so they could become

    This is nice!
    But how would the implementation know which hook is being invoked currently?

    We also need to think about how this should affect api.php file documentation. When Hook() attributes become the default way of implementing a hook, an api.php file containing procedural functions won't make much sense.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    But how would the implementation know which hook is being invoked currently?

    They don't. This feature is for code that doesn't care. Please check node_comment_insert and node_comment_update -- they are literally the same code.

  • 🇺🇸United States DamienMcKenna NH, USA

    But how would the implementation know which hook is being invoked currently?

    Isn't it up to the person writing the code to make it aware of what is triggering the code? Or am I missing something?

  • 🇩🇪Germany Fabianx

    I apologize if I am missing something, but:

    #[Hook(hook: 'user_cancel')]

    feels a little bit duplicated (or is that because of alter hooks being different?).

    But I would prefer clearly:

    #[Hook('user_cancel')]
    #[AlterHook('module_implements')]
    
    or
    
    #[AlterHook('module_implements_alter')]
    

    Therefore we get rid of the magic naming, but don't have: Hook(hook: ) duplicated.

    But maybe it is necessary, then please disregard.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada
    1. I am not sure how this works. It's entirely possible you can already drop hook: -- if it just calls the constructor then you already can. I pushed a commit doing this.
    2. We could add an AlterHook attribute extending Hook which adds _alter and ... that's it. So Hook('foo_alter') and AlterHook('foo') is the exact same. Is this acceptable?
    3. Regarding procedural, this is described in the issue summary: I advocate against the complexity of adding a harness to call the old ones from the new system but of course that's just me. But I also wrote it up in #8 how to integrate them seamlessly using a ProceduralCall class and a magic __call method call whatever procedural call it is passed to.
    4. Also I am not seeing the event based hooks take care of module implements alter -- this is explained in the issue summary and an example is provided in the PR. The existing service provider infrastructure is used.
  • Pipeline finished with Success
    8 months ago
    Total: 1020s
    #153597
  • 🇬🇧United Kingdom catch

    You and me, brother. I have no idea what to do here. This is up to community to decide. Code wise, it's trivial to change: change string replace priority to weight and a - sign in HookCompilerPass because the event dispatcher does use priority but that's an internal detail, really.

    There's an old issue at 📌 Add before/after ordering to events Needs work , which with new things, could maybe look like #[Before('whatever')] #[After('whatever')] #[Replaces('whatever') (or an extra param to the hook attribute) then numeric priorities/weights wouldn't need to be interacted with by modules.

    Isn't it up to the person writing the code to make it aware of what is triggering the code? Or am I missing something?

    It's for cases where exactly the same things needs to happen. i.e. sometimes you have identical code running on insert or update hooks, right now you'd put that in a extra method/function somewhere and call it from both, with being able to annotate the same method for multiple hooks, everything is in one place.

    If the implementation needs to be different, then it should two methods with their own hook attributes.

  • 🇬🇧United Kingdom catch

    So Hook('foo_alter') and AlterHook('foo') is the exact same. Is this acceptable? Would look like like

    I think this might have been discussed in one of the other issues (or even implemented in @donquixote's issue). Alter hooks are special so it's worth doing dedicated for it IMO.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    I am all in for adding before / after into the attribute, I see zero challenges here, HookCompilerPass could run the (already documented) min/max as needed, but could we punt this into a followup? My hand is up, I will write it, no problems. I just would like to proceed in minimal slices so we can proceed in parallel after. Likely the conversions made in this issue shouldn't be merged either, they are there to ferret out any problems (and they did).

    Because while I see no technical challenges, the exact syntax likely will take quite a bit of discussion to figure out -- is it before/after a module? a list of modules? a list of class-method pairs? how do we indicate I want to be first/last? what if two does say want that? what if the ordering is not doable? A wants to be after B, B wants to be after C, C wants to be after A, opsie. So there's a bit to discuss on that front.

  • 🇬🇧United Kingdom catch

    I am all in for adding before / after into the attribute, I see zero challenges here, HookCompilerPass could run the (already documented) min/max as needed, but could we punt this into a followup?

    Yes but I think it means we can defer some discussion about weights vs. priorities too.

  • Status changed to Needs work 8 months ago
  • 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.

  • 🇩🇪Germany Fabianx

    One thing I liked about the Hux-style approach was that you could just add a Hook attribute to any service and it was not limited to things in the hook namespace - you just tag the service as a 'hook_listener' and it works.

    BUT this can and should be a follow-up.

    I just want to ensure that this use case CAN still be made to work with the current architecture. (even if we then just later decide for or against it)

  • 🇩🇪Germany Fabianx

    Patch looks great so far, but if it was me:

    - We only allow one hook implementation per module right now (which is fine)

    Why don't we just merge the collected listeners with the procedural when:

    moduleHandler->invokeAll() collects the procedural implementations?

    Then the whole module_implements_alter(), etc. would still work (without any changes or before/after!).

    That would ensure for this FIRST iteration, we get OOP hooks via event listeners and auto-wired services, but moduleHandler really does not change except for discovering these implementations for the hook.

    Before this was not possible due to how we did not have getListeners() so ordering became impossible, but now it's natural.

    And we still throw an Exception() if a hook is defined more than once in the same way.

    I feel - and I might be wrong - this would make for the most minimal patch.

  • 🇫🇷France andypost

    Good idea to figure out what can fit into 10.3.x (deadline - week 29 Apr'24)

    Current state looks good starting point but Experimental API needs more discussion

    PS: looking at NodeNooks.php implantation it reminds me Action plugin, for example \Drupal\user\Plugin\Action\RemoveRoleUser

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    I posted a new version folding procedural hooks in resulting in much simplified ModuleHandler -- and the build time parts are not so bad either. We discussed with catch and just throwing in every function as a hook candidate seems a very reasonable plan as already there are not that many functions which are not hooks.

    A few tests around install and update are not passing yet, the rest are happy.

    The only unrelated change I made is renaming core_field_views_data to views_field_views_data. But since that's a hook implementation I doubt it's a problem. If not desirable, we can revert -- it will work now but when I made the change ModuleHandler::invoke was stricter, requiring $module to be a $module and $hook to be a $hook. Since then I dropped this for more backwards compatibility.

  • Pipeline finished with Failed
    8 months ago
    Total: 549s
    #157274
  • 🇺🇸United States nicxvan

    I'm not sure if this is out of scope, but I don't see any special handling for .theme files.
    I do see the update for where hook_theme() is invoked in registry, but while there is some code to parse in HookHelper for .module, .inc, and .install, I don't see the same for .theme.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    I would punt on themes. While ThemeHandler does add the themes to the PSR-4 namespaces and so it is maybe possible to give themes a similar treatment I would like to proceed in as small chunks as possible to make it easier to get this in. Please note DrupalKernel currently has zero to do with themes and we would need to figure out some sort of theme namespaces during container build which might raise some issues: the patch currently has failures around installation, I am afraid it make even this worse.

  • 🇺🇸United States nicxvan

    Fair enough, I'm ok with that scope, we should be sure to include that carve out in the change record so there is no confusion.

    Another note from the slack conversation between Chx, godotislate and myself is the following from godotislate:

    it looks like they are all invoked after module handler has its turn so keeping them OOS seems fine

    So they are likely not intermingled and can be modified in another issue.

  • 🇺🇸United States nicxvan

    Just gathering the test failures:

    Drupal\Tests\demo_umami\Functional\DemoUmamiProfileTest
    Drupal\Tests\media\Functional\UrlResolverTest
    Drupal\Tests\system\Functional\UpdateSystem\UpdatePathLastRemovedTest
    Drupal\Tests\system\Functional\UpdateSystem\UpdateScriptTest
    Drupal\Tests\standard\Functional\StandardTest
    Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest
    Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest
    
  • Pipeline finished with Canceled
    8 months ago
    Total: 270s
    #157951
  • Pipeline finished with Failed
    8 months ago
    #157962
  • Pipeline finished with Failed
    8 months ago
    Total: 2602s
    #157970
  • 🇺🇸United States nicxvan

    The cache test is passing for me locally, I am going to try rebasing.

  • 🇺🇸United States nicxvan

    It was already up to date, I'm not sure what is going on, the test passes locally, but it is failing upstream.

    I narrowed it down to this assertion in: core/modules/block/tests/src/Functional/BlockCacheTest.php

    // Block content not served from cache.
    $this->drupalLogout();
    $this->drupalGet('user');
    $this->assertSession()->pageTextContains($current_content);

  • 🇦🇺Australia acbramley

    It's great to see more movement on this problem space but this new issue has raised a few questions for me. Quick background: I have been using Hux since it's inception, and am arguably one of its biggest users and supporters seeing as my colleague wrote it :)

    We now have several implementations of Hux like OOP hooks, it would be great to get a consensus on what one we're going forward with and close out the other MRs/issues ( https://www.drupal.org/project/drupal/issues/2237831 🌱 Allow module services to specify hooks Needs work for example)

    I'm also curious what the differences (if any) there are with this solution when compared to Hux. It seems quite similar but I haven't had a chance to review the code yet.

    Thanks for all the great work so far!

  • Pipeline finished with Failed
    8 months ago
    Total: 192s
    #158335
  • Pipeline finished with Failed
    8 months ago
    Total: 573s
    #158352
  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    @acbramley that decision is not mine to make. As far as I can tell, there are two things Hux allows for and the current PR does not and both are easy to add either here if it's blocking or in a followup if desired. I do not yet see the need but I am easy to convince.

    1. hooks in subdirectories -- we already have multiple classes, do we need multiple directories too? I can imagine arranging into say NodeUserHooks.php and NodeTermHooks.php but ... subdirectories? Are we really going to need them? I can write them, sure, we already have a recursive iterator, using another is not a biggie.
    2. hooks as tagged services -- I think the Hook attribute on a class completely replaces this need. If I am wrong, let me know and I will add this.

    Re-reading Hux after writing this was certainly useful because I quickly added the replaces functionality.

    This PR has some advantages over Hux. Apologies if I missed some of its functionality.

    1. Complete subsuming of procedural hooks resulting in a simplification of ModuleHandler by moving some of the code to discovery time.
    2. Support for the rather complex alter ordering for multiple types. I think my support is easier to understand than the current one, even.
    3. Speaking of ordering, an equivalent for module implements alter is supported -- without adding any new mechanism even. Just use service provider alters on the new container parameter.

    For the future, this patch delegates the actual sorting to the event dispatcher allowing for a followup which adds before/after functionality benefiting both events and hooks automatically.

  • Status changed to Needs review 8 months ago
  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    While more tests certainly need to be added this is now green and certainly ready for review.

  • 🇬🇧United Kingdom catch

    @acbramley I've gone ahead and postponed 📌 Hux-style hooks, proof of concept Needs work which has a similar API but ended up with a very complicated bc layer, the discussion on that issue is extremely relevant to here still and we should copy credit over etc. It could also use a proper comparison of the different approaches between these two issues somewhere here, but not going to attempt that on Sunday morning..

    I've also postponed 🌱 Allow module services to specify hooks Needs work which didn't have any recent code but was the original, ten year old discussion, that wanted what we ended up with here but didn't have Symfony 6 and attributes (or the experience of Hux) to make it a reality.

    Between all the various issues, and can't separate my own very strong opinions here, I think there is the following in common between the two active issues that so far seems non-controversial:

    • Allow OOP hooks to be invoked with the current hook invoking APIs, so that conversions to OOP hooks can be done independently from any changes to invocation.
    • Continue to support passing 'arbitrary' arguments to hooks, so that e.g. node load implementations type hint NodeInterface $node and get that, not an Event with a ::getNode() method.
    • #[Hook] attribute possibly with #[HookAlter] variation, and autowiring, so that hooks can still be defined within a single method in a single file (no requirements for separate YAML or ::getSubscribedEvents()).
    • Allow multiple hook attributes on a method (for the insert/update identical logic case).
    • Allow multiple classes for hooks so that they can inject only the services they need.

    Things that are still to be defined somewhat and may end up in follow-ups:

    • Allowing multiple implementations of the same hook in one module vs. not.
    • Hook weights vs. priorities vs. before/after/replaces support, how much bc there is for hook_module_implements_alter(), and what any eventual replacement for hook_module_implements_alter() might look like (can we do it all with before/after/replaces attributes + a compiler pass?)
  • 🇨🇭Switzerland berdir Switzerland

    catch also tagged 📌 Replace hook_cron() with a more modern approach Needs work as related. Being able have multiple hooks in a module would definitely solve one aspect that the cron subscriber issue is trying to solve, another is the ability to have cron subscribers in core components. Would this allow to also basically treat each component as a "module"? That would allow to move a lot of the code that's currently in system.module, especially hooks, to the respective component. Similar to how we already support plugin discovery there, each core component could have a Hooks folder as well.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    Supporting the full container.namespaces instead of module namespaces to include core is certainly easily doable.

  • 🇬🇧United Kingdom joachim

    How will hooks whose names include a variable work?

    E.g. hook_form_FORM_ID_alter, hook_ENTITY_TYPE_update()?

    Could the variable part be a separate value in the attribute?

  • 🇬🇧United Kingdom catch

    @joachim I would think that would work the same as now? - i.e. hook_form_FORM_ID_alter() is always implemented as hook_form_my_form_id_alter() same as hook_ENTITY_TYPE_load() is the same as hook_node_load(). If you want to operate on all entities or all forms, then you go back to hook_form_alter() and hook_entity_load()

  • 🇬🇧United Kingdom joachim

    What I meant is, would we have:

      #[Hook('hook_form_myform_alter')]
    

    or split this out to a parameter:

      #[Hook('hook_form_FORM_ID_alter', form: 'myform')]
    
  • Status changed to Needs work 7 months ago
  • 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.

  • 🇦🇺Australia acbramley

    Big +1 to just having it as it is now (i.e #[Hook('hook_form_myform_alter')], not #[Hook('hook_form_FORM_ID_alter', form: 'myform')] - I don't even know how that'd work...

  • 🇨🇭Switzerland berdir Switzerland

    > #[Hook('hook_form_FORM_ID_alter', form: 'myform')]

    I don't think your proposal makes it any easier to use, it's longer, and you still have to remember the pattern, which tends to be confusing as many forms have a "form" in their form_id (is it form_FORM_ID_alter() or FORM_ID_form_alter()...)

    What we *could* add is something like a #[FormAlter('form_id')] or #[EntityHook(entity_type: 'node', hook: 'presave')], specific attribute classes that then build the specific hook name patterns that they know about. That could easily be a follow-up, the discovery already explicitly supports any Hook subclass (for example the already implemented AlterHook), here can be as many as we want, you could even add your own.

  • 🇬🇧United Kingdom joachim

    > #[Hook('hook_form_FORM_ID_alter', form: 'myform')]
    > #[FormAlter('form_id')]

    I'm not hung up on the specific syntax, the point was splitting out the variable part for the FORM_ID placeholder into a separate value.

    Happy to leave it as a follow-up to keep this issue simple.

  • 🇬🇧United Kingdom catch

    We have hook_form_BASE_FORM_ID_alter() too, so any special handling would need to also handle that.

  • 🇦🇺Australia mstrelan

    We could take that one step further and turn every hook into its own attribute that extends the base hook class. In some ways that would be nice because you could just use #[Install] for hook_install, or #[Cron] for hook_cron, which removes the magic hook name string, but it would also be fairly annoying that every module would need to declare a whole bunch of hook attributes. Although, this could replace mymodule.api.php so maybe it has some merit.

  • 🇬🇧United Kingdom joachim

    Oh yes, we need to rethink MODULE.api.php as part of this issue. Otherwise it's not going to make sense.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    mstrelan , that's a wonderful idea. Custom code could still just use #[Hook] for an undocumented one-off hook if needed for quick-and-dirty -- the ability to keep the zero fuss on either caller is implementation side is of extreme importance to me (indeed that's why I jumped on this). But clean code could just use create a custom attribute class and put the API documentation on the constructor. A somewhat small change to api.drupal.org, hopefully. I presume it would still all extend the Hook attribute just set the hook name to something relevant. We could use a class constant for that, so it would become ->invokeAll(Cron::HOOK) and #[Cron] on the implementation side which would be the exact same as >invokeAll('hook') and #[Hook('cron')]. I really like this idea.

  • 🇬🇧United Kingdom longwave UK

    Re form alters once we've stopped using procedural hooks we could just use the form class name as the form ID, and the ::class constant in hooks, I think? Yo start with this would only be for new forms but we could eventually deprecate form IDs and add some form of BC, maybe.

  • 🇬🇧United Kingdom joachim

    > So if you have a fixed hook then you could just create a attribute class and put the API documentation on the constructor. A somewhat small change to api.drupal.org, hopefully

    Harder to discover by reading code if hook attributes are mixed in with other kinds of attribute in src/Attribute.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    Oh, that's easy to solve, just put them in src/Attribute/Hook.

  • With the possibility of additional/contrib/custom hook classes, it might make sense to add HookInterface and make every relevant attribute class implement that. With the Interface definining some basic setters and getters (setHook(), getHook() etc..

    Also, and I haven't thought through the complexities, I wonder if there are any advantages to making Hook attributes support multiple hooks, such as node_update + node_insert in one hook attribute, or form_node_form + form_media_form in one alter hook attribute?

    So

    #[Hook('node_update')]
    #[Hook('node_insert'])]
    public function doNodeStuff(...) {}
    

    becomes

    #[Hook(['node_update', 'node_insert'])]
    public function doNodeStuff(...) {}
    

    or

    #[AlterHook('form_node_form')]
    #[AlterHook('form_media_form')]
    public function alterNodeOrMediaForm(...) {}
    
    #[AlterHook(['form_node_form', 'form_media_form'])]
    public function alterNodeOrMediaForm(...) {}
    
  • 🇬🇧United Kingdom longwave UK

    #59 feels like a complication we don't need for now, multiple attributes are supported and feel much cleaner to read.

  • 🇬🇧United Kingdom longwave UK

    Tagging for framework manager signoff as this is a significant addition to an existing API (or set of APIs depending on how you look at it)

  • I'm also not sure we need an interface if everything must descend from the base Hook class anyway.

    I don't have any use cases in mind, but the idea for an interface would be to allow some degree of future proofing, in case the hook attribute class properties need amendment at some point.

    It would also allow other hook attribute classes to do some logic in getters and setters instead of directly acting on property values.

    Note that I don't feel strongly about this, but I thought I'd mention it in case.

  • #[FormAlter(FormTestAlterForm::FORMID)]

    This would probably be better as #[FormAlter(FormTestAlterForm::FORM_ID)], but I like the idea of adding FormAlter.

    I'm not sold on removing Hook from the attribute name. I like being able to see at a glance which attributes are for hooks. We could conceivably have other attributes on functions that aren't for hooks, and that would make it harder to see which ones are hooks.

  • Pipeline finished with Failed
    7 months ago
    Total: 239s
    #162027
  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    While I did resurrect ModuleHandlerTest it is an extremely strongly coupled unit test which has assumptions about how the implementation exactly works. In particular, ::testInvokeAll presumes addModule followed by invokeAll loads the module while ::testLoadModule asserts addModule does not load the module. There is no rhyme or reason to presume addModule doesn't load the module and so now it does. The system proposed here can't work otherwise because module_implements_alter needs to be called build time and it would be inconsistent if we only loaded the modules implementing it. Since addModule is a very rare, install time only thing, I see no harm in this change.

  • Pipeline finished with Failed
    7 months ago
    Total: 198s
    #162033
  • Pipeline finished with Failed
    7 months ago
    #162038
  • Pipeline finished with Failed
    7 months ago
    #162054
  • Pipeline finished with Failed
    7 months ago
    #162453
  • Pipeline finished with Failed
    7 months ago
    Total: 511s
    #162476
  • Pipeline finished with Success
    7 months ago
    Total: 542s
    #162488
  • Status changed to Needs review 7 months ago
  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    I just rebased this so it applies.

  • Pipeline finished with Failed
    7 months ago
    Total: 497s
    #162579
  • Pipeline finished with Success
    7 months ago
    Total: 704s
    #163396
  • Pipeline finished with Success
    7 months ago
    Total: 548s
    #163406
  • Pipeline finished with Success
    7 months ago
    Total: 608s
    #164567
  • 🇺🇸United States nicxvan

    I'm wondering if the merge request being in draft is intentional?

    My understanding is that this is ready for a deeper review with the following comments that all need to be addressed.

    1. A framework manager needs to review per 61.
    2. The suggestion to change FormAlter(FormTestAlterForm::FORMID) to FormAlter(FormTestAlterForm::FORM_ID) per 63.
    3. One question in the MR about handling the profile extension that I think may be addressed.
    4. The question in the issue summary about whether additional tests are needed.

    Let me know if I missed anything, I added these to the issue summary too.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    Thanks for the meta review :)

    1. It's in draft because I have no idea what that means. I am just writing patches, here (in two weeks it'll be twenty years). This new workflow is a complete unknown for me.
    2. I talked with catch about a review , likely next week.
    3. I will
    4. Profile was addressed I thought?
    5. That's part of #1 I think

    Edit: sorry I didn't see the review by solideogloria I will process that too.

  • @ghost-of-drupal-past If you click on the Merge Request (and make sure you are signed in to GitLab; just click Log In and it will automatically auth you), there should be an option to mark the MR as "Ready". That will remove the Draft status.

    In addition, there should be a way to mark each of my comments as "resolved", either now or after you have committed a change that addresses a comment.

  • For Drupal core MRs, I don't know that marking them as "draft" really matters much, but often on other projects, reviewers are accustomed to skip reviewing draft MRs/PRs, because it's a signifier of WIP product not ready for review or to merge.

    FYI, if you are the MR author, you can move it out of draft by clicking on the kebab menu in the upper right and selecting "Mark as ready".

  • Yes. Also, only the author can select "Mark as ready", I think, so it would be a good idea to do so.

  • 🇺🇸United States nicxvan

    Thanks guys. As far as the profile question it is resolved, I was on my phone so I couldn't see the changes super accurately.

    collectModuleHookImplementations takes care of it I think so I've removed that.

    I also want to document the .theme discussion from slack here a bit clearer.

    From godotislate:

    you can implement hook_form_alter, hook_page_attachments_alter, among a few others in .theme
    but that is via theme manager, not module handler
    it looks like they are all invoked after module handler has its turn
    so keeping them OOS seems fine

  • keeping them OOS

    What does "OOS" stand for?

  • 🇺🇸United States nicxvan

    It's what godotislate said in slack, I assume it means Out Of Scope.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    Note the FORMID constant was just a discussion in the issue, it's not found in the code yet. The attribution parameter is called form_id.

    I marked the PR as non-draft.

  • Pipeline finished with Failed
    7 months ago
    Total: 184s
    #175478
  • 🇺🇸United States nicxvan

    I fixed the codesniff issues. I think that only runs once the MR is out of draft which is why we didn't catch it before.

  • Pipeline finished with Failed
    7 months ago
    Total: 879s
    #176696
  • 🇺🇸United States nicxvan

    I rebased to try to fix the unrelated failures.

  • Status changed to Needs work 7 months ago
  • The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

  • Pipeline finished with Success
    7 months ago
    Total: 695s
    #176708
  • Status changed to Needs review 7 months ago
  • 🇦🇺Australia acbramley

    naughty bot

  • 🇺🇸United States nicxvan

    Thanks! I was about to say the same lol, tests are passing again.

  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan

    I addressed the two comments on the MR that I could, there are a few comments that @Ghost of Drupal Past should address from @solideogloria.

    Two questions about targeting classes: #[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::IS_REPEATABLE)]
    And the regex here can module names have pipes | in them: $module_preg = '/^function (?(?' . implode('|', $modules) . ')_(?!preprocess_)(?[a-zA-Z0-9_\x80-\xff]+))\(/m';

  • 🇺🇸United States nicxvan
  • Status changed to Needs work 7 months ago
  • The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

  • Status changed to Needs review 7 months ago
  • Pipeline finished with Success
    7 months ago
    Total: 671s
    #176962
  • Assigned to nicxvan
  • 🇺🇸United States nicxvan

    I'm rebasing

  • Issue was unassigned.
  • 🇺🇸United States nicxvan

    Ok I rebased this so it applies again.

    @solideogloria's questions have been responded to on the MR.

    Two remaining tasks are whether more tests are needed and a framework manager review.

  • Pipeline finished with Success
    7 months ago
    Total: 1368s
    #185223
  • 🇺🇸United States nicxvan

    Copying some relevant abridged discussion from slack if others think the class target is worth dropping:

    chx (he/him)
    if we dont want the class target, dropping it is really easy

    chx (he/him)
    10 hours ago
    one could write the relevant part of getHookAttributesInClass like this to show it doesn't leak

    <?
    $class_implementations = array_map(function (\ReflectionAttribute $reflection_attribute) use ($class) {
    $hook = $reflection_attribute->newInstance();
    assert($hook instanceof Hook);
    if (!$hook->method) {
    if (method_exists($class, '__invoke')) {
    $hook->setMethod('__invoke');
    }
    else {
    throw new \LogicException("The Hook attribute for hook $hook->hook on class $class must specify a method.");
    }
    }
    return $hook;
    }, $reflection_class->getAttributes(Hook::class, \ReflectionAttribute::IS_INSTANCEOF));

  • Pipeline finished with Success
    6 months ago
    Total: 724s
    #186223
  • 🇺🇸United States nicxvan

    @chx nor I can reproduce the config validation failures locally so any insight would be appreciated.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    I pushed a small change which shows there's no need to use loadInclude now. I am not 100% on doing this but it'll go away anyways when node admin is converted to a class which seems inevitable.

  • Pipeline finished with Success
    6 months ago
    Total: 727s
    #188482
  • Pipeline finished with Failed
    6 months ago
    Total: 591s
    #208577
  • Status changed to Needs work 6 months ago
  • 🇬🇧United Kingdom longwave UK

    Thanks for the discussion about this issue at Dev Days, I feel it was super productive and myself, @alexpott and @catch all agree that this is a great change and will unlock several further improvements in the future.

    Added some minor feedback but overall the approach is great and I have no architectural concerns.

  • 🇬🇧United Kingdom longwave UK

    We probably also need to be clear that this doesn't apply to preprocess hooks, nor hook_update_N or post_update hooks. Unsure if there are any other special cases on top of these?

    As hook_hook_info() is gone we should remove the docs and any implementations and write a separate change record for it? Or, should the HookCollector still use hook_hook_info() to learn what files to scan?

  • 🇦🇺Australia acbramley

    Re #94 - I think (at least this is a limitation of Hux) that no themeing hooks work at all, including hook_theme. Would be good to confirm that too.

  • Pipeline finished with Success
    6 months ago
    Total: 536s
    #210427
  • Status changed to Needs review 6 months ago
  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    Should the HookCollector still use hook_hook_info() to learn what files to scan? No, it shouldn't, it's not adequate, it's possible for modules/includes to include further files and those are hidden from us unless we the full scanning done here. We got our framework review, we have quite a few new things now from DevDaysBurgas, most importantly moving to a compiler pass and the introduction of LegacyHook so I think needs review is the best state right now. I will add a LegacyHook test ASAP.

  • Pipeline finished with Failed
    5 months ago
    Total: 186s
    #211146
  • Pipeline finished with Failed
    5 months ago
    Total: 198s
    #211174
  • Pipeline finished with Failed
    5 months ago
    Total: 177s
    #211215
  • Pipeline finished with Failed
    5 months ago
    Total: 187s
    #211222
  • Pipeline finished with Failed
    5 months ago
    Total: 607s
    #211226
  • Pipeline finished with Failed
    5 months ago
    #211237
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Adding back tag, still on our radar

  • Pipeline finished with Success
    5 months ago
    Total: 702s
    #211928
  • Pipeline finished with Success
    5 months ago
    Total: 485s
    #212515
  • Status changed to Needs work 5 months ago
  • 🇬🇧United Kingdom longwave UK

    There are a few merge conflicts and some missing docblocks but otherwise to me this is ready to go - still needs framework manager signoff though.

  • First commit to issue fork.
  • 🇳🇿New Zealand quietone

    Only did a rebase and some docs updates because of request in Slack. I did notice there are more methods that need documentation, that declare strict types hasn't been added and that not all method arguments have type hints. I can't help with that now.

  • First commit to issue fork.
  • 🇮🇳India bhanu951

    Added strict_types on new files and rebased to head.

  • Pipeline finished with Failed
    5 months ago
    Total: 559s
    #234610
  • 🇺🇸United States nicxvan

    @Bhanu951 thank you for your contribution!

    In the future I think rebasing is preferred over merging for bringing upstream changes into an MR.

    Here is the documentation I usually follow: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... .

    I've taken care of updating this MR with the most recent changes already so no need to rebase at the moment.

  • Pipeline finished with Failed
    4 months ago
    Total: 116s
    #238733
  • Pipeline finished with Failed
    4 months ago
    Total: 6662s
    #238751
  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    With 📌 Handling update path divergence between 11.x and 10.x Fixed committed this issue reached a breaking point.

    If no other help is forthcoming I will won't fix this in a week.

  • 🇬🇧United Kingdom catch

    Two things on the recent test failures:

    1. I didn't think we were trying to support hooks in .install files (either in this issue, or even in a follow-up), so I am a bit confused why they are causing test failures, is it because .install files are using the new discovery even though oop hooks aren't supported?

    2. The conditionally defined functions in update test modules are an extreme edge case and abuse of the update API, and I don't think we need the bc layer to handle them. Either we can use the old discovery for .install hooks, or find a different way to test the upgrade path.

    I think it might be easier to go back to the old discovery for update (.install/post_update.php hooks, and only use the tokenizer temporarily), but there might be reasons to not do that I haven't thought of at all.

  • Pipeline finished with Failed
    4 months ago
    Total: 659s
    #238848
  • Pipeline finished with Failed
    4 months ago
    Total: 565s
    #238856
  • Pipeline finished with Failed
    4 months ago
    Total: 118s
    #238891
  • Pipeline finished with Failed
    4 months ago
    Total: 515s
    #238894
  • Pipeline finished with Failed
    4 months ago
    Total: 159s
    #239381
  • Pipeline finished with Failed
    4 months ago
    Total: 464s
    #239398
  • Pipeline finished with Success
    4 months ago
    Total: 536s
    #239407
  • Pipeline finished with Success
    4 months ago
    Total: 560s
    #243049
  • Status changed to Needs review 4 months ago
  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    All MR comments except https://git.drupalcode.org/project/drupal/-/merge_requests/7604#note_332589 are resolved and I am not keen on resolving that one for reasons outlined there. catch already granted a BC break exception to this issue (conditionally defined hooks are not supported), hopefully longwave will grant another.

    However, new first/last/before/after functionality just landed. I hope both the documentation and testing will be found adequate.

    For both, I think needs review is the correct status at this point.

  • Pipeline finished with Success
    4 months ago
    Total: 720s
    #243075
  • Pipeline finished with Success
    4 months ago
    Total: 599s
    #243673
  • Pipeline finished with Failed
    4 months ago
    Total: 167s
    #244895
  • Pipeline finished with Failed
    4 months ago
    Total: 150s
    #246157
  • 🇬🇧United Kingdom longwave UK

    Given that the static helper is really just syntactic sugar, we could make it even cleaner by moving the before/after/first/last directly to the attribute?

    So

    class CkEditor5Hooks {
      #[FormAlter('filter_format_form')]
      public function formFilterFormatFormAlter() {}
    }
    
    class CkEditor5ServiceProvider extends ServiceProviderBase {
      public function alter(ContainerBuilder $container) {
        HookHelper::after($container, 'form_filter_format_form_alter', CkEditor5Hooks::class . '::formFilterFormatFormAlter', MediaHooks::class .  '::formFilterFormatFormAlter', EditorHooks::class . '::formFilterFormatFormAlter');
      }
    }
    

    becomes

    class CkEditor5Hooks {
      #[FormAlter('filter_format_form'), after: [
        MediaHooks::class .  '::formFilterFormatFormAlter',
        EditorHooks::class . '::formFilterFormatFormAlter',
      ]]
      public function formFilterFormatFormAlter() {}
    }
    

    or for e.g. last

    class CkEditor5Hooks {
      #[FormAlter('filter_format_form'), last: TRUE)]
      public function formFilterFormatFormAlter() {}
    }
    

    This doesn't solve the really awkward/dynamic cases which would still need the service provider, but the 90% use case is surely likely to be before/after/first/last and we can make it much easier to understand this way.

  • 🇬🇧United Kingdom longwave UK

    Separate attributes could also work, but feel a bit more unintuitive:

    class CkEditor5Hooks {
      #[FormAlter('filter_format_form')]
      #[RunAfter([
        MediaHooks::class .  '::formFilterFormatFormAlter',
        EditorHooks::class . '::formFilterFormatFormAlter',
      ])]
      public function formFilterFormatFormAlter() {}
    }
    
  • 🇺🇸United States nicxvan

    Personally I think this feels cleaner

    class CkEditor5Hooks {
      #[FormAlter('filter_format_form', after: [
        MediaHooks::class .  '::formFilterFormatFormAlter',
        EditorHooks::class . '::formFilterFormatFormAlter',
      ])]
      public function formFilterFormatFormAlter() {}
    }
    

    I'm not sure I like:

    class CkEditor5Hooks {
      #[FormAlter('filter_format_form')]
      #[RunAfter([
        MediaHooks::class .  '::formFilterFormatFormAlter',
        EditorHooks::class . '::formFilterFormatFormAlter',
      ])]
      public function formFilterFormatFormAlter() {}
    }
    

    I have not used attributes much beyond custom Drush commands so this isn't from a deep perspective, just from reading the comments the former feels clearer to me while the latter feels more disconnected as a separate attribute.

  • 🇦🇺Australia mstrelan

    FWIW these before and after additions are essentially making hook implementations public API, so if we wanted to move MediaHooks::formFilterFormatFormAlter to another class, or rename it, we'd need to think about how to deprecate it.

  • 🇦🇺Australia acbramley

    @mstrelan that's a great point. Do we need to implement this stuff in the MVP of OOP hooks? Might be quite a bit of bike shedding for something that probably isn't even used in 90% of sites. I've heavily used Hux since its inception on some very complex sites and never needed before, after, or even reordering. I understand it's to match feature parity with module_implements_alter but do we have to have 100% feature parity for the first iteration?

  • 🇬🇧United Kingdom longwave UK

    OK, maybe #111 is a step too far for this first issue. Although I'm not sure how this is really making it API any more than it would be inside a service provider, it's the same end result with a different syntax.

  • 🇦🇺Australia mstrelan

    I wasn't specifically talking about attributes, just the concept of before and after in general.

  • 🇺🇦Ukraine pingwin4eg Zaporizhia 🇺🇦

    Is it safe to use class names like that in recent comments?

    What if an extension declaring the hook is not dependent on those other extensions and classes may be missing?

    Is it possible to use just extension names instead?

  • 🇬🇧United Kingdom longwave UK

    The ::class constant doesn't invoke the autoloader and it works even if the class doesn't exist.

    php > print \Non\Existent\Code::class . '::someMethod';
    Non\Existent\Code::someMethod
    

    With OOP hooks an extension can implement the same hook more than once if they want to, so the extension name isn't enough on its own.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    It also works with use https://3v4l.org/QBPtj

    Moving it into the attribute instead of a service provider is an easy followup.

    If we want to discuss further, moving HookHelper into a followup is a possibility too. It is not required at all, it's completely independent. Yes, using class names and methods is unfortunate but there's just no way around it as noted. But I imagine we could write another helper which gathers the implementation of a hook by a given module. Such a thing wouldn't even need to iterate the listeners, the hook_implementations_map parameter has the info necessary, it's not a lot of code. Most of the time the returned array would have 0 or 1 elements. I consider it a followup because of syntax, possible integration with before/after etc.

  • Status changed to Needs work 3 months ago
  • 🇬🇧United Kingdom joachim
    #[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::IS_REPEATABLE)]
    class FormAlter extends Hook {
    

    There is a core standard that attribute classes go in an Attribute namespace.

  • Status changed to Needs review 3 months ago
  • Pipeline finished with Failed
    3 months ago
    Total: 245s
    #270740
  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    I think joachim asked Hook and FormAlter to live in Drupal\Core\Hook\Annotation instead of Drupal\Core\Hook and that seemed like a reasonable request -- every core attribute I can find is in an Annotation namespace indeed. I moved over and rebased on 11.x, The change record didn't have the FQCN, now it does.

  • Pipeline finished with Failed
    3 months ago
    Total: 331s
    #270782
  • 🇬🇧United Kingdom joachim

    Yup, that was it -- sorry for the unclear snippet. When we started adding attributes to core, we came up with a policy on where to put them -- 📌 [policy, no patch] Decide on a namespace for PHP Attribute classes Fixed .

  • 🇬🇧United Kingdom joachim

    > Oh yes, we need to rethink MODULE.api.php as part of this issue. Otherwise it's not going to make sense.

    This appears to have been forgotten about.

    We also need to figure out how to document the hook classes -- see my comments on the MR.

    It would be great to get this in for 11.1, but it's frustrating when work on docs and standards is neglected, and then the fact that doing it would hold up an issue getting fixed is used as a reason to handwave it out of the way.

  • Assigned to nicxvan
  • 🇺🇸United States nicxvan

    @joachim, do you have any specifics on what you think needs to change about MODULE.api.php?

    Assigning to myself to take care of rebasing and addressing a lot of the comment feedback since it's pretty straightforward.

  • Status changed to Needs work 3 months ago
  • 🇺🇸United States nicxvan

    Rebasing took care of a bunch of failures, there is still one job failing.

    I replied to a few comments for clarification and took care of all of the simple documentation comments tense etc.

  • Issue was unassigned.
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan

    The failures seem to be unrelated, each run they are different tests.

  • 🇺🇸United States nicxvan

    I reran the test and it passed.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    I am done.

  • Assigned to nicxvan
  • 🇺🇸United States nicxvan

    I'm taking another pass.

  • Pipeline finished with Success
    3 months ago
    Total: 481s
    #271792
  • 🇺🇸United States nicxvan

    There is a comment mentioning that install hooks are not included. I am not sure what the exact makeup of install hooks are.

    Initially I thought it was the following:

    • hook_install()
    • hook_install_tasks()
    • hook_install_tasks_alter()
    • hook_uninstall()
    • hook_schema()
    • hook_update_N()
    • hook_requirements()

    There is a comment in the IS that hook_requirements may not be included in the list.

    Also there are a bunch more hooks in module.api.php. If someone can give insight into which hooks explicitly are excluded I can update docs.

    I also identified the theme hooks that are not included in the CR.
    The CR will need to be updated once we identify the excluded hooks.

    As I was writing this @joachim found this: https://github.com/drupal-code-builder/drupal-code-builder/blob/b0fc30b5...

    I'll update the cr and docs based on this list.

  • 🇬🇧United Kingdom joachim

    > @joachim, do you have any specifics on what you think needs to change about MODULE.api.php?

    Well, EVERYTHING.

    Because at the moment, those functions can be discovered from the 'Implements' docblocks. And also because documenting OO hooks as functions is going to look silly.

    So what we need is a pathway from this code:

      #[Hook('user_cancel')]
      public function userCancelBlockUnpublish($edit, UserInterface $account, $method): void {
    

    to the hook_user_cancel() function.

    The simplest thing is to put in the the 'Implements hook_user_cancel()' line.

  • 🇺🇸United States nicxvan

    Here is the list of hooks I added documentation that will remain procedural.
    * - hook_install()
    * - hook_uninstall()
    * - hook_schema()
    * - hook_update_N()
    * - hook_update_last_removed()
    * - hook_requirements()

    Let me know if there are others such as hook_requirements_alter, hook_install_tasks, or hook_install_tasks_alter

  • Issue was unassigned.
  • 🇺🇸United States nicxvan

    I've added some remaining tasks, three are documentation and one is from a comment on this thread.

  • 🇫🇷France andypost

    @nicxvan what about hook_post_update_NAME?

  • 🇺🇸United States nicxvan

    @andypost, I don't know. I think the main question is what are the criteria for what an install hook is.

    I think the true exclusion is hooks not processed by moduleHandler.

    Those seem to be theme and install hooks.

    If it includes all hooks in module.api.php then we're missing quite a few, but I don't have the answer.

    I honestly don't understand why there is a distinction, but I suspect it's the following:

    Included

    core/lib/Drupal/Core/Extension/ModuleHandler.php

    Not included

    core/lib/Drupal/Core/Extension/ModuleInstaller.php
    core/lib/Drupal/Core/Theme/Registry.php

  • 🇺🇸United States nicxvan

    Further investigation, I think anytime $this->invoke or $this->invokeAll is called in module installer would be excluded:

    $this->invokeAll('module_preinstall', [$module, $sync_status]);
    $this->invoke($module, 'update_last_removed')
    $this->invoke($module, 'install', [$sync_status])
    $this->invokeAll('modules_installed', [$modules_installed, $sync_status]);
    $this->invokeAll('module_preuninstall', [$module, $sync_status]);
    $this->invoke($module, 'uninstall', [$sync_status]);
    $this->invokeAll('modules_uninstalled', [$module_list, $sync_status]);
    $this->invokeAll('cache_flush');
    // Called from installSchema
    $tables = $this->invoke($module, 'schema') ?? [];
    // Called from uninstallSchema
    $tables = $this->invoke($module, 'schema') ?? [];

    invokeAll just loops over hooks:
    foreach ($this->moduleHandler->getModuleList() as $module => $extension) {
    $this->invoke($module, $hook, $args);
    }

    Reading this list it looks like the new final list is:

    • module_preinstall
    • update_last_removed
    • install
    • modules_installed
    • module_preuninstall
    • uninstall
    • modules_uninstalled
    • schema
  • 🇺🇸United States nicxvan

    Question is do the alter versions of the listed hooks get excluded too?

    E.g. we exclude hook_install, do we exclude hook_install_tasks and hook_install_tasks_alter. I don't think we do.

  • 🇬🇧United Kingdom joachim

    > @andypost, I don't know. I think the main question is what are the criteria for what an install hook is.

    I assume it's because install hooks are invoked before the container can discover class-based hooks.

    But that needs documenting.

  • Pipeline finished with Canceled
    3 months ago
    Total: 621s
    #271864
  • Assigned to nicxvan
  • 🇺🇸United States nicxvan
  • Pipeline finished with Failed
    3 months ago
    Total: 135s
    #271879
  • Issue was unassigned.
  • 🇺🇸United States nicxvan

    @catch mentions this above:

    1. I didn't think we were trying to support hooks in .install files (either in this issue, or even in a follow-up), so I am a bit confused why they are causing test failures, is it because .install files are using the new discovery even though oop hooks aren't supported?

    I think I've taken care of most documentation other than the why install hooks are not included. I'll wait for someone else to weigh in before doing any more.

  • Pipeline finished with Failed
    3 months ago
    Total: 493s
    #271883
  • Pipeline finished with Success
    3 months ago
    Total: 428s
    #271917
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan

    After digging further due to some questions in slack from @catch.

    hook_update_N is part of the exclusion since it's called from the installer by the UpdateHookRegistry.

    I don't think hook_post_update_NAME need to be excluded since this is part of the documentation:
    * These updates are executed after all hook_update_N() implementations. At this
    * stage Drupal is already fully repaired so you can use any API as you wish.

    I've updated the comments for hook_update_N

  • 🇺🇸United States nicxvan
  • 🇬🇧United Kingdom joachim

    Will API module work with hook methods in classes?

    If it doesn't, moving node hooks to a class will break api.d.o.

  • Pipeline finished with Success
    3 months ago
    Total: 649s
    #271940
  • 🇺🇸United States nicxvan

    Ok after discussion in Slack @ghost of drupal past clarified exclusions.

    The following hooks will likely will never be OOP, there are significant barriers to converting.
    hook_install
    hook_uninstall
    hook_update_N

    The following hooks can be split into a procedural part and an OOP part, but is OOS for this issue:
    hook_requirements

    The following hooks are invoked by the module installer and will require a follow up to determine if they can be replaced.
    In preliminary testing significant numbers of tests break when these are converted so they are most likely not able to be converted either.
    hook_module_preinstall()
    hook_module_preuninstall()
    hook_modules_installed()
    hook_modules_uninstalled()
    hook_schema()
    hook_update_last_removed()
    hook_post_update_NAME()

    Theme hooks are not affected by this MR.
    hook_theme()
    hook_theme_suggestion_HOOK()
    hook_preprocess_hook()
    hook_process_hook()
    hook_theme_suggestions_HOOK_alter()

    I will add post updates to the exclusion list.

  • 🇺🇸United States nicxvan
  • Pipeline finished with Canceled
    3 months ago
    Total: 311s
    #271968
  • Pipeline finished with Canceled
    3 months ago
    Total: 457s
    #271972
  • 🇺🇸United States nicxvan

    Until hooks actually start getting replaced module.api.php does not need to be changed I think.

    This just enables the replacement. Once hooks are replaced, the documention will be removed from module.api.php and the documentation will reside in the respective modules new hook attribute.

    I am also testing: $theme_registry = $this->themeRegistry->getRuntime();

  • Pipeline finished with Failed
    3 months ago
    Total: 485s
    #271975
  • 🇺🇸United States nicxvan

    Ok I did a quick test and reverted the change:
    $theme_registry = $this->themeRegistry->getRuntime(); to put it after where ti was before.

    That causes several exceptions in kernal tests from this line:

    // If called before all modules are loaded, we do not necessarily have a
    // full theme registry to work with, and therefore cannot process the theme
    // request properly. See also \Drupal\Core\Theme\Registry::get().
    if (!$this->moduleHandler->isLoaded() && !defined('MAINTENANCE_MODE')) {
    throw new \Exception('The theme implementations may not be rendered until all modules are loaded.');
    }

    It's not clear why running $theme_registry = $this->themeRegistry->getRuntime(); before the exception works but after does not. It requires some investigation.

  • Pipeline finished with Success
    3 months ago
    Total: 1082s
    #271994
  • Status changed to Needs review 3 months ago
  • 🇺🇸United States nicxvan

    Tests are passing and all comments have been addressed, I think this is ready for review again.

  • 🇬🇧United Kingdom joachim
  • 🇬🇧United Kingdom catch

    @nicxvan thanks for translating my somewhat cryptic questions from slack on my phone into actual docs and answers here.

    Re #149 I think the 'install hooks' part of this we should never attempt to do a 1-1 conversion to OOP hooks and instead defer that to issues like #3167625: Deprecate/replace hook_update_N() in favor of an object-oriented approach similar to Laravel migrations , 🌱 [meta] Replace hook_schema() implementations with ::ensureTableExists() Active , 📌 Move the on-demand-table creation into the database API Needs work etc. that will eventually replace those hooks with something else entirely. hook_update_N() and post updates we don't only have the issue of them being in the .install or post_update.php file but that the entire API relies on procedural function names (getting the schema number from post updates, storing all the post update function names in key/value). I guess we could add some kind of meta for that even if only as something to point to in case someone attempts to do the 1-1 conversion, like '[meta] Replace all remaining procedural-only hooks with something else' - and that can include allowing OOP implementations if/when we find out they're fine.

    Theme hooks like preprocess, I think we should add a direct follow-up for this issue to try to allow OOP implementations, but that's going to be a whole complex task in itself so too much to try to do here.

  • Status changed to Needs work 3 months ago
  • 🇺🇸United States nicxvan

    Ghost of drupal past did some additional investigation into why the exception was being thrown when $this->themeRegistry->getRuntime(); was being called later.

    I pushed his work from the test issue here: https://git.drupalcode.org/project/drupal/-/merge_requests/7604/diffs?co...

    It does the following:

    a) reverts ThemeManager to 11.x
    b) removes the loadAll() from invoke() (yay!)
    c) fixes the failing tests and any weird code which would call ThemeManager()->render() after a rebuildContainer() call while not in install.

    It seems that Drupal was handling this situation differently in tests vs production:
    It sets this: $this->setInstallProfile('standard');
    Which does this: $this->container->get('kernel')->rebuildContainer();
    Which resets the loadAll.

    There is a follow up issue that can be created:

    Investigate moving loadAll from preHandle and (new) rebuildContainer into initializeContainer (all this on DrupalKernel). This is somewhat tricky because there are a lot of calls to preHandle(). But initializeContainer() is called from boot() which is always (?) called before preHandle(). Given preHandle() uses $this->container I would say it is indeed mandatory to call boot() before it. So the issue would be to 1) add a note to preHandle() doxygen to call boot() before it 2) add a guard into preHandle , there's a $this->booted flag 3) move the loadAll() calls , both , into intializeContainer()

  • Status changed to Needs review 3 months ago
  • 🇺🇸United States nicxvan

    @catch, I agree with your assessment.

    Setting needs review, I'll bump it back to needs work if tests don't pass.

  • Pipeline finished with Failed
    3 months ago
    Total: 473s
    #272515
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan

    I think the Hook attribute docs should look more like this:
    I think the test hook section can be removed, but included it for review.

    /**
     * Defines a Hook attribute object.
     *
     * This allows defining a hook implementation using attributes.
     * Hook implementations needs to be marked with a Hook attribute.
     * Either on a method #[Hook('user_cancel')] or on the class
     * #[Hook('user_cancel', method: 'UserCancel')].
     * If the class Hook doesn't specify a method, __invoke is the default method.
     *
     * The attribute supports a priority as well: #[Hook('user_cancel', priority: -20)].
     * The priority defaults to such values as to keep module order.
     *
     * Reordering hook implementation is done by manipulating the kernel listeners
     * in service alter providers.
     * A HookHelper providing setBefore/setAfter/setFirst/setLast/setPriority
     * functionality is included.
     *
     * Old style procedural calls are integrated into the new system.
     * See LegacyHook for additional information.
     *
     * These are automatically registered as autowired services with the
     * class name as the service id.
     * If autowire doesn't suffice -- should be very rare -- they can be
     * registered manually as well, the service id is the class name.
     *
     * The edge case of "implementing a hook on behalf of another module"
     * is also supported by specifying the module in the attribute.
     * It defaults to the defining module.
     *
     * Multiple implementations are allowed on multiple axis:
     * one method can implement multiple hooks by adding a Hook attribute for each.
     * One module can implement a hook as many times as it wants in as many
     * classes as it wants.
     * This allows, for example, adding form_alter implementations firing on
     * other conditions than form id without modifying any existing implementations.
     *
     * Exceptions:
     * Hooks fired via ModuleHandler::invoke
     * - library_build_info
     * - mail
     * - help
     *
     * Help expects a string return, it is not clear what multiple implementations
     * would mean in this case.
     *
     * Also, ModuleHandler::invoke is used as a replacement for a failure-tolerant
     * $function() call, it is not at all clear what multiple implementations
     * would mean.
     * - mass_update
     * - node_update_index
     * - field_views_data
     *
     * Test hooks:
     * - test_hook
     * - test1
     * - test2
     * - views_data
     * - views_data_alter
     * - views_query_substitutions
     * - views_form_substitutions
     * - views_analyze
     * - views_pre_view
     * - views_pre_build
     * - views_post_build
     * - views_pre_execute
     * - views_post_execute
     * - views_pre_render
     * - views_post_render
     * - views_query_alter
     * - views_invalidate_cache
     * - hook
     */
    
  • 🇺🇸United States nicxvan

    Ok, got a review in slack from @catch for the comment, see the MR for the final.

    We cut the section about:

    ModuleHandler::invoke is sometimes used as a replacement for a failure-tolerant $function() call.
    

    This should be addressed in a follow-up.

    The test hooks have been removed from the comment.

  • Status changed to Needs work 3 months ago
  • 🇬🇧United Kingdom joachim
    Defines a Hook attribute object.
     *
     * This allows defining a hook implementation using attributes.
    

    Merge these into:

    > Hook attribute for defining a class method hook implementation.

    > Either on a method ...

    Would be clearer as a bullet list of the 3 techniques.

    > * is supported by specifying the module in the attribute.

    A code example would be good.

    > * It defaults to the defining module.

    I think we can drop this.

    > * See LegacyHook for additional information.

    Classes in docs must be FQCN to make links. (Others too)

    > * Old style procedural calls are integrated into the new system.

    Don't say 'new'. Docs should not be time-based.

    > * Multiple implementations are allowed on multiple axis.

    I have no idea what this means. (and 'axes' plural)

    > * Hooks that must remain procedural:

    'must' and 'remain' -- wrong words to use here. Docs are not about 'doing' or 'changing', they are about what IS.

    > * Exceptions to multiple implementations.

    Needs a colon. Probably a heading, and nested lists. In general this needs a clean-up.

  • Pipeline finished with Success
    3 months ago
    Total: 783s
    #272572
  • 🇬🇧United Kingdom joachim

    Which order does priority go in?

  • 🇺🇸United States nicxvan

    It follows Symfony's priority since it's using the event dispatcher I think from comment 8 https://www.drupal.org/project/drupal/issues/3442009#comment-15560211 📌 OOP hooks using attributes and event dispatcher Needs review .

    So it's the opposite of weight.

  • 🇺🇸United States nicxvan

    Accidental status change.

  • 🇬🇧United Kingdom joachim

    What happens if you try to implement hook_install or hook_theme with a class method?
    We should probably throw an exception for DX.

  • 🇬🇧United Kingdom joachim

    Rather than brute force finding ALL php files in collectModuleHookImplementations(), I think we should have kept hook_hook_info(), as that specifically tells us which PHP files to load.

  • Pipeline finished with Failed
    3 months ago
    Total: 481s
    #272648
  • 🇬🇧United Kingdom catch

    Rather than brute force finding ALL php files in collectModuleHookImplementations(), I think we should have kept hook_hook_info(), as that specifically tells us which PHP files to load.

    Don't think that would work.

    The discovery uses StaticReflectionParser, which can't handle situations like a .module that does manual require_once of a file with a set of hooks in it (which used to be a very common pattern ten years ago, probably not so much now but who knows). I don't like having to look for procedural hooks everywhere either, but it's a bc layer we can eventually remove, so can live with it.

    What happens if you try to implement hook_install or hook_theme with a class method?
    We should probably throw an exception for DX.

    This might be possible with a deny list in hook discovery?

  • 🇬🇧United Kingdom joachim

    I've added support for Module Builder for generating attribute-based hooks (with legacy hook implementations too).

    To try it out, use the 4.0.x branch of Module Builder, and branch `feature-oo-hooks` of the drupal-code-builder/drupal-code-builder package.

  • 🇺🇸United States nicxvan

    I think the deny list might work for the install hooks:

        hook_install()
        hook_module_preinstall()
        hook_module_preuninstall()
        hook_modules_installed()
        hook_modules_uninstalled()
        hook_post_update_NAME()
        hook_schema()
        hook_uninstall()
        hook_update_last_removed()
        hook_update_N()
    

    It's more complicated for theme hooks since themes can define their own hooks, but the standard ones are:
    hook_theme()
    hook_theme_suggestion_HOOK()
    hook_preprocess_hook()
    hook_process_hook()
    hook_theme_suggestions_HOOK_alter()

  • Status changed to Needs review 3 months ago
  • 🇬🇧United Kingdom joachim

    > It's more complicated for theme hooks since themes can define their own hooks, but the standard ones are:

    I don't think that complicates things, since themes can't define attribute hooks anyway.

    The deny list would be for the attribute hook discovery to throw an exception if it finds a Hook('procedural_only_hook') attribute.

  • Status changed to Needs work 3 months ago
  • 🇬🇧United Kingdom joachim

    (oops, accidental status change)

  • Pipeline finished with Success
    3 months ago
    Total: 459s
    #272800
  • 🇺🇸United States nicxvan

    I edited the list of theme hooks. hook_theme is a module hook and is covered by this change, I updated the comment. in the MR

  • Assigned to nicxvan
  • 🇺🇸United States nicxvan

    I am going to attempt the deny list.

  • 🇺🇸United States nicxvan

    Ok I got the full list of status for hooks that need to remain procedural and need to remain procedural for this issue but could possibly be updated in a future update. I'm going to update the exception now too:

    I'm going to do a direct comparison and regex for the update ones.
    I will also update the docs and the CR

    Cannot be OOP because the module namespace is not in the namespaces yet:
    install
    module_preinstall
    schema

    Excluded for this issue but may be OOP in a followup:
    module_preuninstall
    modules_installed
    modules_uninstalled
    requirements (note this can be split into install requirements which must be procedural and runtime requirements which can be OOP)
    uninstall
    Would take significant effort to convert:
    update_last_removed
    post_update_NAME
    update_N
    Likely simple followup for theme hooks
    preprocess_hook (needs to be functional for now, but an easy followup: ModuleHandler needs to expose "hook exists" functionality to replace function_exists($prefix . '_preprocess') in Registry, not a biggie, the first line of invoke() is it)
    process_hook (see preprocess, also _hook here is a "theme hook" which is not at all the same thing as a hook, see sec_theme_hooks in theme.api.php. Suggestion: use preprocess_HOOK and process_HOOK in this list to indicate it's a theme hook)
    theme_suggestions_HOOK ($suggestions = $this->moduleHandler->invokeAll('theme_suggestions_' . $base_theme_hook, [$variables]); it's an ordinary hook made from the prefix theme_suggestions_ and a theme HOOK just to make sure everyone is confused)
    theme_suggestions_HOOK_alter (that's an ordinary alter once again cooked from fixed strings and a theme HOOK)

  • Pipeline finished with Failed
    3 months ago
    Total: 182s
    #272989
  • 🇺🇸United States nicxvan

    I added the deny list and a refactor of invoke that was suggested by Ghost.

    I paused writing the test because locally I'm getting a white screen with this fork so I'm going to investigate that for a bit.

  • Pipeline finished with Success
    3 months ago
    Total: 693s
    #272992
  • 🇺🇸United States nicxvan

    I think there might be an issue in the following code:

    if (!class_exists($class)) {
          return [];
        }
        $reflection_class = new \ReflectionClass($class);
        $class_implementations = [];
        foreach ($reflection_class->getAttributes(Hook::class, \ReflectionAttribute::IS_INSTANCEOF) as $reflection_attribute) {
          $hook = $reflection_attribute->newInstance();
    

    I can't seem to figure out how to get the code in the foreach to run.

  • 🇬🇧United Kingdom joachim

    The @defgroup hooks section in core.api.php is going to need a major rewrite too.

  • 🇺🇸United States nicxvan

    Ghost may have figured out how to make install hooks OOP, here is a poc for the eventual followup: https://git.drupalcode.org/project/drupal/-/merge_requests/7620/diffs?co...

  • 🇺🇸United States nicxvan

    @joachim, for now that probably just needs a reference to the hooks attribute docs, that won't have to be fully rewritten until procedural hooks are deprecated / removed.

  • 🇬🇧United Kingdom joachim

    > for now that probably just needs a reference to the hooks attribute doc

    If we want people to implement OO hooks, then it needs to be rewritten to say that's the new official way of doing it, and procedural hooks are for a small subset of hooks and OO.

  • 🇫🇷France andypost

    Could use to followup to upgrade status bot to notify and convince a rector rule to apply to existing code for contrib at least

  • 🇺🇸United States nicxvan

    @andypost yep, and the rector rule is already written!

  • 🇺🇸United States nicxvan

    Got a fix for the the exclusion and a test I'll be pushing up shortly.

    I'll take a quick pass at those docs too after that.

  • Pipeline finished with Failed
    3 months ago
    Total: 159s
    #273741
  • Pipeline finished with Failed
    3 months ago
    Total: 166s
    #273761
  • Status changed to Needs review 3 months ago
  • 🇺🇸United States nicxvan

    Ok I've added the deny list and took a pass at the core docs implementation section.

    The previous run of the tests passed and this is just a documentation change so I'm setting needs review, but I'll keep an eye on it.

  • Pipeline finished with Success
    3 months ago
    Total: 532s
    #273797
  • Issue was unassigned.
  • 🇺🇸United States nicxvan
  • Pipeline finished with Success
    3 months ago
    Total: 672s
    #273887
  • Pipeline finished with Success
    3 months ago
    Total: 522s
    #273986
  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    While I refuse to work on the issue noe, I do want to note I very much dislike setFirst/setLast/setBefore/setAfter. It was fine the way it was, without set. setPriority makes sense but setFirst doesn't: you are setting the priority of something but you are not setting the first of something, makes absolutely no sense. setAsFirst perhaps but really, no need, first tells the story.

    Perhaps change the classname instead, HookHelper is more of a development artefact than anything else. HookOrder for example.

  • Assigned to nicxvan
  • Status changed to Needs work 3 months ago
  • 🇺🇸United States nicxvan

    I'm happy to make the change once there is consensus.

  • Issue was unassigned.
  • Status changed to Needs review 3 months ago
  • 🇺🇸United States nicxvan

    This issue really wants to sneak in status and assignment changes...

  • 🇦🇺Australia acbramley

    Echoing what I said back in #37 this is awesome to see this issue get so close, I really hope we can get to a state with the documentation that we can move some things to follow ups (that could block next minor) and get this thing in. Kudos to @nicxvan for the incredible effort recently!

    I would love to test this on some of our projects that heavily use Hux and see what the migration path would look like. I'm hoping it's as simple as a namespace change on the attributes (and probably a class name change). I don't think that'll be possible though until the sites are on Drupal 11.

  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan

    @acbramley, I've not used HUX but looking at the syntax, it looks like it's a matter of matching the new Hook attribute unless I'm missing something, but it definitely needs testing.

    On another note, I got excited and I created a follow up for converting core hook implementations.
    Naive estimate is there are 1382 implementations over 283 hooks.

    Obviously not all hooks are implemented and not all implementations can be converted, but that gives scope once this is in.

    I know it's premature to flesh out that plan, but we have a place for it.

  • 🇺🇸United States nicxvan
  • 🇩🇪Germany FeyP

    Reading through the CR, I wonder why we recommend creating a service for compatibility with older Drupal versions. What prevents usage of \Drupal::classResolver(), which is already the recommended way to call OOP implementations from hooks and does not require creating a service for each hook (class), thus providing better DX?

  • 🇩🇪Germany FeyP

    Okay, answering my own question here, I guess it is about the autowiring part. I can see how creating a service is then easier, if don't have an existing implementation of a "hook class" already. Probably also more fool proof to ensure that the autowiring actually works later even without a create method. I just shouldn't post to issues this early in the morning... Rock on and sorry for the noise.

  • 🇬🇧United Kingdom joachim

    @feyp with ClassResolver, you need to add a create() method if you want DI, so that's rather more boilerplate code to write.

    > Perhaps change the classname instead, HookHelper is more of a development artefact than anything else. HookOrder for example.

    +1 to HookOrder. Clearer than 'helper'

    > setAsFirst perhaps

    +1 to that too.

  • 🇺🇸United States nicxvan
  • 🇺🇸United States nicxvan
  • Pipeline finished with Failed
    3 months ago
    Total: 195s
    #275037
  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    To clarify what happened here, after the doxygen for this patch got absurd level of scrutiny far above what anyone else gets and I expressed my dismay over this and the overall state of doxygen gate for core, I got called names on Slack and got even more absurd demands, this time to provide documentation about a subsystem I have not been involved with for more than a decade, I kind of stopped working on this and I am grateful for nicxvan for all the work he did.

    Now, I have renamed HookHelper to HookOrder and renamed back the methods to first/last/before/after which they should be. There's no reason to bloat things even further.

    There won't be more pushes from me but after careful consideration I thought this needed to be done and said.

  • Assigned to nicxvan
  • 🇺🇸United States nicxvan

    Gonna get some missing renames

  • Issue was unassigned.
  • 🇺🇸United States nicxvan

    That should be good.

  • Pipeline finished with Failed
    3 months ago
    Total: 177s
    #275060
  • Pipeline finished with Success
    3 months ago
    Total: 538s
    #275079
  • 🇬🇧United Kingdom joachim

    > To clarify what happened here, after the doxygen for this patch got absurd level of scrutiny far above what anyone else gets

    I file a LOT of documentation bugs, and a good portion of them are nitpicks. You can check: I tag them as 'Novice' if they're really simple nitpicks.

    But I admit I did give the docs for this more scrutiny than other issues, though I didn't realise at the time. There is a reason though, and it's not personal at all. Unlike the Variation Cache issue (which is about something I barely understand) I needed to understand this issue's code, and RIGHT NOW. Because I wanted Module Builder to be able to generate code for the new style of hooks as soon as this is committed to core.

    So I was reading MR and its docs to try to understand how to use it, and immediately spotting lots of gaps in the docs.

    And then because as I was working on Module Builder, I was generating hook class code that was trying to match the NodeHooks class in the MR here, I was looking at the code as a new developer would, and spotting issues there too. For example that without the "Implements ..." docblock on the method, it won't join up to the documentation.

    > When the documentation gate is rewritten to be about developers instead of the API module

    The API module is for developers. It's important that documentation is to be written in a way that can be parsed, just as PHP code has to be written so PHP can parse it.

    And this issue is adding completely new pieces of code structure. Variation cache added some classes and services, and API module can already handle those. Here we're adding new stuff and we need to get it right.

    And as I have repeatedly pointed out on this issue, if we commit this without a corresponding update in the API module, then we will be BREAKING the API site.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    If this patch goes in then, I believe the only change will be on https://api.drupal.org/api/drupal/core%21modules%21user%21user.api.php/f... where the implementation of user_cancel by node module won't be linked. But actual breakage I do not expect despite you literally yelling about this. Where do you expect such?

    Luckily the D10 version of api.drupal.org has been launched a few months ago as per https://www.drupal.org/project/api/issues/3342815#comment-15578952 🐛 Classes missing from Drupal 10 Fixed and that uses a reasonably modern version of nikic/php-parser. So the parsing is here https://git.drupalcode.org/project/api/-/blob/2.x/src/Parser.php#L1489 and the api_references View needs to be amended to print these as well. I am not saying it's trivial but it's not too hard. If it weren't using php-parser then it would be practically impossible, yes, but it does.

  • 🇺🇸United States nicxvan

    Ah I see now, there is an issue for the api project Support for OO hooks implementations in classes Postponed .

    In my opinion that issue is not a blocker here since it won't be effected until we start converting implementations on a RELEASED version of Drupal.

    There are two implementations converted as part of this so work can begin.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Re #199 please stop spreading this lie:

    AccessPolicyInterface.php doesn't explain what's a $scope or a policy

    You disrespectfully complained about it before on Slack and when I pointed out that you had failed to read the official documentation , you deleted the thread. As I mentioned in said thread:

    Access policies are considered internal code, so their in-code documentation is limited. The AccessPolicyProcessor, however, goes into great detail in the code comments.

    If you want to stop working on something for whatever reason, that's your prerogative, but don't drag other people's work into it.

    On the subject of doxygen, I do agree that some aspects of core are more documented in code and others are more documented on the website. If you want to see that changed, you could create (or find) an issue for it, link to it here and then use that as a starting point to figure out whether or not this issue's MR should already adhere to what's being discussed.

    Again, if you'd rather not have that discussion and stop working on this MR altogether because of it, that's also fine. No-one is forcing anyone to work on something, we're mostly all volunteers here.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    I was never one for minute disrepectful, joachim calls me a diva and you call me a liar? WHAT THE EVEERLIVING FUCK IS GOING ON HERE?

    $scope is not explained, end!

  • 🇸🇰Slovakia poker10

    Thanks everyone for working on this issue!

    No need to be personal. Last comments seem to be a bit on the edge, so please focus on finishing the issue, so that we all can benefit from it. Thanks!

  • 🇬🇧United Kingdom joachim

    > got even more absurd demands, this time to provide documentation about a subsystem I have not been involved with for more than a decade.

    I assure you was not at all what I meant, though I can see that it could have been misread like that. I could have phrased it more clearly, and I'm sorry for the misunderstanding.

    What I was trying to say is that FormAPI is one example (of many!!!) where there is knowledge about how the system works which is ONLY in the heads of a small number of people, and not written down in the docs. This is bad because some developers don't know who to ask and give up, some developers waste time doing code archaeology, and some developers bother the small number of people for answers.

    I used this example because just the other week I was asking about FormAPI innards and you responded on Slack. The point I was trying to make was that getting the docs right is important because this is a situation we should avoid creating more of.

    > According to joachim, I am a diva

    I try hard to criticise *behaviour* rather than *people*. If I failed in that, I'm sorry.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    Can we get back to hooks, please?

  • 🇫🇷France andypost

    Would be great to get it in before Barcelona'24 to polish follow-ups at sprints

  • 🇬🇧United Kingdom catch

    For high level documentation discussion, see 🌱 Document high-level API concepts in an easier format Active which is specifically trying to deal with the mismatch between doxygen/api.drupal.org and d.o - I think that's the issue to have that discussion in.

    I'll try to review this again soon, it would be nice to reserve the remaining comments for reviews and not docs meta discussion since we are already over 200 here.

  • 🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    Please do not credit me on this issue. Thanks.

  • 🇺🇸United States nicxvan

    I rebased for the conflict, it was a performance test conflict.

    I will keep an eye on the tests.

  • 🇺🇸United States nicxvan

    There are a couple of suggestions from Support for OO hooks implementations in classes Postponed

    Suggestion 1:
    Update the constructor for the hook attribute to assign:

    $this->hook = static::PREFIX . $hook . static::POSTFIX;
    

    This will allow extending it to be cleaner.

    Suggestion 2:
    Remove FormAlter or rename FormAlter to something more resembles what it really is: hook_form_FORM_ID_alter.

    I'm making change 1, then renaming FormAlter to FormAlterId then overriding the prefix and postfix.

  • Pipeline finished with Failed
    3 months ago
    Total: 278s
    #282811
  • 🇦🇺Australia mstrelan

    I can see a future where we use form classes instead of ids, so I'd rather not use FormAlterId. Plus it sounds like you're altering the id rather than the form. Would be nice to just pass the relevant form class to the form alter hook, and it can alter any subclass.

  • Pipeline finished with Failed
    3 months ago
    Total: 186s
    #282814
  • Pipeline finished with Canceled
    3 months ago
    Total: 287s
    #282821
  • 🇺🇸United States nicxvan

    I agree. This probably should move to a follow up to be honest, but what about something like

    FormAlter
    BaseFormAlter
    SpecificFormAlter

    To match:
    hook_form_alter
    hook_form_BASE_FORM_ID_alter
    hook_form_FORM_ID_alter

    I'm inclined to postpone this to a follow up.

  • Pipeline finished with Success
    3 months ago
    Total: 499s
    #282826
  • 🇺🇸United States nicxvan

    We could also name if FormAlterById

  • 🇺🇸United States nicxvan

    Ok I think I figured out a clean way to do this.

    I added an IMPLEMENTS constant we can use in the documentation issue.

    I also updated FormAlter so it can only implement hook_form_alter.
    I also updated FormAlterById so that it takes the form id as a parameter.

  • Pipeline finished with Success
    3 months ago
    Total: 704s
    #283394
  • 🇬🇧United Kingdom catch

    I went through this again and it's incredibly compact given what it does. Obviously we have all the conversions to follow later!

    Mostly only had comment nits and +1s.

    I want to separately open an issue to look into creating a filecache-backed class attribute getter - i.e. given a file, get the attributes/an attribute from the file - but where we could use filecache to only actually do the reflection work once. However that issue is orthogonal to this one (and also to annotation/attribute conversion), we can work on all this in parallel. However I'll link that issue here once I've actually opened it.