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

Files: 
CommentFileSizeAuthor
#15 2232275.15.patch10.93 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,624 pass(es). View
#15 13-15-interdiff.txt7.35 KBalexpott
#13 2232275.13.patch7.13 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,470 pass(es). View
#13 9-13-interdiff.txt960 bytesalexpott
#9 2232275.9.patch7.06 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,459 pass(es). View
#9 5-9-interdiff.txt9.04 KBalexpott
#6 2232275.5.patch12.23 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,071 pass(es). View
#6 3-5-interdiff.txt888 bytesalexpott
#3 2232275.3.patch12.26 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,031 pass(es). View
#3 2-3-interdiff.txt369 bytesalexpott
#2 2232275.2.patch12.28 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#2 1-2-interdiff.txt1.77 KBalexpott
#1 2232275.1.patch10.34 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,023 pass(es). View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
10.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,023 pass(es). View
alexpott’s picture

FileSize
1.77 KB
12.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Added a specific test for SystemMenuBlock's config dependencies.

alexpott’s picture

FileSize
369 bytes
12.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,031 pass(es). View

:( whitespace and new line...

The last submitted patch, 2: 2232275.2.patch, failed testing.

xjm’s picture

There's another one in there...

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

alexpott’s picture

FileSize
888 bytes
12.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,071 pass(es). View

More whitespace - I suck at this.

tim.plunkett’s picture

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

dawehner’s picture

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.

alexpott’s picture

FileSize
9.04 KB
7.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,459 pass(es). View

@dawehner, @tim.plunkett what a nice idea :) - works a treat. Where do we document generic plugin definition keys like provider and now config_dependencies. I'm super happy that addDependency() stays protected and we don't get yet another interface.

tim.plunkett’s picture

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!

xjm’s picture

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.

xjm’s picture

  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?

alexpott’s picture

FileSize
960 bytes
7.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,470 pass(es). View
  1. Well random name guarantees uniqueness (it didn't always) but I just didn't want someone to look at the code well they possibly could be the same
  2. Really unsure about where to do this. It feels wrong to document this in \Component. And Plugin annotation classes all extend Drupal\Component\Annotation\Plugin. Obviously this would only be of use in a configurable but again they all extend Drupal\Component\Plugin\ConfigurablePluginInterface. Anyone got any good ideas?
  3. Fixed
  4. Fixed
dawehner’s picture

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

alexpott’s picture

FileSize
7.35 KB
10.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,624 pass(es). View

@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)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you very much

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

We still need to decide where to document this.

alexpott’s picture

Status: Needs work » Needs review

... but perhaps someone can have a bright idea whilst reviewing the patch

xjm’s picture

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.

xjm’s picture

Issue tags: +Configuration system
jessebeach’s picture

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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

@see #16

jessebeach’s picture

Does this code example look right?

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

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

dawehner’s picture

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit d8a9912 on 8.x by catch:
    Issue #2232275 by alexpott: System menu blocks need to be able to depend...
jessebeach’s picture

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.

jessebeach’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.