Problem/Motivation

The list of available plugins is not static, as the derivative system allows you to create a dynamic list of "subplugins"
based upon entities, configuration, content and what not. While this is a really nice feature it results in the fact that these
plugins might disappear.

One example: The menu module provides one block per menu. Once a menu is removed the corresponding block configuration should disappear itself. Once you are in contrib world, also the corresponding panels configuration should be cleaned up.

Proposed resolution

  • Allow tell the plugin manager that a certain plugin ID is removed
  • The plugin manager throws an event, which allows various subsystems to subscribe to that event.

Remaining tasks

User interface changes

API changes

Original report by @sdboyer

@dawehner pointed me to #2018493: Tests for: FATAL error when a view with a placed block is disabled or removed, which made me realize that we're letting other code assume things about how block plugins get placed and used that they should not be assuming. specifically, this code. that code deletes all block plugin instances that are based on a particular views block display when that display is removed. this is the outcome we want, but it's not the way to achieve it - even after that patch goes in.

the problem is that any other system that might be wanting to create or manage block instances in its own way - like, say, how princess does right now - is left out in the cold. so, this behavior needs to be changed to invoke a hook with the ID of the disappearing block plugin, so that all block implementations can listen and update their datastructures accordingly.

moreover, that hook should have a flag that is used to indicate whether the disappearance of the block plugin identified by the provided ID is (more) permanent or temporary. e.g., deleting a view with a block display is a permanent deletion, but merely disabling the block display on that view is equally temporary (disabling a module is a tougher question, but the flag still helps us be more granular) this way, the hook implementations can potentially choose to 'stash' the created block instances in some way for the temporary case, rather than deleting it, which could be a big gotcha for users.

now, yes, the example i linked to above is within block.module, but that specific plugin's responsibility is to (ultimately) provide block plugins via a derivative - which means it's on the block-plugin-producing side of block.module's responsibility, not the block-instance-placing-and-management side. and keeping block plugins, or their producers, from assuming anything about block plugin instance storage, is the crux of this issue.

CommentFileSizeAuthor
#163 2031859-163.patch128.14 KBdawehner
#159 plugin_manager-2031859-159.patch128.07 KBLetharion
#151 plugin_manager-2031859-150.patch129.48 KBLetharion
#148 plugin_manager-2031859-148.patch129.55 KBLetharion
#147 plugin_manager-2031859-145.patch129.38 KBLetharion
#140 interdiff.txt1.58 KBpfrenssen
#140 plugin_manager-2031859-140.patch129.31 KBpfrenssen
#137 interdiff.txt1.4 KBpfrenssen
#137 plugin_manager-2031859-136.patch130.75 KBpfrenssen
#133 plugin_manager-2031859-133.patch130.67 KBpfrenssen
#133 interdiff.txt15 KBpfrenssen
#131 interdiff.txt14.61 KBpfrenssen
#131 plugin_manager-2031859-131.patch133.2 KBpfrenssen
#127 interdiff.txt5.92 KBdawehner
#127 plugin_manager-2031859-127.patch131.45 KBdawehner
#123 interdiff.txt800 bytesdawehner
#123 plugin_manager-2031859-123.patch131.19 KBdawehner
#121 interdiff.txt12.89 KBdawehner
#121 plugin_manager-2031859-121.patch130.69 KBdawehner
#120 interdiff.txt1.41 KBdawehner
#120 plugin_manager-2031859-120.patch117.63 KBdawehner
#119 interdiff-114-118.txt1.84 KBmartin107
#118 plugin_manager-2031859-117.patch122.4 KBdawehner
#116 interdiff.txt111.58 KBdawehner
#116 plugin_manager-2031859-116.patch111.58 KBdawehner
#114 interdiff.txt1.96 KBdawehner
#114 plugin_manager-2031859-114.patch120.56 KBdawehner
#110 interdiff-108-110.txt878 bytesmartin107
#110 plugin_manager-2031859-110.patch119.35 KBmartin107
#108 plugin_manager-2031859-108.patch118.49 KBmartin107
#106 plugin_manager-2031859-106.patch118.48 KBmartin107
#106 interdiff-104-106.txt757 bytesmartin107
#104 interdiff.txt100.38 KBdawehner
#104 plugin_manager-2031859-104.patch117.74 KBdawehner
#103 interdiff.txt15.33 KBpfrenssen
#103 2031859-plugins-103.patch22.48 KBpfrenssen
#95 interdiff.txt725 bytesdawehner
#95 2031859-plugins-95.patch19.59 KBdawehner
#93 interdiff.txt16.25 KBdawehner
#93 2031859-plugins-93.patch19.6 KBdawehner
#90 2031859-plugins.patch4.83 KBdawehner
#89 2031859-do-not-test.patch1.13 KBdawehner
#86 block-2031859-86.patch14.94 KBmartin107
#86 interdiff-83-86.txt1.55 KBmartin107
#83 block-2031859-83.patch15.07 KBmartin107
#80 block-2031859-80.patch10 KBjoshk
#74 interdiff.txt2.19 KBdawehner
#74 block-2031859-74.patch15.47 KBdawehner
#72 block-2031859-72.patch15.5 KBdawehner
#69 block_plugin-2158571-68.patch15.69 KBdawehner
#63 interdiff.txt827 bytesdawehner
#63 block_plugin-2031859-63.patch15.72 KBdawehner
#61 interdiff.txt3.87 KBdawehner
#61 block_plugin-2031859-61.patch15.68 KBdawehner
#59 block-2031859-59.patch14.93 KBtim.plunkett
#59 interdiff.txt506 bytestim.plunkett
#57 block-2031859-57.patch14.94 KBdawehner
#49 block-plugin-event-fix-with-tests-2031859-interdiff-44-49.txt5.74 KBhussainweb
#49 block-plugin-event-fix-with-tests-2031859-49.patch22.51 KBhussainweb
#44 block-plugin-event-fix-with-tests-2031859-44.patch19.43 KBhussainweb
#34 block-plugin-event-tests-only-2031859-34.patch3.12 KBhussainweb
#34 block-plugin-event-tests-with-patch-24-2031859-34.patch19.56 KBhussainweb
#34 block-plugin-event-fix-with-tests-2031859-34.patch19.57 KBhussainweb
#28 block-plugin-event-2031859-28.patch16.45 KBhussainweb
#28 block-plugin-event-2031859-interdiff-24-28.txt1.25 KBhussainweb
#24 2031859-24.patch16.44 KBfubhy
#24 interdiff.txt2.1 KBfubhy
#23 drupal-2031859-23.patch16.48 KBdawehner
#21 vdc-2031859-21.patch16.36 KBdawehner
#21 interdiff.txt646 bytesdawehner
#19 drupal-2031859-19.patch16.36 KBdawehner
#19 interdiff.txt6.36 KBdawehner
#15 2031859-15.patch14.76 KBEclipseGc
#15 2031859-interdiff.txt2.89 KBEclipseGc
#11 interdiff.txt7.99 KBdawehner
#11 drupal-2031859-11.patch12.73 KBdawehner
#8 2031859-8.patch11.53 KBEclipseGc
#8 2031859-interdiff.txt10.31 KBEclipseGc
#2 block-2031859-2.patch6.02 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner’s picture

Status: Active » Needs review
Issue tags: +VDC
FileSize
6.02 KB

Here is a first start.

Status: Needs review » Needs work

The last submitted patch, block-2031859-2.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
+++ b/core/modules/block/lib/Drupal/block/BlockPluginEvents.phpundefined
@@ -0,0 +1,29 @@
+   * This could be the case, when something is disabled but not deleted.
+   */
+  const DISAPPEAR_TEMPORARY = 'temporary';
...
+   * This could be the case, when something is disabled but not deleted.
+   */
+  const DISAPPEAR_PERMANENT = 'temporary';

Copypaste?

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -191,6 +194,8 @@ public function remove() {
+    $event = new GenericEvent($plugin_id);
+    \Drupal::service('event_dispatcher')->dispatch(BlockPluginEvents::DISAPPEAR_PERMANENT, $event);
     foreach (entity_load_multiple_by_properties('block', array('plugin' => $plugin_id)) as $block) {
       $block->delete();

Could this be handled by the block storage controller?

sdboyer’s picture

ohhh man, @dawehner made an actual event. i thought a hook would be perfectly sufficient :)

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
@@ -191,6 +194,8 @@ public function remove() {
     foreach (entity_load_multiple_by_properties('block', array('plugin' => $plugin_id)) as $block) {
       $block->delete();

this is the key line that really needs to go away - things on the other side of the event should be doing the deletion.

@tim.plunkett

Could this be handled by the block storage controller?

it could, which is part of what's being done in #2018493: Tests for: FATAL error when a view with a placed block is disabled or removed. however, that's specifically what i'm trying to avoid with this issue. direct use of the block storage controller makes the assumption, true in core but not in princess (or potentially elsewhere in contrib), that the block storage controller is the only thing that writes block instances. that's why we need to decouple these things by putting any use of that storage controller behind a hook/event implementation.

tim.plunkett’s picture

Fair point.
It will be interesting to see how this works along with #2018493: Tests for: FATAL error when a view with a placed block is disabled or removed, especially the removeBlock() method added there.

sdboyer’s picture

@tim.plunkett - yep, that'd have to change, too. i posted this as a followup to that, with the idea that we could just invoke the event from there. it should simplify that plugin implementation, at least - it won't need all that stuff injected. from my fairly quick reading, that seemed doable...

EclipseGc’s picture

FileSize
10.31 KB
11.53 KB

Ok, getting late and brain not working as well as it should be at this point. A few things.

While the BlockManager should obviously be a subscriber so that it can clear cache, any system wanting to utilize block plugins is going to need its own event subscriber class. Since core is using an entity based one, I created the BlockEntityBlockEventSubscriber class. This gets the plugin id when a block is deleted and does its own loop through the blocks by plugin id and removes the relevant ones.

This should be able to replace the various menu_delete, aggregator_feed/category deletes, etc that exist out there. This patch attempts to do that work. We'll see what the bot things of it.

In doing this I noticed that the Feed entity was still using the old config prefix logic, and was even using the old config prefix. Likewise I _THINK_ the language system should have one of these deletes, but I could not find it (at least it had one when I did the initial conversion). Hopefully this patch keeps us moving in the right directions.

Eclipse

Status: Needs review » Needs work

The last submitted patch, 2031859-8.patch, failed testing.

sdboyer’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Core/Entity/Feed.php
@@ -227,13 +228,10 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
+      if (Drupal::moduleHandler()->moduleExists('block')) {

hmm...do we hurt anything by not doing this? if not, let's just dispatch the event unconditionally.

+++ b/core/modules/menu/menu.module
@@ -303,9 +304,10 @@ function menu_menu_predelete(Menu $menu) {
+  if (Drupal::moduleHandler()->moduleExists('block')) {

ibid.

+++ b/core/modules/block/lib/Drupal/block/BlockEntityBlockEventSubscriber.php
@@ -0,0 +1,36 @@
+    foreach (entity_load_multiple_by_properties('block', array('plugin' => $plugin_id)) as $block) {
+      $block->delete();

let's inject the storage controller into this and use that, rather than relying on the global procedural.

+++ b/core/modules/block/lib/Drupal/block/BlockPluginEvents.php
@@ -0,0 +1,30 @@
+  /**
+   * This event is thrown when a block plugin is temporary unavailable.
+   *
+   * This could be the case, when something is disabled but not deleted.
+   */
+  const DISAPPEAR_TEMPORARY = 'temporary';
+
+  /**
+   * This event is thrown when a block plugin is permanently unavailable.
+   *
+   * This could be the case, when a module has been uninstalled or the data
+   * from which a block plugin was derived was deleted.
+   */
+  const DISAPPEAR_PERMANENT = 'permanent';

i'm not thrilled with the constant names. i know "disappear" is the word we used in IRC, but still.

i think TEMPORARILY_UNAVAILABLE and PERMANENTLY_UNAVAILABLE might be better. thoughts?

+++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.php
@@ -37,4 +41,20 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getSubscribedEvents() {
+    $events[BlockPluginEvents::DISAPPEAR_PERMANENT][] = 'blockPluginRemoved';
+    return $events;
+  }
+
+  /**
+   * Reacts on removing a block plugin.
+   */
+  public function blockPluginRemoved(GenericEvent $event) {
+    $this->clearCachedDefinitions();
+  }

just wanted to comment and say that i only realized dawehner's patch did this when talking with EclipseGc later last night, and this is FUCKING BRILLIANT. with a hook, we'd have to have some logic that reaches in to the block manager and clears the cache, and even if that were in block.module it'd be a bit of a weird separation.

but with an event, the block manager gets to be a first-class citizen and take care of its own garbage. it's beautiful.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.73 KB
7.99 KB

The module exists are needed as this constants are provided by block module.

i'm not thrilled with the constant names. i know "disappear" is the word we used in IRC, but still.

i think TEMPORARILY_UNAVAILABLE and PERMANENTLY_UNAVAILABLE might be better. thoughts?

This is also more parallel to the comment above. Damn you can just rename event names in your IDE and it works!

Let's also fix the broken code.

This will be green.

EclipseGc’s picture

I'm a little curious about the specifics of the constants and their values. Do we need something more specific than just "temporary" and "permanent"?

Eclipse

dawehner’s picture

Title: Invoke a hook[s] when something happens that makes a block plugin go away » Invoke an event[s] when something happens that makes a block plugin go away
Issue tags: +PHPUnit

Change title. I don't care at all, as you should never care about the actual values of the constants.

Most of them seem to use strings:

DYNAMIC = 'routing.route_dynamic'
ALTER = 'routing.route_alter'
dawehner’s picture

As constant values are not a public api, we could even change them later.

EclipseGc’s picture

Issue tags: +Needs tests
FileSize
2.89 KB
14.76 KB

ok, after a discussion with dawehner in irc, I think we're roughly on the same page with my constants value comment. I've updated that in this patch to be something a bit more specific.

I found that the language block didn't have any delete logic (that I could find) which is disturbing since I wrote that logic in the initial blocks conversion. In any event, I added what should pass for logic for that under this new patch. We're obviously missing tests...

Likewise, the views issue that spawned this one was also experience by custom blocks. A solution was written for custom blocks, and I've ported it over to this system. Some interface changes had been made to support the custom blocks fix for this problem and that has since been used elsewhere in the system. It still looked perfectly viable to me, so I didn't remove these methods, I just stopped relying on them for the custom block instance delete process.

I THINK this should mostly cover what we're trying to do here, obviously there's the issue with the language block and missing tests, so I'm marking this needs tests for that, otherwise, I think we're really close.

Eclipse

larowlan’s picture

+++ b/core/includes/language.incundefined
@@ -209,6 +211,11 @@ function language_types_disable($types) {
+    // Inform the block plugin system you have removed a language.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Core/Entity/Feed.phpundefined
@@ -227,13 +229,10 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
+      // Inform the block plugin system you have deleted a feed.

+++ b/core/modules/menu/menu.moduleundefined
@@ -303,9 +305,10 @@ function menu_menu_predelete(Menu $menu) {
+  // Inform the block plugin system you have deleted a menu.

What about 'Inform the block plugin system that a menu|feed|language has been deleted'?

+++ b/core/includes/language.incundefined
@@ -209,6 +211,11 @@ function language_types_disable($types) {
+    if (\Drupal::moduleHandler()->moduleExists('block')) {

+++ b/core/modules/menu/menu.moduleundefined
@@ -303,9 +305,10 @@ function menu_menu_predelete(Menu $menu) {
+    \Drupal::service('event_dispatcher')->dispatch(BlockPluginEvents::PERMANENTLY_UNAVAILABLE, $event);

why \Drupal in procedural code? Isn't just Drupal enough?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Core/Entity/Feed.phpundefined
@@ -227,13 +229,10 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
+      if (\Drupal::moduleHandler()->moduleExists('block')) {
...
+        \Drupal::service('event_dispatcher')->dispatch(BlockPluginEvents::PERMANENTLY_UNAVAILABLE, $event);

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.phpundefined
@@ -225,9 +227,8 @@ public function preSaveRevision(EntityStorageControllerInterface $storage_contro
+    \Drupal::service('event_dispatcher')->dispatch(BlockPluginEvents::PERMANENTLY_UNAVAILABLE, $event);

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -191,9 +194,8 @@ public function remove() {
+    \Drupal::service('event_dispatcher')->dispatch(BlockPluginEvents::PERMANENTLY_UNAVAILABLE, $event);

Can we inject moduleHandler and event dispatcher? It's a plugin so we can use ContainerFactory as the factory and the ContainerFactoryPluginInterface. See https://drupal.org/node/2012118

+++ b/core/modules/block/lib/Drupal/block/BlockEntityBlockEventSubscriber.phpundefined
@@ -0,0 +1,55 @@
+    $events[BlockPluginEvents::PERMANENTLY_UNAVAILABLE][] = 'deleteEntityByPlugin';
...
+  public function deleteEntityByPlugin(GenericEvent $event) {

Nice!

+++ b/core/modules/block/lib/Drupal/block/BlockEntityBlockEventSubscriber.phpundefined
@@ -0,0 +1,55 @@
+   * Reacts on removing a block plugin.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.phpundefined
@@ -37,4 +41,20 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
+   * Reacts on removing a block plugin.

Missing @param

+++ b/core/modules/block/lib/Drupal/block/BlockPluginInterface.phpundefined
@@ -102,4 +102,6 @@ public function submit($form, &$form_state);
+

Extra blank line

+++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.phpundefined
@@ -37,4 +41,20 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
+    $this->clearCachedDefinitions();

Nicerer!

+++ b/core/modules/block/tests/lib/Drupal/block_test/Plugin/Type/BlockManagerTest.phpundefined
@@ -0,0 +1,51 @@
+      ->method('clearCachedDefinitions');

Is this the essence of the test? I'm not seeing any asserts?

EclipseGc’s picture

why \Drupal in procedural code? Isn't just Drupal enough?

I'd need a:

use Drupal

statement if we wanted to do that, and I think we've settled on NOT doing that as our standard. (Anyone wanna back me up or debunk me here?)

Can we inject moduleHandler and event dispatcher? It's a plugin so we can use ContainerFactory as the factory and the ContainerFactoryPluginInterface. See https://drupal.org/node/2012118

Since the EntityManager is not responsible for instantiating entities (i.e. they're not full fledged plugins, they just use the discovery) we cannot do that.

Is this the essence of the test? I'm not seeing any asserts?

Ask dawehner :-)

Eclipse

longwave’s picture

You don't need a use statement for a top-level class in procedural code. See e.g. common.inc, which uses several Drupal:: functions but does not have or need use Drupal;

dawehner’s picture

FileSize
6.36 KB
16.36 KB

Can we inject moduleHandler and event dispatcher? It's a plugin so we can use ContainerFactory as the factory and the ContainerFactoryPluginInterface. See https://drupal.org/node/2012118

The problem is that entities are not initialized using the normal plugin factories, so we can't initialize stuff.

Is this the essence of the test? I'm not seeing any asserts?

Yes, the idea is that we check that the method is executed.

EclipseGc’s picture

Why'd you set the language stuff to use the temporary status? We've not built any logic to actually do stuff on that status yet, and we do actually need a test to check if language blocks are being removed properly. ???

Eclipse

dawehner’s picture

FileSize
646 bytes
16.36 KB

As you explained, you would readd the language type.

Status: Needs review » Needs work

The last submitted patch, vdc-2031859-21.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.48 KB

Rerole.

fubhy’s picture

FileSize
2.1 KB
16.44 KB
+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -333,9 +335,8 @@ function aggregator_save_category($edit) {
+        \Drupal::service('event_dispatcher')->dispatch(BlockPluginEvents::DISAPPEAR_PERMANENT, $event);

Redundant leading backslash.

+++ b/core/modules/block/lib/Drupal/block/BlockEntityBlockEventSubscriber.phpundefined
@@ -0,0 +1,55 @@
+    foreach ($this->blockStorageController->loadByProperties(array('plugin' => $plugin_id)) as $block) {
+      $block->delete();
+    }

What about invoking delete() on the block storage controller directly to benefit of the multi-delete functionality there instead?

+++ b/core/modules/menu/menu.moduleundefined
@@ -303,9 +305,11 @@ function menu_menu_predelete(Menu $menu) {
+  // Inform the block plugin system you have deleted a menu.
+  // Inform the block plugin system that a menu has been deleted.

I guess one can go away? :)

fubhy’s picture

RTBC on anything pre-#24

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Nice

hussainweb’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -333,9 +335,8 @@ function aggregator_save_category($edit) {
+        $event = new GenericEvent('aggregator_category_block:' . $edit['cid']);
+        Drupal::service('event_dispatcher')->dispatch(BlockPluginEvents::DISAPPEAR_PERMANENT, $event);

From #10, we decided to shift to PERMANENTLY_UNAVAILABLE. I guess this one got missed.

+++ b/core/modules/block/lib/Drupal/block/BlockPluginEvents.phpundefined
@@ -0,0 +1,30 @@
+/**
+ * Contains events fired when a block plugin instance might disappear.
+ */

If we are avoiding the use of disappear here, should we change this docblock too?

I will try and post a patch for this soon.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
16.45 KB

Here is a patch fixing issues found in #27.

dawehner’s picture

So in other words we need test for that code to run?

hussainweb’s picture

@dawehner - Yes, but I thought it would be out of scope here. Should it be a new issue? I will take a quick look to verify that the function is not being tested anywhere at all.

tim.plunkett’s picture

Tests are never out of scope :)

hussainweb’s picture

Assigned: Unassigned » hussainweb
Status: Needs review » Needs work

I just took a look. The function with the block in question - aggregator_save_category() is called by a single function (form callback) which is tested, but not in the specific conditions to reach the block with the issue in #27. I agree we need tests for this.

Initially, I thought that this test should come through a different issue, as it relates to aggregator.module and this issue is major. But it seems that it is better to do it here? If so, I will give it a shot today over the day. It will be a nice experience for me personally as well.

EclipseGc’s picture

Yes, any time you identify missing tests that an issue exposed, add tests on that issue. Any exception to this would be incredibly rare.

Eclipse

hussainweb’s picture

I am attaching three patch files.

  • The tests-only patch should pass, as the problem was with changes in the patch in #24.
  • The tests-with-patch-24 patch should fail, as that contains the test and the problem.
  • The fix-with-tests patch should pass, as that contains the patch from #28, and the test.

I did a manual test and ran the modified test case and everything was as expected. I hope the testbot concurs.

dawehner’s picture

That is cool, thank you.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/CategorizeFeedTest.phpundefined
@@ -53,5 +64,28 @@ function testCategorizeFeed() {
+    // Delete category 2.
+    $edit = array('title' => $category_2_new_title, 'description' => 'Category renamed from ' . $category_2['title']);
+    $this->drupalPost('admin/config/services/aggregator/edit/category/' . $category_2_record->cid, $edit, t('Delete'));
+    $this->assertRaw(t('The category %title has been deleted.', array('%title' => $category_2_new_title)), format_string('The category %title has been deleted.', array('%title' => $category_2_new_title)));
+
+    // Verify the categories overview page is correctly displayed (after deleting).
+    $this->drupalGet('aggregator/categories');
+    $this->assertText($category_1['title']);
+    $this->assertNoText($category_2['title']);

Should we also add a test to ensure that potential blocks got removed as well?

hussainweb’s picture

I think the blocks were not added by this test at all (CategorizeFeedTest). In fact, I added the dependency to block for this test.

That said, it does make sense to add that test, but I am not too sure. I see there is a similar test in AggregatorRenderingTest. I will try to adapt that here (or modify that test case itself), if it makes sense.

Where do you think this test should go?

hussainweb’s picture

On that note, I think there are some test case for views related functionality in #2018493: Tests for: FATAL error when a view with a placed block is disabled or removed (from where I found this issue in the first place). I think once this patch goes in, we can commit the test cases from #2018493: Tests for: FATAL error when a view with a placed block is disabled or removed.

Should we do the same for aggregator tests and post a follow-up issue, or should we continue it in this thread?

dawehner’s picture

Well, I think we should always add tests if there are none yet, but we touch the actual code.

hussainweb’s picture

I agree. My only question was to do it here or in a follow-up issue (like there is a views block related test in #2018493: Tests for: FATAL error when a view with a placed block is disabled or removed.

dawehner’s picture

Yes it should be done here.

dawehner’s picture

@hussainweb Are you still working on the tests?

hussainweb’s picture

Yes, I was but I am traveling until Wednesday now. If someone can look at it before then, that's great! Otherwise, I will pick this up on Wednesday/Thursday.

benjy’s picture

Status: Needs review » Needs work

Updating status, awaiting tests.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
19.43 KB

I was beginning the work on tests and decided to see if reroll was required. Turns out it was. There was a small conflict with changes from #1957346: Add some settings on the block display to allow overrides on the block instance configuration which is resolved in the attached patch. I will continue with the tests once the testbot find it okay.

Status: Needs review » Needs work
Issue tags: -PHPUnit, -VDC, -happy princess API

The last submitted patch, block-plugin-event-fix-with-tests-2031859-44.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit, +VDC, +happy princess API
hussainweb’s picture

I started working on a test when I found a major bug in aggregator.module in rendering blocks. I have filed an issue at #2057355: Aggregator Category blocks do not render. I think I can continue working on the tests but it won't really work until that issue gets committed first.

hussainweb’s picture

Status: Needs review » Needs work
hussainweb’s picture

I have added the tests but this is still dependent on #2057355: Aggregator Category blocks do not render.

There seems to have been some changes to certain parts as the tests started failing again. Essentially, sending the block PERMANENTLY_UNAVAILABLE event after the item is deleted results in some errors. That is why, I am now sending the events before the entity is deleted.

I also added a similar test for AggregatorRenderingTest (and the corresponding change to send the event before deleting the feed). All up for review.

I suspect the tests will fail now, until #2057355: Aggregator Category blocks do not render gets in.

Status: Needs review » Needs work

The last submitted patch, block-plugin-event-fix-with-tests-2031859-49.patch, failed testing.

hussainweb’s picture

Failure:

Feed Category block category-o6GGIssWSy is displayed on the page.

Just the expected failure! This will pass when #2057355: Aggregator Category blocks do not render gets in.

tim.plunkett’s picture

This could solve most of #2078217: [meta] Write tests for deleting blocks provided by derivatives at once. But those issues all should have tests in them already...

catch’s picture

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.php
@@ -228,9 +230,8 @@ public function preSaveRevision(EntityStorageControllerInterface $storage_contro
+    \Drupal::service('event_dispatcher')->dispatch(BlockPluginEvents::PERMANENTLY_UNAVAILABLE, $event);

While it's only a couple of lines, this is a lot of copy/paste code when the only thing that changes is the block plugin ID.

It also looks very odd to have a bunch of modules manually dispatching an event - if this was a hook we'd not encourage people to put module_invoke_all('block_plugin_deleted') all over the shop. Can we move that to a wrapper which handles the event dispatching itself?

If we're checking for block module here, that provides no way for say panels module to replace block module in Drupal 8 - given that block module is optional it ought to be possible to replace it, but still pick up blocks via its own plugin manager or similar. Right now despite looking like a generic event, it's hard-wired to block module existing via both the class constants and the module_exists() check - might as well just call a function in block module by that point.

Looking at the workarounds in #2078247: Placed views blocks will throw errors if the view is deleted. resolving this overall issue is critical I think, so bumping even though I'm not comfortable with the fix here at the moment.

catch’s picture

Thinking about this more - do we need a generic event when a plugin derivative is removed - that could take the plugin type + ID? I can't see this only being an issue for block module.

catch’s picture

Priority: Major » Critical
dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.94 KB

Here is a reroll.

Status: Needs review » Needs work

The last submitted patch, 57: block-2031859-57.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 59: block-2031859-59.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.68 KB
3.87 KB

Feed module currently uses one block plugin with settings to specify which feed should be displayed,
so you cannot really remove an instance of the block in a really generic way.

One solution could be to get rid of the block and just use views: #2223899: Convert the feed block into a view
Another solution is to provide arbitrary conditions in the event.

Status: Needs review » Needs work

The last submitted patch, 61: block_plugin-2031859-61.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.72 KB
827 bytes

That is not really nice.

Status: Needs review » Needs work

The last submitted patch, 63: block_plugin-2031859-63.patch, failed testing.

aspilicious’s picture

Nice patch. :)

catch’s picture

Issue tags: +beta target
dawehner’s picture

63: block_plugin-2031859-63.patch queued for re-testing.

The last submitted patch, 63: block_plugin-2031859-63.patch, failed testing.

dawehner’s picture

Assigned: hussainweb » Unassigned
Status: Needs work » Needs review
FileSize
15.69 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 69: block_plugin-2158571-68.patch, failed testing.

catch’s picture

Title: Invoke an event[s] when something happens that makes a block plugin go away » Invoke an event[s] when something happens that makes a plugin go away
dawehner’s picture

Status: Needs work » Needs review
FileSize
15.5 KB

Rerolled and fixed the error in the installer (just a bad function signature)

Status: Needs review » Needs work

The last submitted patch, 72: block-2031859-72.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.47 KB
2.19 KB

Some fixes.

Status: Needs review » Needs work

The last submitted patch, 74: block-2031859-74.patch, failed testing.

xjm’s picture

Component: block.module » plugin system

This will affect more than just the Block module.

dawehner’s picture

@xjm
Do you suggest that we have to implement a solution which works for any kind of plugin not just block plugins? Maybe have a quick look at the patch ...
this is coupled 100% to blocks at the moment.

catch’s picture

@dawehner I said the same thing back in #53/54 in September.

martin107’s picture

Issue tags: +Needs reroll

No longer applies, look like a pre-psr4 patch

joshk’s picture

Issue tags: -Needs reroll
FileSize
10 KB

Naive re-roll attached.

martin107’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 80: block-2031859-80.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
15.07 KB

I suspect there was only a few things wrong with #80.

It was using the old psr-0 style naming convention for the new files that it introduces. so my changes are :-
renamed: core/modules/block/lib/Drupal/block/BlockEntityBlockEventSubscriber.php -> core/modules/block/src/BlockEntityBlockEventSubscriber.php
renamed: core/modules/block/lib/Drupal/block/BlockPluginEvents.php -> core/modules/block/src/BlockPluginEvents.php

(#80 no longer applied cleanly so just to be safe I used #74 as my starting point. )

Status: Needs review » Needs work

The last submitted patch, 83: block-2031859-83.patch, failed testing.

martin107’s picture

Regarding the 26 new failings in Drupal\block_content\Tests\BlockContentCreationTest ( with respect to #74 )

Looks to me like this patch may not be upto date with recent changes to recent plugin discovery changes.

This is a stack trace from BlockContentCreationTest.php line 205

 $this->drupalPostForm('block/add/basic', $edit2, t('Save'));

In response the the post I get the following stack trace

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "block_content:1f3c8ba7-5d67-4273-bf05-5c1596288fc4" plugin does not exist. in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 57 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).
Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition([Array], block_content:1f3c8ba7-5d67-4273-bf05-5c1596288fc4, TRUE)
Drupal\Core\Plugin\DefaultPluginManager->getDefinition(block_content:1f3c8ba7-5d67-4273-bf05-5c1596288fc4)
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance(block_content:1f3c8ba7-5d67-4273-bf05-5c1596288fc4, [Array])
Drupal\Component\Plugin\PluginManagerBase->createInstance(block_content:1f3c8ba7-5d67-4273-bf05-5c1596288fc4, [Array])
Drupal\Core\Plugin\DefaultSinglePluginBag->initializePlugin(block_content:1f3c8ba7-5d67-4273-bf05-5c1596288fc4)
Drupal\block\BlockPluginBag->initializePlugin(block_content:1f3c8ba7-5d67-4273-bf05-5c1596288fc4)
Drupal\Component\Plugin\PluginBag->get(block_content:1f3c8ba7-5d67-4273-bf05-5c1596288fc4)
Drupal\block\BlockPluginBag->get(block_content:1f3c8ba7-5d67-4273-bf05-5c1596288fc4)
Drupal\block\Entity\Block->getPlugin()
block_list(sidebar_first)
block_get_blocks_by_region(sidebar_first)
block_page_build([Array])
drupal_prepare_page([Array])
Drupal\Core\Page\DefaultHtmlFragmentRenderer->render(Drupal\Core\Page\HtmlFragment)
Drupal\Core\EventSubscriber\HtmlViewSubscriber->onHtmlFragment(Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent, kernel.view, Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher)
call_user_func([Array], Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent, kernel.view, Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher)
Symfony\Component\EventDispatcher\EventDispatcher->doDispatch([Array], kernel.view, Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch(kernel.view, Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent)
Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(kernel.view, Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Symfony\Component\HttpFoundation\Request, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Symfony\Component\HttpFoundation\Request, 1, TRUE)
Drupal\Core\HttpKernel->handle(Symfony\Component\HttpFoundation\Request, 1, TRUE)
Drupal\Core\DrupalKernel->handle(Symfony\Component\HttpFoundation\Request)
drupal_handle_request()

martin107’s picture

Just nibbling on the edges..

BlockManagerTest.php was broken all the way back in #74, is now green

Simple resolution of namespace issues.

martin107’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 86: block-2031859-86.patch, failed testing.

dawehner’s picture

FileSize
1.13 KB

What do you all think about some method on the plugin manager, maybe even a static one at some point.

dawehner’s picture

FileSize
4.83 KB

Can someone correct the title, this cannot be proper english.

Expanded the work to define a default plugin manager interface.

benjy’s picture

Title: Invoke an event[s] when something happens that makes a plugin go away » Invoke an event[s] when a plugin instance is deleted
yched’s picture

issue title is "Invoke an event[s] when a plugin instance is deleted"
Isn't it rather "... when a plugin ID disappears" ?

dawehner’s picture

Title: Invoke an event[s] when a plugin instance is deleted » Invoke an event[s] when a plugin ID disappears
Status: Needs work » Needs review
FileSize
19.6 KB
16.25 KB

issue title is "Invoke an event[s] when a plugin instance is deleted"
Isn't it rather "... when a plugin ID disappears" ?

Sure ...

Expanded the patch in #90 with the different places of removing blocks in #86.

Status: Needs review » Needs work

The last submitted patch, 93: 2031859-plugins-93.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
19.59 KB
725 bytes

Fixed that failure.

Status: Needs review » Needs work

The last submitted patch, 95: 2031859-plugins-95.patch, failed testing.

yched’s picture

Does it mean we expect modules that provide plugins to manually call removePluginId() on uninstall, or is that just required for dynamic plugins / derivatives ?

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

Going to have a look at the failing tests.

pfrenssen’s picture

Issue tags: +Drupalaton 2014
pfrenssen’s picture

@yched, for the sake of consistency I would call removePluginId() also when modules that provide plugins are uninstalled. It would be nice if this could be done automatically, rather than require modules to call this themselves.

Edit: changed my mind.

dawehner’s picture

@pfrenssen
In case you made some progress here, feel free to post it at anytime. Afaik you worked on fixing some failures.

pfrenssen’s picture

I'll post my progress when I get home tonight.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
22.48 KB
15.33 KB

Here's my iteration. I introduced a new PluginRemovalEvent instead of relying on the default GenericEvent, changed BlockPluginEvents to PluginRemovalEvents and updated other places where blocks were referred instead of generic plugins.

dawehner’s picture

FileSize
117.74 KB
100.38 KB

In order to be able to properly just remove the plugins you really want to remove we need to introduce some concept of plugin types.

  1. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
    @@ -241,4 +224,14 @@ protected function findDefinitions() {
    +  public function removePluginId($plugin_id, $permanent = TRUE, array $conditions = array()) {
    +    /** @var \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher */
    +    $event_dispatcher = \Drupal::service('event_dispatcher');
    

    I guess we want to properly inject that service as well :( This for sure requires a non-trivial amount of work. [Done] (hopefully)

  2. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManagerInterface.php
    @@ -0,0 +1,58 @@
    +   * Announces that the plugin with the specified ID disappears.
    +   *
    +   * The plugin might be removed permanently, for example when something on
    +   * which it depends is deleted, or it might be removed temporarily, for
    +   * example when something is disabled but might be re-enabled in the future.
    +   *
    

    I guess bringing on an actual example like: removed custom block would make that better to understand.

  3. +++ b/core/lib/Drupal/Core/Plugin/Event/PluginRemovalEvent.php
    @@ -0,0 +1,87 @@
    +class PluginRemovalEvent extends Event {
    

    I think it is fine to not include setters here, there isn't really a usecase for it.

  4. +++ b/core/lib/Drupal/Core/Plugin/Event/PluginRemovalEvent.php
    @@ -0,0 +1,87 @@
    +  private $pluginId;
    ...
    +  private $permanent;
    ...
    +  private $conditions;
    

    Let's use protected to be more like drupal.

  5. +++ b/core/lib/Drupal/Core/Plugin/Event/PluginRemovalEvent.php
    @@ -0,0 +1,87 @@
    +   * @var bool
    ...
    +   * @return boolean
    

    bool or boolean afaik it is bool

Status: Needs review » Needs work

The last submitted patch, 104: plugin_manager-2031859-104.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
757 bytes
118.48 KB

A little fix

Status: Needs review » Needs work

The last submitted patch, 106: plugin_manager-2031859-106.patch, failed testing.

martin107’s picture

Assigned: Unassigned » martin107
FileSize
118.49 KB

Mini-commit spree overnight, means the patch needs a reroll, no conflicts just auto-merging and 3-way merging.

file diff shows a small change.

<   * In-place editor manager.
< @@ -28,9 +29,11 @@ class InPlaceEditorManager extends DefaultPluginManager {
---
>   * Provides an in-place editor manager.
> @@ -33,9 +34,11 @@ class InPlaceEditorManager extends DefaultPluginManager {

I can see a similar fix to #106 so I will grab the issue until its is fixed.

martin107’s picture

Status: Needs work » Needs review
martin107’s picture

Assigned: martin107 » Unassigned
FileSize
119.35 KB
878 bytes

The next obvious step :-

Fixup \Drupal\condition_test\FormController::_construct

The last submitted patch, 108: plugin_manager-2031859-108.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 110: plugin_manager-2031859-110.patch, failed testing.

martin107’s picture

Hmm, so missing a few use statements ?

use DependencySerializationTrait;

dawehner’s picture

Status: Needs work » Needs review
FileSize
120.56 KB
1.96 KB

@martin107
Thank you!

Let's fix the failures.

Status: Needs review » Needs work

The last submitted patch, 114: plugin_manager-2031859-114.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
111.58 KB
111.58 KB

Green!

Status: Needs review » Needs work

The last submitted patch, 116: plugin_manager-2031859-116.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
122.4 KB

Missing files.

martin107’s picture

FileSize
1.84 KB

I am following along ... here is the interdiff for reference

Yay green

dawehner’s picture

Reroll + some "minor" correction.

dawehner’s picture

Rerolled, wow that was hard.

Implemented the feature of automatic calling when a plugin got removed, as we can apply some diff during cache clearing.

Status: Needs review » Needs work

The last submitted patch, 121: plugin_manager-2031859-121.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
131.19 KB
800 bytes

Let's fix it.

EclipseGc’s picture

I really love seeing the old cache vs new definitions check and invoking this event based on that. Total ++

Eclipse

dawehner’s picture

Issue summary: View changes

Adapted the IS. @EclipseGc So was that a RTBC?

xjm’s picture

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -17,14 +17,16 @@
-class DefaultPluginManager extends PluginManagerBase implements PluginManagerInterface, CachedDiscoveryInterface {
+class DefaultPluginManager extends PluginManagerBase implements PluginManagerInterface, CachedDiscoveryInterface, DefaultPluginManagerInterface {

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManagerInterface.php
@@ -0,0 +1,61 @@
+interface DefaultPluginManagerInterface extends PluginManagerInterface {

IF DefaultPluginManagerInterface extends PluginManagerInterface, then why does the DefaultPluginManager need to implement both?

dawehner’s picture

IF DefaultPluginManagerInterface extends PluginManagerInterface, then why does the DefaultPluginManager need to implement both?

You could indeed ask the same question with PluginManagerBase, good catch!

EclipseGc’s picture

So, BlockManager looks completely wrong to my eyes. After discussing this in IRC, I think there's quite a bit left to do. I'll try to give a more thorough review tomorrow.

Eclipse

EclipseGc’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Block/BlockManager.php
    @@ -111,4 +117,22 @@ public function getSortedDefinitions() {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function getSubscribedEvents() {
    +    $events[PluginRemovalEvents::PERMANENTLY_UNAVAILABLE][] = 'blockPluginRemoved';
    +    return $events;
    +  }
    +
    +  /**
    +   * Reacts on removing a block plugin.
    +   *
    +   * @param \Drupal\Core\Plugin\Event\PluginRemovalEvent $event
    +   *   The event containing the plugin ID which is updated.
    +   */
    +  public function blockPluginRemoved(PluginRemovalEvent $event) {
    +    $this->clearCachedDefinitions();
    +  }
    +
    

    So managers shouldn't have to subscribe to an event to find out that one of their plugins has been removed, they should be the hub of that action.

  2. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
    @@ -166,6 +161,15 @@ public function clearCachedDefinitions() {
    +
    +    if ($existing_definitions = $this->getCachedDefinitions()) {
    +      $new_definitions = $this->findDefinitions();
    +      $removed_plugin_ids = array_diff(array_keys($existing_definitions), array_keys($new_definitions));
    +      foreach ($removed_plugin_ids as $removed_plugin_id) {
    +        $this->removePluginId($removed_plugin_id);
    +      }
    +    }
    +
    
    @@ -241,4 +245,14 @@ protected function findDefinitions() {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function removePluginId($plugin_id, $permanent = TRUE, array $conditions = array()) {
    +    $event_name = $permanent ? PluginRemovalEvents::PERMANENTLY_UNAVAILABLE : PluginRemovalEvents::TEMPORARILY_UNAVAILABLE;
    +    // @todo Introduce a proper plugin type.
    +    $plugin_type = isset($this->subdir) ? strtolower(str_replace('/', '_', $this->subdir)) : get_class($this);
    +    $this->eventDispatcher->dispatch($event_name, new PluginRemovalEvent($plugin_type, $plugin_id, $permanent, $conditions));
    +  }
    +
    

    These two blocks look super sane to me. I can't imagine needing more. dawehner and I have discussed this at length and have different view points, I'm just expressing mine.

  3. +++ b/core/lib/Drupal/Core/Plugin/PluginRemovalEvents.php
    @@ -0,0 +1,30 @@
    +  const TEMPORARILY_UNAVAILABLE = 'plugin.unavailable.temporarily';
    

    What's the usecase for this?

  4. +++ b/core/modules/aggregator/src/Entity/Feed.php
    @@ -112,15 +112,12 @@ public static function preDelete(EntityStorageInterface $storage, array $entitie
    +        $block_manager->removePluginId('aggregator_feed_block', FALSE, array('settings.feed' => $entity->id()));
    

    I'm actually wondering W.T.F. here. Why is this identified by some arcane settings.feed configuration in the plugin? This should absolutely be at the plugin_id layer... This might explain some of the disagreement we've had.

  5. +++ b/core/modules/block/src/BlockEntityBlockEventSubscriber.php
    @@ -0,0 +1,76 @@
    +    if ($blocks = $this->blockStorageController->loadMultiple($result)) {
    +      $this->blockStorageController->delete($blocks);
    +    }
    

    Is this safe to do? Shouldn't we log the else result of this at the bare minimum?

  6. +++ b/core/modules/system/src/Entity/Menu.php
    @@ -64,4 +64,17 @@ public function isLocked() {
    +    foreach ($entities as $entity) {
    +      /** @var \Drupal\block\BlockManagerInterface $block_manager */
    +      $block_manager = \Drupal::service('plugin.manager.block');
    +      $block_manager->removePluginId('menu_menu_block:' . $entity->id());
    +    }
    

    This is exactly what I expected the feeds implementation to look like and is why I've been pushing for cache clearing as our methodology here since the derivative handler will flag all the old vs new ones. Your conditions array on removePluginId() is where this gets interesting.

All in all, I think I understand your position a little better after reading this. Let's talk more soon in irc and see if we can find the right middle ground.

Eclipse

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

I'm going to have a look.

pfrenssen’s picture

Status: Needs work » Needs review
pfrenssen’s picture

  • BlockManager no longer subscribes to the plugin removal event itself. This addresses remark #1 from comment #129.
  • Removed the distinction between a temporary removal and a permanent removal. This addresses #3 from #129.
  • Renamed PluginRemovalEvents to PluginEvents, this is more inline with how other events are defined in core.
  • Renamed BlockManager::removePluginId() to BlockManager::removePlugin(). This method name was rubbing me the wrong way, we're removing plugins, not plugin IDs.
  • Some minor documentation fixes.

Status: Needs review » Needs work

The last submitted patch, 133: plugin_manager-2031859-133.patch, failed testing.

pfrenssen’s picture

Answering some questions from #129:

+++ b/core/modules/aggregator/src/Entity/Feed.php
@@ -112,15 +112,12 @@ public static function preDelete(EntityStorageInterface $storage, array $entitie
+        $block_manager->removePluginId('aggregator_feed_block', FALSE, array('settings.feed' => $entity->id()));

I'm actually wondering W.T.F. here. Why is this identified by some arcane settings.feed configuration in the plugin? This should absolutely be at the plugin_id layer... This might explain some of the disagreement we've had.

+++ b/core/modules/system/src/Entity/Menu.php
@@ -64,4 +64,17 @@ public function isLocked() {
+    foreach ($entities as $entity) {
+      /** @var \Drupal\block\BlockManagerInterface $block_manager */
+      $block_manager = \Drupal::service('plugin.manager.block');
+      $block_manager->removePluginId('menu_menu_block:' . $entity->id());
+    }

This is exactly what I expected the feeds implementation to look like and is why I've been pushing for cache clearing as our methodology here since the derivative handler will flag all the old vs new ones. Your conditions array on removePluginId() is where this gets interesting.

I totally agree that this doesn't look like the right way to do this, however reworking how this is handled in the Aggegator module is out of scope for this issue.

+++ b/core/modules/block/src/BlockEntityBlockEventSubscriber.php
@@ -0,0 +1,76 @@
+    if ($blocks = $this->blockStorageController->loadMultiple($result)) {
+      $this->blockStorageController->delete($blocks);
+    }

Is this safe to do? Shouldn't we log the else result of this at the bare minimum?

Yes it's safe, if no blocks match the conditions then we don't need to delete them.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned

Fixed failures and a small documentation update. Unassigning myself for the moment until we have a consensus on the way forward with this issue.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
130.75 KB
1.4 KB

Status: Needs review » Needs work

The last submitted patch, 137: plugin_manager-2031859-136.patch, failed testing.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
129.31 KB
1.58 KB

The BlockManagerTest was testing the use case of the BlockManager that was subscribed to the PluginRemovalEvent to clear the cache. This is no longer used so we no longer need to test it.

mgifford’s picture

tim.plunkett’s picture

That's this issue.

mgifford’s picture

Drat.. Annoying copy/paste error. Not sure how to find the related issues that point to this one that aren't explicitly defined in the "Related issues".

Thanks for catching that.

mgifford’s picture

Letharion’s picture

Rerolled patch to fix conflicts. No interdiff.
The syntax is now sadly invalid, as we now have

+use Symfony\Component\EventDispatcher\EventDispatcherInterface;
+use Symfony\Component\Validator\ValidatorInterface;
use Symfony\Component\Validator\Validation;
use Symfony\Component\Validator\Validator\ValidatorInterface;

And thus two ValidatorInterface. I'll see if I can work out what to do about this.

hussainweb’s picture

@Letharion: The latter ValidatorInterface is the new one from Symfony 2.5. This was modified in #2278353: Update to Symfony 2.5. I would suggest you to keep it that way. You only need the EventDispatcherInterface anyway.

Letharion’s picture

Actual file

Letharion’s picture

@hussainweb, thanks.

Dropped the extra validator interface.

The last submitted patch, 147: plugin_manager-2031859-145.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 148: plugin_manager-2031859-148.patch, failed testing.

Letharion’s picture

FileSize
129.48 KB

Let's try another patch where I do less stupid things to break it before uploading it.

mgifford’s picture

Status: Needs work » Needs review
Letharion’s picture

I talked to Kris and Daniel in Amsterdam, and we (or rather they) agreed that the way forward is to rewrite the weird aggregator special case by making aggregator feeds a context of their own.

Status: Needs review » Needs work

The last submitted patch, 151: plugin_manager-2031859-150.patch, failed testing.

Letharion’s picture

lol at 10000 failed tests.
Tests say "SQLSTATE[08004] [1040] Too many connections" though, so I don't think it's my fault.

Letharion’s picture

Status: Needs work » Needs review

EclipseGc’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Block/BlockManager.php
    @@ -121,4 +124,12 @@ public function getFallbackPluginId($plugin_id, array $configuration = array())
    +  public function removePlugin($plugin_id, array $conditions = array()) {
    

    I see no reason for these conditions. A plugin id is either available or its not. There's no "available if" situation.

  2. +++ b/core/lib/Drupal/Core/EventDispatcher/ContainerAwareEventDispatcher.php
    @@ -0,0 +1,21 @@
    +<?php
    +
    +/**
    + * @file
    + * Contains \Drupal\Core\EventDispatcher\ContainerAwareEventDispatcher.
    + */
    +
    +namespace Drupal\Core\EventDispatcher;
    +
    +use Drupal\Core\DependencyInjection\DependencySerializationTrait;
    +use Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher as BaseContainerAwareEventDispatcher;
    +
    +/**
    + * Extends the base event dispatcher to be serializable.
    + */
    +class ContainerAwareEventDispatcher extends BaseContainerAwareEventDispatcher {
    +
    +  use DependencySerializationTrait;
    +
    +}
    +
    

    Explain why this was necessary for me?

  3. +++ b/core/modules/aggregator/src/Entity/Feed.php
    @@ -112,15 +112,12 @@ public static function preDelete(EntityStorageInterface $storage, array $entitie
    +
    +    // Make sure there are no active blocks for these feeds.
    +    /** @var \Drupal\Core\Plugin\DefaultPluginManagerInterface $block_manager */
    +    if (\Drupal::hasService('plugin.manager.block') && $block_manager = \Drupal::service('plugin.manager.block')) {
    +      foreach ($entities as $entity) {
    +        $block_manager->removePlugin('aggregator_feed_block', array('settings.feed' => $entity->id()));
    

    This is the very code I have problems with in this whole thing. This isn't removing a plugin id, it's removing plugin instances based upon their configuration. That's a COMPLETELY different thing. This code block has go to go. If we need to write another patch that fixes this first, then we have to do that.

  4. +++ b/core/modules/block/src/BlockEntityBlockEventSubscriber.php
    @@ -0,0 +1,78 @@
    +class BlockEntityBlockEventSubscriber implements EventSubscriberInterface {
    

    Again, the condition section of this is unnecessary. We can just remove by id.

Letharion’s picture

> I see no reason for these conditions. A plugin id is either available or its not. There's no "available if" situation.

The conditions were added back between #90 and #93, not sure why.
The comment says that they are for allowing subscribers to decide "how to react on the plugin removal". It's not clear to me what the usecase is for that, so I've rerolled the patch without them. Lets see what happens.

Because we now have a ContainerAwareEventDispatcher in core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php, I've left this one out, as I'm not sure what it's doing. If the tests break, or someone yells, I'll add it back in. :)

Letharion’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 159: plugin_manager-2031859-159.patch, failed testing.

dawehner’s picture

What a pitty, what a sad story we have in this issue. After we had once a working patch, which solved proper usecases we had in core, we derailed the issue
and now just have to wait.

dawehner’s picture

Status: Needs work » Needs review
FileSize
128.14 KB

Let's start to get this fixed after a strict reroll.

Status: Needs review » Needs work

The last submitted patch, 163: 2031859-163.patch, failed testing.

EclipseGc’s picture

dawehner,

Hey, so I have a couple of issues I'm pursuing right now trying to unblock the problems I had with this issue. I'd really love to chat some this week and outline what I'm trying to do to clear a path for this specific issue. I'll ping when I'm on irc next and if we can agree on it, I'll file issues so that we have a clear road map.

Eclipse

dawehner’s picture

In general I really wonder whether we need this issue at all.

Config entities now can have in theory dependencies to other config/content entities/modules, so if all plugins store those information on the main config entity,
Config entities itself can react on it, see EntityDisplayBase::onDependencyRemoval(), so from my point of view we no longer need this issue.

alexpott’s picture

Well we certainly have solved the problem outlined in the summary with fallback plugin ids and EntityDisplayBase::onDependencyRemoval()

EclipseGc’s picture

That's not as direct as saying "plugin of id x was removed". I still think subscribing to the Plugin's knowledge instead of trying to source a bunch of different (perhaps unknowable) entity types is the preferable solution. I've not forgotten about this, but I had to square away Aggregator block first, which I'm trying to do with #2377757: Expose Block Context mapping in the UI, since it was confusing this issue.

I don't think we should abandon this issue.

Eclipse

dawehner’s picture

Well, all the cases in which things are broken are handled kind of by different subsystems.
In general I would rather consider this issue as major then, because you don't really need as there are alternative ways to fix
the existing problem.

catch’s picture

Priority: Critical » Major

Yes I think this is worth doing, but configuration entity dependencies have all the use cases handled at the moment, so the practical problem is solved, just in a CMI centric as opposed to plugin centric way.

Bumping down to major since as long as all the use cases are covered we can add this post 8.0.0 and convert core systems to use it in minor releases. If it happens before release candidate and cleans things up that's great as well.

Xano’s picture

aspilicious’s picture

I have a real life use case where I need those events.

While porting the prod_check module to Drupal 8 I converted the "production checks" to plugins.
Suddenly I realised I needed to store settings. In order to follow core patterns I created a config entity, connected to the plugin.
I "stole" most of the code from the action module.

Those "Production Check Plugins" have a one on one relationship with a "production check config entity". In order to synchronise both I created an ugly hack in the 8.x-1.x branch of the module. See the last two functions: http://cgit.drupalcode.org/prod_check/tree/prod_check.module

In order to remove that hack, I need some kind of event that notifies me when a plugin gets added/deleted. Which sounds similar to this issue.
I looked at the patch but it's a scary one at this moment as it changes all the plugin managers.

Is there a way to have an event system without passing the service to the constructor?

Xano’s picture

The way core and contrib have been dealing with this so far, is to let plugin factories (and therefore managers) return fallback plugin instances for plugins that no longer exist. This does not, however, work when calling DiscoveryInterface::getDefinition(). Calling that for an unknown plugin still fails (return NULL or throws an exception).

Let's summarize what we have:

  • Currently calling code only knows about plugins through plugin managers' discovery. Unless you know how a plugin manager works internally, you don't know how plugins were discovered, and when, and whether anything changed since the previous time plugins were discovered.
  • Plugin managers, with possibly few exceptions, cache definitions. Cached can be wiped without notice, meaning that whenever plugins are discovered, there may or may not be any information about previously discovered plugins, even within plugin managers themselves.
  • Plugins can specify config entities as their dependencies using DependentPluginInterface, but plugins can never be specified as dependencies anywhere in the system. This is partly caused by the fact that Drupal core provides no way to discover plugin types (Plugin provides this functionality instead).

The first step towards providing any kind of dependent plugin management is to make plugin managers keep a persistent copy of the discovered plugins until after a new discovery round has taken place, so the old and new definitions can be compared, in order to fire events. Since caches are not meant to be persistent, we would need to use something like state.

The lowest level we can add this to is DefaultPluginManager, since PluginManagerBase is part of \Drupal\Component and can therefore not depend on state, which is part of \Drupal\Core.

Ideally we'd inject state and the event dispatcher into DefaultPluginManager through the constructor, but seeing as that will break most child classes, we will probably have to resolve to service location from within DefaultPluginManager. It's nasty, but otherwise we'll break child classes (constructors are not APIs, but at this point in the release cycle we cannot break non-API code without a VERY good reason).

The actual logic could then be added to DefaultPluginManager::findDefinitions().

xjm’s picture

Version: 8.0.x-dev » 8.2.x-dev
Issue tags: -beta target

This issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.

As an API addition, it should be targeted for 8.2.x at this point.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

darvanen’s picture

Would this issue be the reason that when switching between branches with different configuration and running an import, I get the plugin does not exist error?

I would happily work on this but might need a bit of guidance.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.