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.

Files: 
CommentFileSizeAuthor
#163 2031859-163.patch128.14 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,028 pass(es), 722 fail(s), and 128 exception(s).
[ View ]
#159 plugin_manager-2031859-159.patch128.07 KBLetharion
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,821 pass(es), 18 fail(s), and 159 exception(s).
[ View ]
#151 plugin_manager-2031859-150.patch129.48 KBLetharion
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,871 pass(es).
[ View ]
#140 interdiff.txt1.58 KBpfrenssen
#140 plugin_manager-2031859-140.patch129.31 KBpfrenssen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,735 pass(es).
[ View ]

Comments

dawehner’s picture

dawehner’s picture

Status:Active» Needs review
Issue tags:+VDC
StatusFileSize
new6.02 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found.
[ View ]

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

StatusFileSize
new10.31 KB
new11.53 KB
FAILED: [[SimpleTest]]: [MySQL] 56,661 pass(es), 29 fail(s), and 0 exception(s).
[ View ]

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
StatusFileSize
new12.73 KB
PASSED: [[SimpleTest]]: [MySQL] 56,616 pass(es).
[ View ]
new7.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
StatusFileSize
new2.89 KB
new14.76 KB
PASSED: [[SimpleTest]]: [MySQL] 56,561 pass(es).
[ View ]

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:

<?php
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

StatusFileSize
new6.36 KB
new16.36 KB
PASSED: [[SimpleTest]]: [MySQL] 56,784 pass(es).
[ View ]

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

StatusFileSize
new646 bytes
new16.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vdc-2031859-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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
StatusFileSize
new16.48 KB
PASSED: [[SimpleTest]]: [MySQL] 56,688 pass(es).
[ View ]

Rerole.

fubhy’s picture

StatusFileSize
new2.1 KB
new16.44 KB
PASSED: [[SimpleTest]]: [MySQL] 56,539 pass(es).
[ View ]
+++ 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
StatusFileSize
new1.25 KB
new16.45 KB
PASSED: [[SimpleTest]]: [MySQL] 56,924 pass(es).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new19.57 KB
PASSED: [[SimpleTest]]: [MySQL] 57,059 pass(es).
[ View ]
new19.56 KB
FAILED: [[SimpleTest]]: [MySQL] 57,051 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.12 KB
PASSED: [[SimpleTest]]: [MySQL] 57,038 pass(es).
[ View ]

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
StatusFileSize
new19.43 KB
PASSED: [[SimpleTest]]: [MySQL] 57,874 pass(es).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new22.51 KB
FAILED: [[SimpleTest]]: [MySQL] 57,958 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new5.74 KB

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
StatusFileSize
new14.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Here is a reroll.

Status:Needs review» Needs work

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

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new506 bytes
new14.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,379 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Just fixing the service ID

Status:Needs review» Needs work

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

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new15.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,578 pass(es), 8 fail(s), and 4 exception(s).
[ View ]
new3.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
StatusFileSize
new15.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch block_plugin-2031859-63.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new827 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
StatusFileSize
new15.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

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
StatusFileSize
new15.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,135 pass(es), 61 fail(s), and 1 exception(s).
[ View ]

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
StatusFileSize
new15.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,205 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.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
StatusFileSize
new10 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

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
StatusFileSize
new15.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,498 pass(es), 28 fail(s), and 2 exception(s).
[ View ]

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

StatusFileSize
new1.55 KB
new14.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,479 pass(es), 14 fail(s), and 1 exception(s).
[ View ]

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

StatusFileSize
new1.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

StatusFileSize
new4.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,562 pass(es).
[ View ]

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
StatusFileSize
new19.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new16.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
StatusFileSize
new19.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,140 pass(es), 21 fail(s), and 9 exception(s).
[ View ]
new725 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
StatusFileSize
new22.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,776 pass(es).
[ View ]
new15.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

StatusFileSize
new117.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,153 pass(es), 3,796 fail(s), and 5,696 exception(s).
[ View ]
new100.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
StatusFileSize
new757 bytes
new118.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,567 pass(es), 9 fail(s), and 135 exception(s).
[ View ]

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
StatusFileSize
new118.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,544 pass(es), 146 fail(s), and 175 exception(s).
[ View ]

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
StatusFileSize
new119.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,777 pass(es), 1 fail(s), and 127 exception(s).
[ View ]
new878 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
StatusFileSize
new120.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,763 pass(es), 0 fail(s), and 10 exception(s).
[ View ]
new1.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
StatusFileSize
new111.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new111.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
StatusFileSize
new122.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,700 pass(es).
[ View ]

Missing files.

martin107’s picture

StatusFileSize
new1.84 KB

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

Yay green

dawehner’s picture

StatusFileSize
new117.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,208 pass(es).
[ View ]
new1.41 KB

Reroll + some "minor" correction.

dawehner’s picture

StatusFileSize
new130.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,180 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new12.89 KB

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
StatusFileSize
new131.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,195 pass(es).
[ View ]
new800 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

StatusFileSize
new131.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,629 pass(es).
[ View ]
new5.92 KB

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

StatusFileSize
new133.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,715 pass(es).
[ View ]
new14.61 KB

Rerolled against HEAD.

pfrenssen’s picture

Status:Needs work» Needs review
pfrenssen’s picture

StatusFileSize
new15 KB
new130.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,681 pass(es), 18 fail(s), and 23 exception(s).
[ View ]
  • 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
StatusFileSize
new130.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,710 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.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
StatusFileSize
new129.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,735 pass(es).
[ View ]
new1.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

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

StatusFileSize
new129.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/TypedData/TypedDataManager.php.
[ View ]

Actual file

Letharion’s picture

StatusFileSize
new129.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

@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

StatusFileSize
new129.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,871 pass(es).
[ View ]

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

StatusFileSize
new128.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,821 pass(es), 18 fail(s), and 159 exception(s).
[ View ]

> 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
StatusFileSize
new128.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,028 pass(es), 722 fail(s), and 128 exception(s).
[ View ]

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