🇺🇸United States @EclipseGc

I added relevant commentary here: https://www.drupal.org/project/drupal/issues/3366083#comment-15549894 🌱 [META] Hooks via attributes on service methods (hux style) Active

So I think I'm going to post here, but I'll cross post a link to this comment in 📌 Hux-style hooks, proof of concept Needs work .

I want to come at this from a slightly different angle. The HUX style hook attribute stuff is very cool, and I'm not against it at all, but we have two competing systems in hooks vs event subscribers, and this issue/effort does little to resolve that. To that end, I'm going to provide a POC patch here. It requires a lot more work, but you can drush si a site with the patch applied, you can install new modules, and almost all hooks are working to the best of my testing (this will fail test coverage as I've not migrated any of that over).

Goal

1. Unify hooks and event subscribers
2. Do so with NO Event class boilerplate
3. Event subscriber methods should get the same parameters as corresponding hooks
4. Event subscribers and hooks should be able to be interwoven in their execution order

Caveats

1. I've not attempted to migrate preprocess hooks (That is, I've not done any work on Registry.php)
2. I've not looked at how update hooks fire, my solution might work for them... it might not.

The How

Others have attempted in the past to unify hooks and event subscribers, and we still don't have a solution. While poking at the problem space I noticed a few important things:

1. We don't need "Event" classes if we don't call EventSubscriber::dispatch()
2. Calling EventSubscriber::getListeners() manually (it's public) lets you invoke them however you like, with whatever parameters you like
3. In Drupal 10.2, the ContainerAwareEventDispatcher was happy to take a string as a Callable. During this process I moved the patch to be against 11.x and found that we've moved to use the core Symfony EventDispatcher. It was a little more persnickity, but I got it working.

In order to do this, I've got a compiler pass. It starts by finding all installed modules, and their weight as detailed in core.extension.yml. It then finds any method in .module and .install that COULD be a hook. That means it will wrongly identify functions with the module name prefix as being potential hooks even when they're not. I don't see this as being particularly harmful. The resulting map of function names is relatively small all things considered. Once this list is built, I manually invoke hook_hook_info() in the compiler pass. I use that to find other files that can hold particular hook implementations. Since we know which hooks will appear there, we can be sure to not mis-identify any that exist. We also note which file hook implementations are found in. This will be useful later.

Once we've done this, we re-organize the information and manually invoke hook_module_implements_alter(). The result of that is used to modify the weights of each hook implementation that has been altered. This allows us to maintain an execution order that will keep things in line with Drupal's existing expections while allowing us to also bubble up that information in such a way that it can be stated in "priority" order to the event dispatcher. Doing this will allow regular event subscribers to set priority between existing hook implementations with different priorities.

Once that's complete, we register our hooks on the event dispatcher as listeners to particular hooks and write our list of hooks and corresponding implementations to the container as a parameter any service can use.

The Hook Caller

As I mentioned earlier, the vanilla Symfony Event Dispatcher was not fond on taking function names as callables. In order to make that work I invented a HookCaller class that is small, and useful. It implements the magic __call() method, so we can call any hook implementation against it. It then uses the documented hook implementations from the previous step to figure out which file the implementation exists within, load that file, and then call the function. I initially sort of hated this solution, but now I kind of love it. It really simplifies a lot of complicated things that module handler/loader/stuff all did.

The Module Handler

My code here is still very much in flux and needs some love, but the important bits are there-ish. The invoke method is wrong. To do this "right" we need to identify EventSubscribers to particular hooks, what module's namespace they exist within, and ensure they're invoked too. The code is working right now, and does some weirdness around letting you invoke particular service methods, but that should be replaced by what I've just described.

Similary, with the compiling of a list of hooks to the container, I suspect we no longer need the cache backend stuff that's happening here. I didn't dig in too far into that, it's just a hunch.

Misc changes and thoughts

I had to modify hook_page_top/bottom callers. I think that might be able to be moved back to it's original state with some changes in the hook.caller, but I was tired of messing with it, and this made it work.

I just barely started to poke at the theme side of the equation before I decided to stop and get feedback on this. As mentioned earlier Registry.php will need some changes too.

I added some methods to the ModuleHandler. Again, this is a POC. Some of that stuff can probably be done better than what I did here.

Bringing this all back around to HUX, I bet a simple Trait could be written that generates the getSubscribedEvents() method body from method Attributes. There are a dozen ways that could be done... I'm just trying to keep it somewhat on topic here while I try to make hooks/events the same thing for existing Drupal hooks.

Summary

When we're in the issue queue, there's often a question of "is this a new hook? or event?". I've seen it go both ways, and no one ever seems completely satisfied with the answer. While traditional event subscribers with a custom event class are sort of a different thing from this, my solution here mean we can largely make both camps happy when inventing new "api" level considerations. We don't have to discuss if it's a hook or an event. We only have to discuss if it would be better served with a custom event class, or just passing parameters.

I've attached the patch, and here's an example of what an event subscribe could look like with this patch applied. This code works, it's not a mock up.

namespace Drupal\kris_test\EventSubscriber;

use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Routing\RouteMatchInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

class ExampleHooks implements EventSubscriberInterface {

  /**
   * @inheritDoc
   */
  public static function getSubscribedEvents() {
    $events = [];
    $events['hook_help'][] = ['onHookHelp'];
    $events['hook_page_top'][] = ['onPageTop'];
    $events['hook_form_alter'][] = ['onFormAlter'];
    $events['hook_form_search_block_form_alter'][] = ['onFormSearchBlockFormAlter'];
    return $events;
  }

  #[Hook('help')]
  public function onHookHelp(string $route_name, RouteMatchInterface $rout_match) {
    return 'This is a test';
  }

  public function onPageTop(&$page_top) {
    $page_top['test'] = [
      '#markup' => 'this is a test'
    ];
  }

  public function onFormSearchBlockFormAlter(array &$form, FormStateInterface $form_state) {
    $form['more_test'] = [
      '#markup' => 'This is a search block specific customization'
    ];
  }

  public function onFormAlter(array &$form, FormStateInterface $form_state, string $form_id) {
    $form['test'] = [
      '#markup' => 'This is form: ' . $form_id
    ];
  }
}

hestenet → credited EclipseGc → .

hestenet → credited EclipseGc → .

hestenet → credited EclipseGc → .

hestenet → credited EclipseGc → .

hestenet → credited EclipseGc → .

hestenet → credited EclipseGc → .

hestenet → credited EclipseGc → .

hestenet → credited EclipseGc → .

hestenet → credited EclipseGc → .

Webchick!

I hope you know you'll be incredibly missed. I hope your current work is supremely fulfilling. We'll all miss you. Go make ANOTHER dent in the universe.

hestenet → credited EclipseGc → .

hestenet → credited EclipseGc → .

hestenet → credited EclipseGc → .

hestenet → credited EclipseGc → .

larowlan → credited EclipseGc → .

Adding myself to the ping list

@FiNeX

We'd need the layout section to support such a thing. Definitely a separate issue from this one. It needs its own acceptance criteria and evaluation of whether or not we want to actually do it.

Context tokens are 100% a thing we should be eventually chasing down, but not a thing we really do today. They're especially useful for content blocks, but it's definitely a separate feature.

Eclipse

Ok great, so this is not a bug that you've managed to make happen w/o a core patch. That's good news.

Plugins don't require contexts FROM a specific place. They require contexts of a specific type (say, "node" for example). The config saves the expected key from the contexts array that was configured through the UI, so contexts that are available through the admin that cannot be calculated during the runtime of a page are unsafe. There's a fair amount of code elsewhere in core reenforcing that approach. The best thing to do for the patch in question would be to toss a try/catch around the instantiation of the plugin. If we catch, we can return something else and add higher priority event subscriber that logs the error, stops propagation and returns an empty array as the render of the block without invoking the rest of the event subscribers.

I've not looked at the patch in question, however one of the reasons we've not yet gone after "relationship" style contexts is precisely because we have to compensate for this situation in which the context is calculate-able in the administration, but doesn't actually exist during run time. I'd say it's the domain of that patch to fix. But the problem doesn't invalidate the current code path's approach. We know and understand that this is a real thing. We planned for it and have dealt with it similarly elsewhere.

@see:

\Drupal\block\BlockAccessControlHandler::checkAccess lines 92 & 122

It's not a perfect match in that case because we specifically delay context mapping until access checking in the block module approach so we can just return AccessResults in that case. In this case, our new higher priority event subscriber would need to return an empty build array, but it also needs to addCacheableDependency() for an AccessResult::forbidden() on the event itself. It all seems super achievable, but again I'd say it's outside the scope of this issue. However once this issue lands, ✨ Make it possible to add relationships to layout builder Needs work would need to do something similar for visibility plugins to try/catch their context mapping. Still it's 3001188's issue, not this one's.

Eclipse

Neither issue cited above gives insight into how you've managed to achieve this particular bug. Also, I question your premise. Let me explain why:

• \Drupal\layout_builder\Section::toRenderArray() is responsible for getting the render array of all the blocks and regions representative of a given section.
• \Drupal\layout_builder\Section::toRenderArray() calls: \Drupal\layout_builder\SectionComponent::toRenderArray()
• \Drupal\layout_builder\SectionComponent::toRenderArray() dispatches the Event in question.
• Contexts are already provided to the event object which is capable of getting a block plugin via: \Drupal\layout_builder\Event\SectionComponentBuildRenderArrayEvent::getPlugin()
• The event's __construct() method delegates all the instantiation and context setting bits of the block to: \Drupal\layout_builder\SectionComponent::getPlugin()
• Event subscribers are free to get the plugin or configuration details and make use of it as necessary.
• \Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray actually calls the build() method of the block. This event has a priority of 100.

With that flow in mind, the block plugin, with its context(s) (set properly or otherwise) exists before the render array is attempted to be generated. This fact means there are two potential solutions to the problem you outline above (though again, how you got there is beyond me since the UI strictly prevents the system from getting into this sort of state in the first place).

1. We can properly nuance the existing event subscribers to try/catch this themselves and return empty build arrays with the appropriate caching logic as necessary.
2. It might be possible to build an event subscriber which considers all the plugins under the section and checks their contextual status w/o invoking them. This sounds like new-api territory, so again without understanding how you got to this point I suspect this is a step further than is necessary.

If I've misinterpreted your issue, please elaborate further. I do not favor moving away from the EventSubscriber approach. It buys us too much.

Eclipse

Yeah, that's a failing of the css reset on the settings tray. An on-going problem in core. :-( I've reached out to people who would know how best to solve it.

Eclipse

Also, the patches I've provided are against head and no longer dependent on the defaults issue, so apply these patches, and forgo the defaults patch for the time being (until it lands and I reroll).

curl https://www.drupal.org/files/issues/2937799-8.patch | git apply; curl https://www.drupal.org/files/issues/2916876-16.patch | git apply; curl https://www.drupal.org/files/issues/2937159-layout_drag-2.patch | git apply; curl https://www.drupal.org/files/issues/2936642-context-2.patch | git apply

Yeah, it's a totally annoying thing, but you'll need to follow Tim's directions. :-(

Eclipse

Ok, fixed the missing end line I had in the last patch and made editing a configured visibility condition actually work now.

Eclipse

Ok, I've rerolled this on top of #2937799: Allow greater flexibility within SectionComponent::toRenderArray() → after abstracting that code out of this patch. Also, this should apply directly to core now with only that 1 patch applied. Layout defaults are a separate thing and I don't think it's affected by this code path at all.

I've not provided an interdiff because I actually wasn't certainly how best to get one given that much of the code I removed from the patch still exists in the code base. Whatever the case, the main patch should be significantly smaller now. Fixed a couple of docblocks I'd missed here and there.

Eclipse

Also, if you're wanting to try out a full demo of what we're pursuing, this is accurate as of now, and all the patches should layer just fine:

git clone --branch 8.6.x https://git.drupal.org/project/drupal.git layout_builder
cd layout_builder; curl https://www.drupal.org/files/issues/2922033-layout_defaults-62.patch | git apply; curl https://www.drupal.org/files/issues/2916876-13.patch | git apply; curl https://www.drupal.org/files/issues/2937159-layout_drag-2.patch | git apply; curl https://www.drupal.org/files/issues/2936642-context-2.patch | git apply; composer install

So hopefully that helps with bootstrapping review.

Eclipse

Ok, I didn't have a chance to reroll against only head, so this patch is dependent on #2922033: Use the Layout Builder for EntityViewDisplays → comment 62. I will get a version of this patch for core Jan 18th.

There are a bunch of interesting bits in the interdiff. Most importantly, I rewrote how SectionComponents are rendered at all. This will likely need to be split out into a parent issue of this issue, but for the sake of explaining what's here, I converted SectionComponent render array building into an event which is dispatched. This is super powerful because it means other modules can get involved in the render array building process. It also means that layouts COULD potentially render things which are not blocks. Tim wrote the original code that way and I doubled down on it here. I don't think it's a feature we'll practically use, but we essentially get it for free moving to the event dispatcher. Furthermore, being able to run before or after the normal build process means that introducing something like "visibility" into that paradigm became a simple event subscriber instead of me modifying the monolith that did the render work for us (which is what the last patch did).

Net 0 for the SectionComponent class at the moment. The interdiff removes the condition manager that the first patch added, but adds in the event dispatcher service. Still, that's a practical service when I felt that the condition manager really wasn't.

I'll post more tomorrow. (I'm pretty certain visibility condition editing, not creating, is broken. So that's one of tomorrow's tasks.)

Eclipse

Will do. I'll reroll this shortly.

Eclipse

Developed against preview versions of the patches we were dependent on. It's also dependent on #2922033: Use the Layout Builder for EntityViewDisplays → , so this won't work w/o that.

Eclipse

Sorry for the trash in that patch. I'll fix it in the morning.

Eclipse

How about something along these lines?

Eclipse

EclipseGc → created an issue. See original summary → .