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.
Comment | File | Size | Author |
---|---|---|---|
#163 | 2031859-163.patch | 128.14 KB | dawehner |
#159 | plugin_manager-2031859-159.patch | 128.07 KB | Letharion |
#151 | plugin_manager-2031859-150.patch | 129.48 KB | Letharion |
#140 | interdiff.txt | 1.58 KB | pfrenssen |
#140 | plugin_manager-2031859-140.patch | 129.31 KB | pfrenssen |
Comments
Comment #1
dawehner#2031855: Throw an event which indicates that a block plugin might be temporary/permanent not available contains some more information for usecases.
Comment #2
dawehnerHere is a first start.
Comment #4
tim.plunkettCopypaste?
Could this be handled by the block storage controller?
Comment #5
sdboyer CreditAttribution: sdboyer commentedohhh man, @dawehner made an actual event. i thought a hook would be perfectly sufficient :)
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
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.
Comment #6
tim.plunkettFair 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.
Comment #7
sdboyer CreditAttribution: sdboyer commented@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...
Comment #8
EclipseGc CreditAttribution: EclipseGc commentedOk, 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
Comment #10
sdboyer CreditAttribution: sdboyer commentedhmm...do we hurt anything by not doing this? if not, let's just dispatch the event unconditionally.
ibid.
let's inject the storage controller into this and use that, rather than relying on the global procedural.
i'm not thrilled with the constant names. i know "disappear" is the word we used in IRC, but still.
i think
TEMPORARILY_UNAVAILABLE
andPERMANENTLY_UNAVAILABLE
might be better. thoughts?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.
Comment #11
dawehnerThe module exists are needed as this constants are provided by block module.
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.
Comment #12
EclipseGc CreditAttribution: EclipseGc commentedI'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
Comment #13
dawehnerChange 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:
Comment #14
dawehnerAs constant values are not a public api, we could even change them later.
Comment #15
EclipseGc CreditAttribution: EclipseGc commentedok, 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
Comment #16
larowlanWhat about 'Inform the block plugin system that a menu|feed|language has been deleted'?
why \Drupal in procedural code? Isn't just Drupal enough?
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
Nice!
Missing @param
Extra blank line
Nicerer!
Is this the essence of the test? I'm not seeing any asserts?
Comment #17
EclipseGc CreditAttribution: EclipseGc commentedI'd need a:
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?)
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.
Ask dawehner :-)
Eclipse
Comment #18
longwaveYou 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;
Comment #19
dawehnerThe problem is that entities are not initialized using the normal plugin factories, so we can't initialize stuff.
Yes, the idea is that we check that the method is executed.
Comment #20
EclipseGc CreditAttribution: EclipseGc commentedWhy'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
Comment #21
dawehnerAs you explained, you would readd the language type.
Comment #23
dawehnerRerole.
Comment #24
fubhy CreditAttribution: fubhy commentedRedundant leading backslash.
What about invoking delete() on the block storage controller directly to benefit of the multi-delete functionality there instead?
I guess one can go away? :)
Comment #25
fubhy CreditAttribution: fubhy commentedRTBC on anything pre-#24
Comment #26
dawehnerNice
Comment #27
hussainwebFrom #10, we decided to shift to PERMANENTLY_UNAVAILABLE. I guess this one got missed.
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.
Comment #28
hussainwebHere is a patch fixing issues found in #27.
Comment #29
dawehnerSo in other words we need test for that code to run?
Comment #30
hussainweb@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.
Comment #31
tim.plunkettTests are never out of scope :)
Comment #32
hussainwebI 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.
Comment #33
EclipseGc CreditAttribution: EclipseGc commentedYes, any time you identify missing tests that an issue exposed, add tests on that issue. Any exception to this would be incredibly rare.
Eclipse
Comment #34
hussainwebI am attaching three patch files.
I did a manual test and ran the modified test case and everything was as expected. I hope the testbot concurs.
Comment #35
dawehnerThat is cool, thank you.
Should we also add a test to ensure that potential blocks got removed as well?
Comment #36
hussainwebI 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?
Comment #37
hussainwebOn 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?
Comment #38
dawehnerWell, I think we should always add tests if there are none yet, but we touch the actual code.
Comment #39
hussainwebI 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.
Comment #40
dawehnerYes it should be done here.
Comment #41
dawehner@hussainweb Are you still working on the tests?
Comment #42
hussainwebYes, 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.
Comment #43
benjy CreditAttribution: benjy commentedUpdating status, awaiting tests.
Comment #44
hussainwebI 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.
Comment #46
hussainweb#44: block-plugin-event-fix-with-tests-2031859-44.patch queued for re-testing.
Comment #47
hussainwebI 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.
Comment #48
hussainwebComment #49
hussainwebI 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.
Comment #51
hussainwebFailure:
Just the expected failure! This will pass when #2057355: Aggregator Category blocks do not render gets in.
Comment #52
tim.plunkettThis 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...
Comment #53
catchWhile 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.
Comment #54
catchThinking 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.
Comment #55
catchSince this is required to fix #2018493: Tests for: FATAL error when a view with a placed block is disabled or removed bumping to critical.
Comment #56
dawehnerGood point: https://drupal.org/node/2086077/revisions/view/2836067/2836155
Comment #57
dawehnerHere is a reroll.
Comment #59
tim.plunkettJust fixing the service ID
Comment #61
dawehnerFeed 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.
Comment #63
dawehnerThat is not really nice.
Comment #65
aspilicious CreditAttribution: aspilicious commentedNice patch. :)
Comment #66
catchComment #67
dawehner63: block_plugin-2031859-63.patch queued for re-testing.
Comment #69
dawehnerReroll.
Comment #71
catchComment #72
dawehnerRerolled and fixed the error in the installer (just a bad function signature)
Comment #74
dawehnerSome fixes.
Comment #76
xjmThis will affect more than just the Block module.
Comment #77
dawehner@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.
Comment #78
catch@dawehner I said the same thing back in #53/54 in September.
Comment #79
martin107 CreditAttribution: martin107 commentedNo longer applies, look like a pre-psr4 patch
Comment #80
joshk CreditAttribution: joshk commentedNaive re-roll attached.
Comment #81
martin107 CreditAttribution: martin107 commentedComment #83
martin107 CreditAttribution: martin107 commentedI 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. )
Comment #85
martin107 CreditAttribution: martin107 commentedRegarding 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
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()
Comment #86
martin107 CreditAttribution: martin107 commentedJust nibbling on the edges..
BlockManagerTest.php was broken all the way back in #74, is now green
Simple resolution of namespace issues.
Comment #87
martin107 CreditAttribution: martin107 commentedComment #89
dawehnerWhat do you all think about some method on the plugin manager, maybe even a static one at some point.
Comment #90
dawehnerCan someone correct the title, this cannot be proper english.
Expanded the work to define a default plugin manager interface.
Comment #91
benjy CreditAttribution: benjy commentedComment #92
yched CreditAttribution: yched commentedissue title is "Invoke an event[s] when a plugin instance is deleted"
Isn't it rather "... when a plugin ID disappears" ?
Comment #93
dawehnerSure ...
Expanded the patch in #90 with the different places of removing blocks in #86.
Comment #95
dawehnerFixed that failure.
Comment #97
yched CreditAttribution: yched commentedDoes it mean we expect modules that provide plugins to manually call removePluginId() on uninstall, or is that just required for dynamic plugins / derivatives ?
Comment #98
pfrenssenGoing to have a look at the failing tests.
Comment #99
pfrenssenComment #100
pfrenssen@yched, for the sake of consistency I would callremovePluginId()
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.
Comment #101
dawehner@pfrenssen
In case you made some progress here, feel free to post it at anytime. Afaik you worked on fixing some failures.
Comment #102
pfrenssenI'll post my progress when I get home tonight.
Comment #103
pfrenssenHere'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.
Comment #104
dawehnerIn order to be able to properly just remove the plugins you really want to remove we need to introduce some concept of plugin types.
I guess we want to properly inject that service as well :( This for sure requires a non-trivial amount of work. [Done] (hopefully)
I guess bringing on an actual example like: removed custom block would make that better to understand.
I think it is fine to not include setters here, there isn't really a usecase for it.
Let's use protected to be more like drupal.
bool or boolean afaik it is bool
Comment #106
martin107 CreditAttribution: martin107 commentedA little fix
Comment #108
martin107 CreditAttribution: martin107 commentedMini-commit spree overnight, means the patch needs a reroll, no conflicts just auto-merging and 3-way merging.
file diff shows a small change.
I can see a similar fix to #106 so I will grab the issue until its is fixed.
Comment #109
martin107 CreditAttribution: martin107 commentedComment #110
martin107 CreditAttribution: martin107 commentedThe next obvious step :-
Fixup \Drupal\condition_test\FormController::_construct
Comment #113
martin107 CreditAttribution: martin107 commentedHmm, so missing a few use statements ?
use DependencySerializationTrait;
Comment #114
dawehner@martin107
Thank you!
Let's fix the failures.
Comment #116
dawehnerGreen!
Comment #118
dawehnerMissing files.
Comment #119
martin107 CreditAttribution: martin107 commentedI am following along ... here is the interdiff for reference
Yay green
Comment #120
dawehnerReroll + some "minor" correction.
Comment #121
dawehnerRerolled, wow that was hard.
Implemented the feature of automatic calling when a plugin got removed, as we can apply some diff during cache clearing.
Comment #123
dawehnerLet's fix it.
Comment #124
EclipseGc CreditAttribution: EclipseGc commentedI really love seeing the old cache vs new definitions check and invoking this event based on that. Total ++
Eclipse
Comment #125
dawehnerAdapted the IS. @EclipseGc So was that a RTBC?
Comment #126
xjmIF DefaultPluginManagerInterface extends PluginManagerInterface, then why does the DefaultPluginManager need to implement both?
Comment #127
dawehnerYou could indeed ask the same question with PluginManagerBase, good catch!
Comment #128
EclipseGc CreditAttribution: EclipseGc commentedSo, 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
Comment #129
EclipseGc CreditAttribution: EclipseGc commentedSo 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.
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.
What's the usecase for this?
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.
Is this safe to do? Shouldn't we log the else result of this at the bare minimum?
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
Comment #130
pfrenssenI'm going to have a look.
Comment #131
pfrenssenRerolled against HEAD.
Comment #132
pfrenssenComment #133
pfrenssenComment #135
pfrenssenAnswering some questions from #129:
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.
Yes it's safe, if no blocks match the conditions then we don't need to delete them.
Comment #136
pfrenssenFixed failures and a small documentation update. Unassigning myself for the moment until we have a consensus on the way forward with this issue.
Comment #137
pfrenssenComment #139
pfrenssenComment #140
pfrenssenThe 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.
Comment #141
mgiffordComment #142
tim.plunkettThat's this issue.
Comment #143
mgiffordDrat.. 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.
Comment #144
mgiffordComment #145
Letharion CreditAttribution: Letharion commentedRerolled 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.
Comment #146
hussainweb@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.
Comment #147
Letharion CreditAttribution: Letharion commentedActual file
Comment #148
Letharion CreditAttribution: Letharion commented@hussainweb, thanks.
Dropped the extra validator interface.
Comment #151
Letharion CreditAttribution: Letharion commentedLet's try another patch where I do less stupid things to break it before uploading it.
Comment #152
mgiffordComment #153
Letharion CreditAttribution: Letharion commentedI 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.
Comment #155
Letharion CreditAttribution: Letharion commentedlol at 10000 failed tests.
Tests say "SQLSTATE[08004] [1040] Too many connections" though, so I don't think it's my fault.
Comment #156
Letharion CreditAttribution: Letharion commentedComment #158
EclipseGc CreditAttribution: EclipseGc commentedI see no reason for these conditions. A plugin id is either available or its not. There's no "available if" situation.
Explain why this was necessary for me?
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.
Again, the condition section of this is unnecessary. We can just remove by id.
Comment #159
Letharion CreditAttribution: Letharion commented> 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. :)
Comment #160
Letharion CreditAttribution: Letharion commentedComment #162
dawehnerWhat 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.
Comment #163
dawehnerLet's start to get this fixed after a strict reroll.
Comment #165
EclipseGc CreditAttribution: EclipseGc commenteddawehner,
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
Comment #166
dawehnerIn 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.Comment #167
alexpottWell we certainly have solved the problem outlined in the summary with fallback plugin ids and
EntityDisplayBase::onDependencyRemoval()
Comment #168
EclipseGc CreditAttribution: EclipseGc commentedThat'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
Comment #169
dawehnerWell, 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.
Comment #170
catchYes 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.
Comment #171
XanoRelated issue: #1886462: Determine how to clean up plugin instances when the derivative definitions change.
Comment #172
aspilicious CreditAttribution: aspilicious commentedI 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?
Comment #173
XanoThe 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:
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
, sincePluginManagerBase
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 withinDefaultPluginManager
. 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()
.Comment #174
xjmThis 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.
Comment #178
darvanenWould 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.