Problem/Motivation

Configuration can depend on content entities. For example:

  • The default value of an entity reference field
  • A views filter
  • The placement of a custom block

The custom block example is especially complex since it custom block type is config, the custom block is content and the block placement is a config entity.

Proposed resolution

Add the content dependency information to the config entity.

Remaining tasks

Write patch
Review patch

User interface changes

None

API changes

Potentially additions to the config dependency system and configuration synchronisation.

Files: 
CommentFileSizeAuthor
#16 2282519.16.patch73.02 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,623 pass(es). View
#16 15-16-interdiff.txt3.5 KBalexpott
#15 2282519.15.patch70.61 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,583 pass(es). View
#15 12-15-interdiff.txt1.76 KBalexpott
#12 2282519.12.patch70.54 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,609 pass(es). View
#11 2282519.11.patch67.73 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,513 pass(es). View
#10 2282519.10.patch67.72 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,530 pass(es). View
#10 8-10-interdiff.txt3.75 KBalexpott
#8 2282519.8.patch63.96 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,497 pass(es), 6 fail(s), and 0 exception(s). View

Comments

alexpott’s picture

Berdir’s picture

I'd expect custom block to be easier to implement than the other two, actually.

a) It's static, we know what we're dealing with
b) We might not even have to care about the custom block type because we only need to worry about direct dependencies? If the custom block exists, then we can safely assume that the custom block type exists too? It's also not directly relevant for the block.

On the other side, the field default value and various views arguments and the default handling stuff are plugin-specific, and while there was already work done to convert content entity default values for fields to UUID's, this does not exist for views (which exports them as ID's), so we need to solve that problem first.

Already noticed this when trying to have default views with arguments and default values.

swentel’s picture

catch’s picture

I think they're different issues.

This one only covers configuration entities that have an explicit dependency on particular content entities, and it's open mainly because without this we can't validate deployment of configuration between instances of the same site (right now you could break a production site by deploying configuration without the requisite dependencies in place).

That issue deals with any data that has a dependency on content entities - which might well not be a configuration entity and isn't necessarily data that gets deployed (i.e. comment statistics table) - mainly for the case where modules are uninstalled.

catch’s picture

Issue tags: +D8 upgrade path
xjm’s picture

xjm’s picture

alexpott’s picture

Status: Active » Needs review
FileSize
63.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,497 pass(es), 6 fail(s), and 0 exception(s). View

Patch attached:

  • renames the entity key of config dependencies to config and adds a new content key
  • The getConfigDependencyName is moved to the EntityInterface so content entities get it to
  • Block placements of block content entities now depend on the content entity
  • Adds some tests of finding config entities but content entity dependency

Still to do - although I think this patch is big enough and maybe we just do the other steps in separate issues:

  • Add a step to config import to allow contrib to create content (maybe a separate issue)
  • Views filters (maybe separate issue)
  • Entity reference (maybe separate issue)

I'd like #2224581: Delete forum data on uninstall to land first since it changes the dependency array structure and add docs this will need to change.

Status: Needs review » Needs work

The last submitted patch, 8: 2282519.8.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.75 KB
67.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,530 pass(es). View
cilefen’s picture

FileSize
67.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,513 pass(es). View

reroll

alexpott’s picture

Related issues: +#2271419: Allow field types, widgets, formatters to specify config dependencies
FileSize
70.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,609 pass(es). View

Rerolled as #2224581: Delete forum data on uninstall has landed. We need to do #2271419: Allow field types, widgets, formatters to specify config dependencies for entity reference default values. Views dependencies also have their own issues.

The patch has tests, documentation and a implementation for block placements of content blocks. I think we're good to go here.

alexpott’s picture

swentel’s picture

looks good to me to, some minor nitpicks.

  1. +++ b/core/lib/Drupal/Component/Plugin/ConfigurablePluginInterface.php
    @@ -47,11 +47,11 @@ public function defaultConfiguration();
    -   *     'entity' => array('user.role.anonymous', 'user.role.authenticated'),
    +   *     'config' => array('user.role.anonymous', 'user.role.authenticated'),
    

    should we add a content example here as well ?

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -151,17 +157,17 @@ public function getDependentEntities($type, $name) {
    +      if ($type == 'module' || $type ==  'theme' || $type ==  'content') {
    

    nitpick, two spaces before 'theme' and 'content'

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -151,17 +157,17 @@ public function getDependentEntities($type, $name) {
    +      // If checking content, module, or theme dependencies then discover which
    

    maybe a comma after 'dependencies' ?

alexpott’s picture

FileSize
1.76 KB
70.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,583 pass(es). View

Thanks for the review @swentel - patch attached addresses all the points raised.

alexpott’s picture

FileSize
3.5 KB
73.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,623 pass(es). View

@swentel pointed out in IRC that some docs were not updated with the new location of getConfigDependencyName(). I've also improved the docs on the method.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

perfect!

alexpott’s picture

Discussed this with @damiankloip on IRC - he points out that we can have entity types that are neither content nor config - what are we going to do about them. In the current patch what 'content' really means in the config dependency array is "NOT a config entity". Anyone got any clever ideas on how to resolve. I feel that renaming "content" -> "entity" is confusing since "config" will contain config entities. This is one of the reasons why I named the original key "entity" but the problem is the config dependency system needs to treat dependencies due to config entities and content entities different.

yched’s picture

we can have entity types that are neither content nor config

Are those "grey area" entity types something we really need to be able to specify dependencies on ?
If it's "grey area", it means there is no generic mechanism to handle those dependencies anyway ?

catch’s picture

Agreed with yched in #19, I don't see why we'd want to generically handle unknown types of entity types here. Even content dependencies in config have a relatively limited use case in field API/Views.

Didn't review whole patch yet.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed c2125b0 on 8.0.x
    Issue #2282519 by alexpott, cilefen: Fixed Add content dependency...
Arla’s picture

Issue tags: +Needs change notification

I think a change record for the changed entity key in the config files would be in order.

larowlan’s picture

+1 to change notice - changes to config keys is an API break for anyone with a module containing default config - should have had change notice before it was RTBC in my opinion.

Berdir’s picture

Title: Add content dependency information to configuration entities » Needs change record: Add content dependency information to configuration entities
Status: Fixed » Active
Issue tags: -Needs change notification +Needs change record

Re-opening for change record and change record updates.

xjm’s picture

alexpott’s picture

Status: Active » Needs review
Issue tags: -Needs change record updates

There are no change records to update - config dependencies was completely new.

Added a draft CR https://www.drupal.org/node/2364725

Berdir’s picture

Title: Needs change record: Add content dependency information to configuration entities » Add content dependency information to configuration entities
Status: Needs review » Fixed
Issue tags: -Needs change record

Ok thanks, that looks good enough to me for this, will publish. Not the fault of this issue that we have nothing else, maybe we should, but no need to keep this issue open.

alexpott’s picture

Status: Fixed » Closed (fixed)

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