🇮🇩Indonesia @el7cosmos

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

I wouldn't say hook implementations can replace event dispatcher altogether.

I can add a listener to the event dispatcher dynamically on runtime.

$anon = new class {
  public function userCancel(): void {
  }
};
$eventDispatcher->addListener(UserCancelEvent::class, [$anon, 'userCancel']);

When the event is dispatched, the anonymous class's method is called. However, the same thing does not work with hook implementations.

$eventDispatcher->addListener('drupal_hook.user_cancel', [$anon, 'userCancel']);

I'm not sure if this is a bug or if this is intended, but at the very least there is a feature missing if we want to replace event dispatcher.

larowlan → credited el7cosmos → .

I tried adding duplicate components in test, but can’t reproduce the crash.
Q: do we have any plugin manager/discovery that throws exception when there are duplicate plugins?

HEAD is now all green https://git.drupalcode.org/project/hook_event_dispatcher/-/pipelines/361694

Closing as 📌 Revert case sensitive use-statement sniff Active landed

I have a try at this. I move the responsibility to check the requirements to the component source plugin.

There is not much change in MR; it only moves some code and adds new methods for the component source plugins.

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

I can start with decorating the BlockManager.

Is storing the reasons in the component source definition make sense?

I can start with decorating the BlockManager.

Does storing the reasons in the component source definition make sense?

When disabling a block component, it will get into \Drupal\experience_builder\Plugin\ComponentPluginManager::REASONS_STATE_KEY already.

This is the snippet that does this in \Drupal\experience_builder\Controller\ComponentStatusController::performOperation:

$reasons = $this->state->get(ComponentPluginManager::REASONS_STATE_KEY);
if ($op === 'disable') {
  $component->disable()->save();
  $reasons[$component->getComponentPluginId()] = 'Manually disabled';
}
...
$this->state->set(ComponentPluginManager::REASONS_STATE_KEY, $reasons);

But, this state will be rebuilt each time component plugin definitions are rebuilt, which only accounts for SDC source. Perhaps each component source can define its own state key for where to store its reasons, and then use \Drupal\Core\State\StateInterface::getMultiple for each component source in \Drupal\experience_builder\Controller\ComponentStatusController

lets wait for 📌 Revert case sensitive use-statement sniff Active , and no need to change the phpcs config

wim leers → credited el7cosmos → .

Postponing as this possibly due to custom code or other contrib modules. The view mode isn't meant to be null, core itself have 'full' for default value.

left some comments in the MR

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

el7cosmos → changed the visibility of the branch 2.7.x to hidden.

wim leers → credited el7cosmos → .

I had an attempt at this. Components inserted via the sidebar will then be selected, although the component setting panel will be opened before the component is fully rendered.

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

Here are issues on sortable js library: https://github.com/SortableJS/Sortable/issues?q=esc+

Oops sorry, accidentally changed status

Apologize for coming late, I had some time looking at this.

When I disable a block component via /admin/structure/component, I can't enable it back again with an error message

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "local_tasks_block" plugin does not exist.

.

Is this something to worry about? Or should this be a follow-up issue?

el7cosmos → created an issue.

/**
 * This hook is NOT invoked for the 'user_cancel_delete' account cancellation
 * method. To react to that method, implement hook_ENTITY_TYPE_predelete() or
 * hook_ENTITY_TYPE_delete() for user entities instead.
 */

This can be achieved by subscribing to:

• \Drupal\core_event_dispatcher\EntityHookEvents::ENTITY_PRE_DELETE
• \Drupal\core_event_dispatcher\EntityHookEvents::ENTITY_DELETE

I just added a test case to validate the bug

This is a valid, both \Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent::getArguments and \Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent::setArguments won't work.

I updated tests for 10.3 per #3459926-14: Update CKEditor 5 to 42.0.0 → . I have pushed the changes but can't reopen the MR, should I create a new MR?

el7cosmos → changed the visibility of the branch 3477799-bump-ckeditor5-10.3.x to hidden.

el7cosmos → changed the visibility of the branch 3477799-bump-ckeditor5-10.3.x to active.

It's a best practice to inject the interface instead, as implementation can change.

  /**
   * The module handler service.
   *
   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   */
  protected \Drupal\Core\Extension\ModuleHandlerInterface $moduleHandler;

Injecting interface means you can swap implementation without changing the code.

I still think this is a duplicate as it is the same issue. Can you provide more detail if this is not a duplicate?

el7cosmos → created an issue. See original summary → .

Pushed a fix addressing #167 →

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

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

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

el7cosmos → created an issue.

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

el7cosmos → created an issue.

el7cosmos → created an issue.

el7cosmos → created an issue.

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

Tests on D11 are still blocked on webform dependency, we need to figure out what to do with it.

One option is to remove webform from dev dependency and skip the webform tests, unless we can separate dependency for D10 and D11.

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

Most plugins already have a common interface. An interface for each plugin seems too verbose for most and probably ends up being an empty interface.

From what I understand, the concern was raised because there is no way yet to deprecate a plugin and this is an issue for FQCN as plugin ID, not as alias. Alias definition will still have the same plugin ID (eg Entity type with alias Node::class will still have a definition with ID node). Plugin ID will still end up in configurations, alias will always used in code and never in configurations. When a plugin class is deprecated this won't be a problem as long as the class is tagged as @deprecated.

I just removed additional test

I don't think the related issue is a parent of this, the discussion is more about the plugin ID itself. But in this, we keep the plugin ID as it is, and introduce an alias, which is new, and we can set a new convention around it without worrying about backward compatibility.

As for the FQCN, using FQCN as a method/function argument doesn't necessarily mean we are requesting an instance of that class. If we take a look at this:

\Drupal::entityTypeManager()->getStorage(\Drupal\node\Entity\Node::class);

We don't want an instance of that class, we want something else.

Using FQCN as an argument means better IDE support which will then improve DX.

I make an attempt for aliases in 💬 Allow plugin definition to be aliased Active

That at least works for something like this:

\Drupal::entityTypeManager()->getStorage(\Drupal\node\Entity\Node::class);
\Drupal::entityTypeManager()->getDefinition(\Drupal\node\Entity\Node::class);

Adding aliases directly to the definitions causes problem with various plugin manager as they need some properties of the definitions when processing it. Changing the IS for a different approach.

If we want this by default, do we need to update the existing field config?

It may also make more sense to have "Show all items"/"Show all widgets" instead.

el7cosmos → created an issue.

Thanks, Debdeep for the work.

Can we have a logo that represents events instead?

the ::testProcessChildDefinitionParentInterfaceMismatch method is supposed to pass even without the changes

hestenet → credited el7cosmos → .

hestenet → credited el7cosmos → .

I propose a different approach, instead of using the class name as plugin ID (which will be a problem for derivative definitions), we can introduce plugin alias, just like service container can have service alias.

For example, container element can have definition something like this

$definitions = [
  'container' => [
      'id' => 'container'
      'class' => 'Drupal\Core\Render\Element\Container'
      'provider' => 'core'
  ],
  'Drupal\Core\Render\Element\Container' => [
    'alias' => 'container',
  ],
];

And for definition with \Drupal\Component\Plugin\Definition\PluginDefinition instance, we can introduce AliasPluginDefinition and have something like this:

$definitions = [
  // instance of \Drupal\Core\Entity\ContentEntityType
  'node' => '',,
  // instance of \Drupal\Component\Plugin\Definition\AliasPluginDefinition that have `alias` property with the value `node` 
  'Drupal\node\Entity\Node' => '',
];

Derivative definitions won't have an alias for them, but can still be accessed with their ID.

A plugin discovery should be responsible for building the alias definitions and returning a correct definition if an alias plugin is requested.

The drawback is the plugin definitions will grow in size, but shouldn't be too much. If this is a problem, there should be a way for plugin manager to opt out of this behaviour (eg. Block plugin manager can decide to not have an alias for all their definitions).

In the end, we can get a plugin definition and create its instance either by ID or its alias if it has one.

This is already on the Essentials plugin, so just need to register the toolbar.

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

el7cosmos → created an issue.

Updated the MR, added a widget option to opt-in

Fix in 🐛 Drupal 10.3 compatibility Fixed and released a new version 4.0.3 →

Thanks, this has been added to the module page.

Thanks, this has been added to the module page.

el7cosmos → created an issue.

In the case of module handler, even if without changes from 📌 Allow needs_destruction services to run on page cache hits Needs review , decorators that don't implement DestructableInterface will also fail in 10.2 if we apply 📌 Replace RequestCloseSubscriber with needs_destruction tag on ModuleHandler Fixed .

this just because the call changed from writeCache which declared in ModuleHandlerInterface to destruct which is new implementation.

As for 📌 Allow needs_destruction services to run on page cache hits Needs review , I don't think we can access decorated/original service from DrupalKernel, so even if we register the original service in kernel.destructable_services (eg hux.module_handler.inner) we won't be able to get it from the Kernel with $this->container->get('hux.module_handler.inner').

Perhaps, instead of a parameter we can create a service that can receive the original service as the argument, but that will initialize the instance and defeat the purpose of $this->container->initialized($id).

I can't see any other solution than:

• Implement DestructableInterface from the service's interface, not implementation, this will enforce decorators to implement that also.
• Let it be the decorator's responsibility to implement DestructableInterface, but the container will crash if it is not.

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

I created an MR to decorate the event dispatcher service instead, but this only works in 10.3.

If we want this on 10.2 also, we can create a compiler pass similar to https://git.drupalcode.org/project/drupal/-/commit/6e733866fe414d0521be3... that only works for <10.3 (eg using deprecation helper)

el7cosmos → created an issue.