System menu blocks need to be able to depend on their menu entities

There's another one in there...

Also, while we're nitpicking, @see should come at the end, and @return should always be after @param.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemMenuBlock.php
@@ -24,7 +26,7 @@
-class SystemMenuBlock extends BlockBase implements ContainerFactoryPluginInterface {
+class SystemMenuBlock extends BlockBase implements ContainerFactoryPluginInterface, PluginConfigDependencyInterface {

@@ -107,4 +109,12 @@ protected function getRequiredCacheContexts() {
+  /**
+   * {@inheritdoc}
+   */
+  public function addConfigEntityDependency(ConfigEntityInterface $config_entity) {
+    $menu = \Drupal::entityManager()->getStorage('menu')->load($this->getDerivativeId());
+    $config_entity->addDependency('entity', $menu->getConfigDependencyName());
+  }
+

I 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.

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.

On 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.

It 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!

Yes, 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.

1. +++ b/core/modules/block/tests/Drupal/block/Tests/BlockConfigEntityUnitTest.php
   @@ -93,9 +93,19 @@ public function testCalculateDependencies() {
   +    $instance_dependency_1 = $this->randomName(10);
   +    $instance_dependency_2 = $this->randomName(11);
   

   Why 10 and 11?

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Derivative/SystemMenuBlock.php
   @@ -52,6 +52,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +      $this->derivatives[$menu]['config_dependencies']['entity'] = array($entity->getConfigDependencyName());
   

   And yeah, agreed that we need to document this somewhere.

3. +++ b/core/modules/system/lib/Drupal/system/Tests/Block/SystemMenuBlockTest.php
   @@ -0,0 +1,66 @@
   + * @todo improve the test to include coverage of all SystemMenuBlock related
   + *   functionality. For example, block_menu_delete().
   

   "Improve" should at least be capitalized. Clearer: "Expand test coverage to all SystemMenuBlock functionality, including block_menu_delete()."

4. +++ b/core/modules/system/lib/Drupal/system/Tests/Block/SystemMenuBlockTest.php
   @@ -0,0 +1,66 @@
   +  public function testSystemMenuBlockConfigDependencies() {
   

   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?

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
--- a/core/modules/block/tests/Drupal/block/Tests/BlockConfigEntityUnitTest.php
+++ b/core/modules/block/tests/Drupal/block/Tests/BlockConfigEntityUnitTest.php

+++ b/core/modules/block/tests/Drupal/block/Tests/BlockConfigEntityUnitTest.php
@@ -93,9 +93,19 @@ public function testCalculateDependencies() {
+    $instance_dependency_1 = $this->randomName(10);
+    $instance_dependency_2 = $this->randomName(11);
+    $plugin_definition = array(
+      'provider' => 'test',
+      'config_dependencies' => array(
+        'entity' => array($instance_dependency_1),
+        'module' => array($instance_dependency_2),
+      )
+    );
+    $instance = new TestConfigurablePlugin(array(), $instance_id, $plugin_definition);
 

We 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.

Thank you very much

I 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.

Since 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.

@see #16

Does this code example look right?

/**
 * Defines a Block configuration entity class.
 *
 * @ConfigEntityType(
 *   id = "block",
 *   label = @Translation("Block"),
 *   config_dependencies = {
 *     "entity" = {
 *       "menu.menu"
 *     }
 *   }
 * )
 */

Yeah I agree, there aren't many usecases for static blocks.

Committed/pushed to 8.x, thanks!

Yeah I agree, there aren't many usecases for static blocks.

Ok, 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.

Actually, API is wrong as well. I think I'll just continue with this page: https://drupal.org/node/2235409