Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
block_menu_delete() enforces a relationship between system menus and system menu blocks that needs to be able to be stored in the configuration.
Proposed resolution
Implement an interface that plugins can use to add dependencies during a configuration save
Remaining tasks
Review patch
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#15 | 2232275.15.patch | 10.93 KB | alexpott |
#15 | 13-15-interdiff.txt | 7.35 KB | alexpott |
#13 | 2232275.13.patch | 7.13 KB | alexpott |
#13 | 9-13-interdiff.txt | 960 bytes | alexpott |
#9 | 2232275.9.patch | 7.06 KB | alexpott |
Comments
Comment #1
alexpottComment #2
alexpottAdded a specific test for SystemMenuBlock's config dependencies.
Comment #3
alexpott:( whitespace and new line...
Comment #5
xjmThere's another one in there...
Also, while we're nitpicking, @see should come at the end, and @return should always be after @param.
Comment #6
alexpottMore whitespace - I suck at this.
Comment #7
tim.plunkettI haven't really thought through what I'm about to ask, but...
Is there some way this could live on the deriver? \Drupal\system\Plugin\Derivative\SystemMenuBlock already has the menu entity storage injected, and it seems much more germane to that class.
No idea if we can backtrack to it from the plugin, but I think its worth considering. This block class knows absolutely nothing about entities, it just knows it has a string that MenuTreeInterface::renderMenu() will accept.
We've worked hard to keep plugins in the dark about their surrounding context, it'd be nice to continue that.
Comment #8
dawehnerOn the other hand we also have to support all kind of blocks. Maybe it would be already enough if derivative classes could pass along a dependencies key in the plugin definition.
Comment #9
alexpott@dawehner, @tim.plunkett what a nice idea :) - works a treat. Where do we document generic plugin definition keys like
provider
and nowconfig_dependencies
. I'm super happy thataddDependency()
stays protected and we don't get yet another interface.Comment #10
tim.plunkettIt used to be documented on Drupal\Component\Annotation\Plugin, but we switched that to methods, and now I'm really not sure.
But that aside, I really like the look of this patch now!
Comment #11
xjmYes, I think that addresses the hesitation I had about this as well. Good stuff.
In the process of discussing this issue I was reminded of our never-delivered block context API, and ended up digging out #1879896: Add web tests for module-specific block definitions cache invalidation. I wonder if there is a way we could use this API to partly address those sorts of problems as well? Edit: Obviously it doesn't help for non-config contexts, like node content or something, but it's half a thought.
Comment #12
xjmWhy 10 and 11?
And yeah, agreed that we need to document this somewhere.
"Improve" should at least be capitalized. Clearer: "Expand test coverage to all SystemMenuBlock functionality, including block_menu_delete()."
Needs a docblock, unless the policy got changed while I wasn't looking. Edit: Which could be, I know it's been discussed for test methods the past. Anyone have a coding standards issue link?
Comment #13
alexpott\Component
. And Plugin annotation classes all extendDrupal\Component\Annotation\Plugin
. Obviously this would only be of use in a configurable but again they all extendDrupal\Component\Plugin\ConfigurablePluginInterface
. Anyone got any good ideas?Comment #14
dawehnerWe test functionality on the Config entity base but do test the block config entity?? Yes this was the case before, but this just feels 100% wrong.
Comment #15
alexpott@dawehner you are 100% right :)
Moved the test to ConfigEntityBaseUnitTest and converted the tests to a data provider type test and clean up a few other things (code format and the inline use of TestConfigurablePlugin)
Comment #16
dawehnerThank you very much
Comment #17
alexpottWe still need to decide where to document this.
Comment #18
alexpott... but perhaps someone can have a bright idea whilst reviewing the patch
Comment #19
xjmI spent way too much time going down the rabbit hole to figure out where to put the docs; I can't even find the ones we wrote for entities anymore. I don't think ConfigurablePluginInterface is related other than having "config" in the name, is it?
I can't even find the docs we wrote for the entity definition annotations anymore... filed #2234439: Add a section for the plugin system to the API doc landing page, not that that will help us directly here.
Comment #20
xjmComment #21
jessebeach CreditAttribution: jessebeach commentedSince the documentation bit of this patch is holding up the code and since this task involves more than just writing the docs -- first they must be found -- I've hewn off this bit into another issue. It's assigned to me and on my list of things to do so it won't get dropped.
#2235363: Document config dependencies.
Comment #22
dawehner@see #16
Comment #23
jessebeach CreditAttribution: jessebeach commentedDoes this code example look right?
Comment #24
alexpott@jessebeach well there would never be a config entity called
menu.menu
... this feature is much more likely to be used by derivatives that are based on a config entity like SystemMenuBlock where the instance definition and therefore config dependency can be worked out programmatically.Comment #25
dawehnerYeah I agree, there aren't many usecases for static blocks.
Comment #26
catchCommitted/pushed to 8.x, thanks!
Comment #28
jessebeach CreditAttribution: jessebeach commentedOk, in that case I think this docs should go into API in system.api.php rather than more general documentation about config dependencies. Or maybe a short explanation that dependencies exist and a link to the more detailed API page.
Comment #29
jessebeach CreditAttribution: jessebeach commentedActually, API is wrong as well. I think I'll just continue with this page: https://drupal.org/node/2235409