Have we already decided to use import map? Another option with the new installation method is using UMD build, which shouldn't be blocked by anything
I do more benchmarking with https://www.php.net/manual/en/class.ds-set.php extension with add() method, and it is the most performant with the same behaviour as array_unique(array_merge())
I did some benchmarks with this
use Drupal\Core\Cache\Cache;
final class CacheMergeTagsBench {
private const int RANGE = 100;
public function benchCore(): void {
$tags = [];
foreach (range(1, self::RANGE) as $i) {
$tags = Cache::mergeTags($tags, [(string) $i, (string) $i]);
}
}
public function benchArrayPushUniqueInLoop(): void {
$tags = [];
foreach (range(1, self::RANGE) as $i) {
foreach ([(string) $i, (string) $i] as $tag) {
$tags[] = $tag;
}
$tags = array_values(array_unique($tags));
}
}
public function benchArrayMergeUniqueOutsideLoop(): void {
$tags = [];
foreach (range(1, self::RANGE) as $i) {
$tags = array_merge($tags, [(string) $i, (string) $i]);
}
$tags = array_values(array_unique($tags));
}
public function benchArrayPushUniqueOutsideLoop(): void {
$tags = [];
foreach (range(1, self::RANGE) as $i) {
foreach ([(string) $i, (string) $i] as $tag) {
$tags[] = $tag;
}
}
$tags = array_values(array_unique($tags));
}
}
Here are the results:
RANGE = 100
CacheMergeTagsBench =================== Average iteration times by variant 232.2μs │ █ 203.1μs │ █ 174.1μs │ █ 145.1μs │ █ 116.1μs │ █ █ 87.1μs │ █ █ 58.0μs │ █ █ █ 29.0μs │ █ █ █ ▇ └───────── 1 2 3 4 [█ <current>] 1: benchCore 2: benchArrayPushUniqu᠁ 3: benchArrayMergeUniq᠁ 4: benchArrayPushUniqu᠁ <current> +-------------------------------------+---------+-----------+---------+-----------+ | subject | memory | mode | rstdev | stdev | +-------------------------------------+---------+-----------+---------+-----------+ | benchCore () | 8.648mb | 232.153μs | ±81.02% | 312.908μs | | benchArrayPushUniqueInLoop () | 8.649mb | 115.196μs | ±3.33% | 3.899μs | | benchArrayMergeUniqueOutsideLoop () | 8.649mb | 57.742μs | ±6.06% | 3.611μs | | benchArrayPushUniqueOutsideLoop () | 8.649mb | 24.448μs | ±7.30% | 1.855μs | +-------------------------------------+---------+-----------+---------+-----------+
RANGE = 1_000
CacheMergeTagsBench =================== Average iteration times by variant 8.7ms │ █ 7.6ms │ █ ▃ 6.5ms │ █ █ 5.4ms │ █ █ 4.3ms │ █ █ 3.2ms │ █ █ █ 2.2ms │ █ █ █ 1.1ms │ █ █ █ ▂ └───────── 1 2 3 4 [█ <current>] 1: benchCore 2: benchArrayPushUniqu᠁ 3: benchArrayMergeUniq᠁ 4: benchArrayPushUniqu᠁ <current> +-------------------------------------+---------+-----------+--------+-----------+ | subject | memory | mode | rstdev | stdev | +-------------------------------------+---------+-----------+--------+-----------+ | benchCore () | 8.648mb | 8.665ms | ±2.87% | 251.757μs | | benchArrayPushUniqueInLoop () | 8.649mb | 6.787ms | ±0.78% | 52.716μs | | benchArrayMergeUniqueOutsideLoop () | 8.649mb | 3.171ms | ±1.35% | 42.865μs | | benchArrayPushUniqueOutsideLoop () | 8.649mb | 176.902μs | ±2.94% | 5.238μs | +-------------------------------------+---------+-----------+--------+-----------+
RANGE = 10_000
CacheMergeTagsBench =================== Average iteration times by variant 803.8ms │ █ 703.3ms │ █ ▃ 602.8ms │ █ █ 502.4ms │ █ █ 401.9ms │ █ █ ▂ 301.4ms │ █ █ █ 200.9ms │ █ █ █ 100.5ms │ █ █ █ ▁ └───────── 1 2 3 4 [█ <current>] 1: benchCore 2: benchArrayPushUniqu᠁ 3: benchArrayMergeUniq᠁ 4: benchArrayPushUniqu᠁ <current> +-------------------------------------+---------+-----------+---------+-----------+ | subject | memory | mode | rstdev | stdev | +-------------------------------------+---------+-----------+---------+-----------+ | benchCore () | 8.648mb | 803.765ms | ±19.56% | 174.104ms | | benchArrayPushUniqueInLoop () | 8.649mb | 630.155ms | ±0.17% | 1.100ms | | benchArrayMergeUniqueOutsideLoop () | 8.649mb | 322.530ms | ±20.93% | 75.267ms | | benchArrayPushUniqueOutsideLoop () | 8.649mb | 1.899ms | ±5.04% | 98.035μs | +-------------------------------------+---------+-----------+---------+-----------+
RANGE = 20_000
CacheMergeTagsBench =================== Average iteration times by variant 3.3s │ █ 2.9s │ █ ▃ 2.5s │ █ █ 2.0s │ █ █ 1.6s │ █ █ ▂ 1.2s │ █ █ █ 817.6ms │ █ █ █ 408.8ms │ █ █ █ ▁ └───────── 1 2 3 4 [█ <current>] 1: benchCore 2: benchArrayPushUniqu᠁ 3: benchArrayMergeUniq᠁ 4: benchArrayPushUniqu᠁ <current> +-------------------------------------+---------+---------+---------+-----------+ | subject | memory | mode | rstdev | stdev | +-------------------------------------+---------+---------+---------+-----------+ | benchCore () | 8.648mb | 3.270s | ±7.65% | 264.635ms | | benchArrayPushUniqueInLoop () | 8.649mb | 2.558s | ±7.49% | 198.958ms | | benchArrayMergeUniqueOutsideLoop () | 8.649mb | 1.318s | ±10.64% | 147.986ms | | benchArrayPushUniqueOutsideLoop () | 8.649mb | 3.752ms | ±2.05% | 76.243μs | +-------------------------------------+---------+---------+---------+-----------+
Calling both array_merge and array_unique in a loop is a significant performance hit. Array merge can be replaced with $tags[] = $tag without any side effects, as we don't care about the array keys.
el7cosmos → made their first commit to this issue’s fork.
I do wonder if we should strip the
data-entity-metadataattribute on page output, this can expose information that wouldn't have been exposed before when linking to a page, most importantly the author. As an anonymous user I see this in the markup:
Since this already requires a filter plugin, it is probably worth removing that attribute in the filter.
I've changed the actionsView to toolbarView.
I also removed previewButton.setTemplate as I'm not sure if it is still required. Instead, I bind the preview button href and label property to the entity metadata.
There are failed tests, which I think are related to the UI changes, but I don't have the bandwidth to look at them at the moment.
el7cosmos → created an issue.
el7cosmos → created an issue.
el7cosmos → made their first commit to this issue’s fork.
el7cosmos → created an issue.
frankenphp server won't start properly on https://www.drupal.org/project/drupal/issues/3437187 📌 Add Caddyfile configuration Active .
running frankenphp php-server -a -d gives
Error: flag needs an argument: 'd' in -d
we should get rid of -d or pass something like localhost.
looking at the failed test
cURL error 7: Failed to connect to localhost port 80 after 0 ms: Couldn't connect to server
this seems like the frankenphp server not started properly.
I made a new MR based on 3.x branch as I can't change the base branch on the existing MR
el7cosmos → made their first commit to this issue’s fork.
el7cosmos → created an issue.
el7cosmos → created an issue.
el7cosmos → made their first commit to this issue’s fork.
Thanks for reporting
el7cosmos → made their first commit to this issue’s fork.
Added some comments in the MR
This is no longer needed, HEAD is already green and there are no code sniffer issues.
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 → .
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?