Updated: Comment #10

Problem/Motivation

While #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API concerns the issue of default configuration, this issue is about configuration dependencies when a module is uninstalled. (#2082117: Install default config only when the owner and provider modules are both enabled covers installation.)

During lengthy IRC and discussion at Prague with catch, beejeebus, alexpott, mtift, and others, we came to a general consensus that we should create an API that would allow Module A to declare that it has config entities that depend on Module B. When Module B is being uninstalled this API will be used to inform the user that uninstallation will result in their removal.

Discussion points raised in Prague:

For dual-ownership config entities, what should happen on uninstall? Three options:

  • Allow user to back out or delete all your things
  • Disallow uninstall with soft dependencies
  • Just do nothing and let the site break

If we allow deleting all the things we need:

  • A confirmation screen listing everything that will be deleted
  • We should have a UI to batch deletions for (e.g.) field deletions
  • We should have an API to declare soft dependencies; it should not be in field_system_info()
  • Bundles are not formalized, so field API can't implement this dependency management. Can the entity type provider, e.g. node? There can be any number of bundle providers dependent on one entity type provider.
  • Also would need to create a robust batching API.

We also need to consider if deleting the forum node type also implies those field instances are being deleted? And do we need to tell the user? (we should provide a summary, e.g. this will delete the node type and N nodes)

Proposed resolution

Store dependencies in the configuration entity files in a new dependencies key. For example, field.instance.node.article.body

dependencies:
  entity:
    - 'field.field.node.body'

And the views.view.frontpage
dependencies:
  module:
    - node

Multiple dependencies are possible - for example entity.view_display.node.article.teaser (in the standard profile)
dependencies:
  entity:
    - entity.view_mode.node.teaser
    - field.instance.node.article.body
    - field.instance.node.article.field_image
    - field.instance.node.article.field_tags
    - node.type.article
  module:
    - image
    - taxonomy
    - text

Introduce a ConfigDependencyManager that is able to read configuration files and construct a graph to determine dependencies between configuration entities or determine which entities are dependent on a specific extension (module or theme).

What dependencies do config entities have

Config entity type Dependencies
block The module that provides the plugin. The theme.
breakpoint if the source is theme or module it depends on that. Currently handled by the module - all this code can be removed (yay!).
breakpoint_group the breakpoint entities, if the source is theme or module that too. Currently handled by the module - all this code can be removed (yay!).
contact_category n/a
custom_block_type n/a
editor The filter format it is attached to. The module that supplies the editor plugin. Follow up to convert Editor entity to EntityWithPluginBagInterface and therefore the editor dependency will exist automatically due to ConfigEntityBase::preSave()
entity_form_display the form mode entity, the config entity if one provides the bundle, the fields, the widget providers
form_mode module that provides the target entity type
entity_view_display the view mode entity, the config entity if one provides the bundle, the fields, the formatter providers
view_mode module that provides the target entity type
field_config module that provides the field type
field_instance_config field_config entity, the entity that provides the bundle (eg. node type)
filter_format the modules that supply the filter plugins in use
image_style the modules that supply the image effect plugins in use
language_entity n/a
node_type This has a settings key that modules had into but uninstall is handled. Should be converted to the dependency system if these are converted to plugins
picture_mapping The breakpoint group
rdf_mapping the bundle entity
search_page the module that provides the plugin
shortcut_set n/a
action the modules that supply the action plugins in use
date_format n/a
menu n/a
taxonomy_vocabulary n/a
tour The module the tour is assigned to. The Tip plugin providers. Follow up to convert Tour entity to EntityWithPluginBagInterface and therefore the Tip plugin provider dependency will exist automatically due to ConfigEntityBase::preSave()
user_role n/a
view modules that provide the plugins if not optional, field_instances_config entities?

Remaining tasks

  • Review

User interface changes

API changes

A new API to allow one module to indicate it has soft dependencies through config entities it "owns" (i.e. where it provides the config entity type) on another modules.

#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed
#1765936: Invoke ConfigStorageController::delete on module uninstall and add tests
#1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API
#2079121: When a module providing a base table is uninstalled, delete all views built on it

Files: 
CommentFileSizeAuthor
#129 123-129-interdiff.txt1023 bytesalexpott
#129 2080823.129.patch219.14 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,501 pass(es).
[ View ]
#123 2080823.123.patch219.27 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,491 pass(es).
[ View ]
#123 121-123-interdiff.txt14.48 KBalexpott
#121 120-121-interdiff.txt15.11 KBalexpott
#121 2080823.121.patch215.15 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,555 pass(es).
[ View ]
#120 interdiff.txt18.99 KBWim Leers
#120 2080823.120.patch214 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,552 pass(es).
[ View ]
#119 2080823.119.patch214.76 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,531 pass(es).
[ View ]
#119 117-119-interdiff.txt2.63 KBalexpott
#117 2080823.117.patch212.77 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,474 pass(es).
[ View ]
#117 112-117-interdiff.txt11.64 KBalexpott
#112 2080823.112.patch212.69 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,523 pass(es).
[ View ]
#112 109-112-interdiff.txt6.02 KBalexpott
#112 configuration_deletions_closed.png42.34 KBalexpott
#112 config_deletions_open.png165.08 KBalexpott
#109 2080823.109.patch209.15 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,512 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#109 108-109-interdiff.txt15.68 KBalexpott
#108 2080823.108.patch196.7 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,420 pass(es).
[ View ]
#105 2080823.105.patch195.01 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,343 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#105 103-105-interdiff.txt2.49 KBalexpott
#103 2080823.103.patch193.01 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,432 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#103 102-103-interdiff.txt49.65 KBalexpott
#102 2080823.102.patch151.81 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,359 pass(es).
[ View ]
#102 99-102-interdiff.txt7.91 KBalexpott
#99 2080823.99.patch149.88 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,327 pass(es).
[ View ]
#99 97-99-interdiff.txt328 bytesalexpott
#97 2080823.97.patch150.3 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed to login to test site.
[ View ]
#97 95-97-interdiff.txt30.22 KBalexpott
#95 2080823.95.patch145.3 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,165 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#95 86-95-interdiff.txt12.61 KBalexpott
#87 ConfigDependency.png66.59 KBGábor Hojtsy
#86 2080823.86.patch142.69 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,162 pass(es).
[ View ]
#86 83-86-interdiff.txt1.06 KBalexpott
#83 2080823.83.patch141.34 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,132 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#83 79-83-interdiff.txt1.11 KBalexpott
#79 2080823.79.patch141.59 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,088 pass(es), 1 fail(s), and 508 exception(s).
[ View ]
#79 77-79-interdiff.txt1.66 KBalexpott
#77 2080823.77.patch140.9 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,112 pass(es), 1 fail(s), and 511 exception(s).
[ View ]
#77 73-77-interdiff.txt8.74 KBalexpott
#73 2080823.73.patch134.58 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,095 pass(es), 44 fail(s), and 6 exception(s).
[ View ]
#73 70-73.interdiff.txt916 bytesalexpott
#72 68-70.interdiff.txt19.73 KBalexpott
#70 2080823.70.patch135.64 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,099 pass(es), 44 fail(s), and 6 exception(s).
[ View ]
#68 2080823.68.patch119.5 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 61,676 pass(es), 43 fail(s), and 0 exception(s).
[ View ]
#67 2080823.67.patch110.13 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,896 pass(es).
[ View ]
#67 63-67-interdiff.txt4.34 KBalexpott
#64 2080823.63.patch105.81 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,766 pass(es), 48 fail(s), and 32 exception(s).
[ View ]
#60 2080823.60.patch101.48 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 62,195 pass(es), 105 fail(s), and 4 exception(s).
[ View ]
#45 interdiff.txt2.81 KBswentel
#45 2080823.45.patch70.19 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 64,397 pass(es).
[ View ]
#40 2080823.39.patch66.67 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 64,447 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#40 36-39-interdiff.txt18.62 KBalexpott
#36 2080823.36.patch60.46 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 64,382 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#36 interdiff.txt10.91 KBswentel
#33 30-33-interdiff.txt9.83 KBalexpott
#33 2080823.33.patch53.55 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 64,379 pass(es).
[ View ]
#30 2080823.30.patch53.38 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 64,360 pass(es), 1 fail(s), and 2,201 exception(s).
[ View ]
#30 29-30-interdiff.txt7.97 KBalexpott
#29 2080823.29.patch52.92 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 64,272 pass(es), 1 fail(s), and 2,186 exception(s).
[ View ]
#29 26-29-interdiff.txt8.09 KBalexpott
#29 config-entity-dependencies.png98.21 KBalexpott
#26 2080823.26.patch44.31 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 64,375 pass(es), 3 fail(s), and 2 exception(s).
[ View ]
#26 25-26-interdiff.txt2.61 KBalexpott
#25 2080823.25.patch42.62 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 64,351 pass(es), 28 fail(s), and 2 exception(s).
[ View ]
#22 2080823.22.patch25.34 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 64,411 pass(es), 41 fail(s), and 5 exception(s).
[ View ]
#22 19-22-interdiff.txt3.76 KBalexpott
#19 2080823.19.patch22.61 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 64,250 pass(es), 68 fail(s), and 8 exception(s).
[ View ]

Comments

Priority:Major» Critical

Copying my comment over from that issue:

So the problem with leaving the front page view in when node module is disabled, is the same problem with disabling and re-enabling modules I think. Except it's worse, because we run updates for disabled modules currently, don't do that for uninstalled ones.

Here's what I think will happen, correct me if I'm wrong:

1. Node module is uninstalled, views is left installed.

2. Front page view (still using node entity or other views plugins provided by node) stays in active config.

3. Meanwhile, node module code is updated to rename a plugin, includes an update handler to rename the plugin in views configuration files. Because node module is currently uninstalled, this update never runs.

4. Node module is re-installed, now the view is missing plugins (assuming we don't overwrite it on re-install? But then what would be the point of keeping it...).

So of the options, I prefer this one:

Although, thinking about it explicitly declaring dependencies in config entities would let us fix this problem as well, by reversing it. We could disallow uninstalling module completely if they are required by any configuration entities. That would be similar to what we currently do with field-type-providing modules, except generalized in the Config system. You would then have to completely override or consciously delete the frontpage view in order to uninstall Node module.

I don't see a way around week/month/year-long race conditions unless we either prevent modules from being uninstalled when configuration depends on them, or uninstall all that configuration. Both of those options are valid. People don't like making modules 'required' dynamically though, so here's an alternative suggestion:

- When uninstall is submitted, validate whether any config entities have a dependency on the module.

- If they do, show a confirmation screen.

- That confirmation screen tells you what's going on, if you want to delete those config entities anyway, you have to explicitly select a checkbox. Otherwise it prevents the uninstall of those modules. For drush and similar it could add an option or its own confirmation.

Issue summary:View changes

remove suggestion to change ModuleHandler::uninstall()

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

#1776830-83: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API had a very important fix, we should make sure it doesn't get lost.
Whatever the process for deleting config or not, when we do delete stuff that is a config entity, we should delete it as a config entity, not just wipe its config file.
[edit: fixed link to the real comment I had in mind :-/]

@yched: Yes, although I think you meant #1776830-75: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API. Yesterday in IRC alexpott talked about moving that over.

Doh, sorry, copied the wrong comment #. No, actually the one I was thinking of was @beejebus #83.
I fixed my comment above.

Title:Disallow module uninstall if there are configurables that depend on that moduleDisallow module uninstall if there are content or config entities that depend on that module

Status:Postponed» Active

Title:Disallow module uninstall if there are content or config entities that depend on that moduleCreate API to discover content or config entities soft dependencies and use this to present a confirm form on module uninstall

Retitling issue to focus issue on creating API and providing a use case.

bump.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

log

Issue summary:View changes

log

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
Issue tags:+beta blocker

Considering this is a key API we should have this done before D8 beta.

Providing a clear UI and batching system that can intelligently delete any type of soft dependency seems like an awful lot of work, particularly if there are dependencies of dependencies. Even if such a system could be written, it is making it too easy for site builders to accidentally delete large amounts of content? Maybe you don't want the content to be deleted, but rather converted to some other format.

I think we should be aiming to support a UI that lists the categories of content that are preventing a module from being uninstalled, and allows modules to provide links to admin interfaces to edit/remove that content (similar to the configure links on the Extend page). That way site builders are more likely to understand what they are doing, and each module can provide a suitable UI.

This is not at all a solution to the problem at hand, but I just want to cross link the Entity Dependency module that would benefit of/extend a generic entity dependency API. That module is currently being used to find content dependencies for content staging solutions built with Deploy.

Also, we have sdboyer's graph api in core, I'm using it in github.com/larowlan/default_content to resolve dependency resolution between default content, default config would be very similar.

Is the use case only on module uninstall? Nothing to cover for installation or import here? Then this sounds like something that Drupal did not provide before, did it? So looks like part of the question on how we define CMI's scope. In Drupal 7, if you delete a node type, are its fields deleted? Are the field data storages deleted? In other words is this a regression from Drupal 7 or a want for Drupal 8? :)

In Drupal 7, if you delete a node type, are its fields deleted? Are the field data storages deleted?

Yes, if you use the API properly:
https://api.drupal.org/api/drupal/modules%21field%21field.attach.inc/fun...

For import/install see #2091615: Installation profiles provide default config that is not only used during installation #2090115: Don't install configuration when it has a soft-dependency on a module that's not installed both postponed on this issue.

Status:Active» Needs review
StatusFileSize
new22.61 KB
FAILED: [[SimpleTest]]: [MySQL] 64,250 pass(es), 68 fail(s), and 8 exception(s).
[ View ]

A patch!!! Too tired to write much of a comment - but just in case I'm hit by a bus.

class ViewStorageController extends ConfigStorageController {
  /**
   * {@inheritdoc}
   */
  public function loadMultiple(array $ids = NULL) {
    $entities = parent::loadMultiple($ids);
    // Only return views for enabled modules.
    return array_filter($entities, function ($entity) {
      if (\Drupal::moduleHandler()->moduleExists($entity->get('module'))) {
        return TRUE;
      }
      return FALSE;
    });
  }
}

This patch replaces the above code by having an dependency management system. If you uninstall a module and a view depends on it that view will be removed.

Still have to handle dependencies on other configuration entities.

Status:Needs review» Needs work

The last submitted patch, 19: 2080823.19.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 19: 2080823.19.patch, failed testing.

StatusFileSize
new3.76 KB
new25.34 KB
FAILED: [[SimpleTest]]: [MySQL] 64,411 pass(es), 41 fail(s), and 5 exception(s).
[ View ]

Fixes some of the test failures.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 22: 2080823.22.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new42.62 KB
FAILED: [[SimpleTest]]: [MySQL] 64,351 pass(es), 28 fail(s), and 2 exception(s).
[ View ]

Slightly new approach. I've completely removed the dependency on the Config Entity type actually existing so the dependency management system can work for configuration import validation (i.e. before modules are actually enabled).

Additionally this patch adds the ability the make configuration entities dependent on each other and discover this dependencies using the Graph object. Added test for a couple of scenarios.

Furthermore continued working on the implementation for views. Discovered that View::$module is not correct for a couple of the default views. Also started to implement the dependency in the default views configuration.

StatusFileSize
new2.61 KB
new44.31 KB
FAILED: [[SimpleTest]]: [MySQL] 64,375 pass(es), 3 fail(s), and 2 exception(s).
[ View ]

Fix configuration tests.

The last submitted patch, 25: 2080823.25.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 26: 2080823.26.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new98.21 KB
new8.09 KB
new52.92 KB
FAILED: [[SimpleTest]]: [MySQL] 64,272 pass(es), 1 fail(s), and 2,186 exception(s).
[ View ]

Added dependency management for fields.

StatusFileSize
new7.97 KB
new53.38 KB
FAILED: [[SimpleTest]]: [MySQL] 64,360 pass(es), 1 fail(s), and 2,201 exception(s).
[ View ]

Oops got the direction of dependency discover for entities to wrong way around.

The last submitted patch, 29: 2080823.29.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 30: 2080823.30.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new53.55 KB
PASSED: [[SimpleTest]]: [MySQL] 64,379 pass(es).
[ View ]
new9.83 KB

Fixed the test fails by making the dependency a value rather than a key - since config keys can not contain a dot.

Created a sandbox: http://drupalcode.org/sandbox/heyrocker/1145636.git/shortlog/refs/heads/...

Fixing the views dependencies issues pointed out by @swentel. Uninstalling history was prompting uninstallation of views :) http://imgur.com/lAPVwvV

Move views.view.tracker back to views and undid changes. This view is dependent on both history and comment - we have no module that satisfies these conditions - let's do #1853572: Remove the old default tracker view

StatusFileSize
new10.91 KB
new60.46 KB
FAILED: [[SimpleTest]]: [MySQL] 64,382 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Let's see how much this blows up with the dependant config entities now removed by the Entity API and the removal of field_system_info_alter(). Uninstalling image worked fine already locally! Haven't committed/pushed this yet, code is not /that/ sweet.

Added an isUninstalling() method so we can use this. Currently use in preDelete of Language config entity.

Status:Needs review» Needs work

The last submitted patch, 36: 2080823.36.patch, failed testing.

Never thought I would say this, but these failures are fantastic :)

  1. +++ b/core/modules/views/lib/Drupal/views/Entity/View.php
    @@ -276,6 +277,40 @@ public function getExportProperties() {
    +    $this->createDependency('module', $this->module);

    If we implement all dependencies of Views, I think we don't actually need this line. I'm thinking of a feature module that doesn't provide any Views plugins but provides a couple of default views. In that case, whether we delete the view when the module is uninstalled comes down to choice. Can be follow-up either way.

  2. +++ b/core/modules/views/lib/Drupal/views/Entity/View.php
    @@ -276,6 +277,40 @@ public function getExportProperties() {
    +            if (isset($handler['provider']) && empty($handler['optional'])) {

    This is great!

Status:Needs work» Needs review
StatusFileSize
new18.62 KB
new66.67 KB
FAILED: [[SimpleTest]]: [MySQL] 64,447 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

New patch cut from the sandbox

whether we delete the view when the module is uninstalled comes down to choice

We would ideally still want to know where this view came from though?

This patch relies on the config entity uuid key being uuid - it is hard coded to be but we pretend this is not the case - see #2198377: Enforce UUID key name in configuration entities

Status:Needs review» Needs work

The last submitted patch, 40: 2080823.39.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new70.19 KB
PASSED: [[SimpleTest]]: [MySQL] 64,397 pass(es).
[ View ]
new2.81 KB

Alex fixed the dependency test. I refactored the reEnableModuleFieldTest. I had to add an extra check in FieldInstance to make sure we don't delete fields there while uninstalling, otherwhise FieldableStorageController goes boom.

This should be green again.

- edit - the comment in the test should be adjusted, the table will not be gone yet.

W00t ! Awesome work !

More general / conceptual remarks for now, saving more specific / nitpicky code remarks for later.

  1. +++ b/core/modules/field/field.module
    @@ -146,45 +146,6 @@ function field_cron() {
    -function field_system_info_alter(&$info, $file, $type) {

    Insert Ewok dance here.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -135,4 +157,38 @@ function uninstall($type, $name) {
    +    foreach ($entities as $entity_type_id => $entities_to_load) {
    +      $storage_controller = $this->entityManager->getStorageController($entity_type_id);
    +      $entities_to_return = $entities_to_return + $storage_controller->loadMultiple($entities_to_load);

    Looks like there's a chance of collisions, two entities of different entity types might very well happen to have the same id.

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -0,0 +1,154 @@
    +  protected function loadAllTheThingies () {

    Favorite method name ever ;-)

    More seriously: wow, this reads all config records, and builds & statically persists one ConfigEntityDependency object for each existing config entity.
    Next to that, we also build & persist a graph that contains a proportional amount of data.
    That's a lot of data to keep in memory :-/

    It seems we never actually need the *full* graph for all the config records:
    - getDependentEntities($type, $name) uses loadAllTheThingies() to find config entities that depend on the module $name
    - then we only need the graph starting from *those* config entities.
    So if CDM::getDependentEntities() received an array of $names (instead of currently a single $name, and ConfigManager::findConfigEntityDependents(array $names) iterating seperate calls on each $name), then maybe we could only build a "smaller" graph and then ditch it instead of keeping it in memory ?

    I don't see how we can avoid parsing all config entity records.
    But maybe there's at least a way to limit the amount of ConfigEntityDependency objects we create and persist ?

  4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -43,6 +43,20 @@
    +  private $isUninstalling = FALSE;

    The property name (and associated setter and getter names) looks a bit off.

    Entity->isSyncing() is fine because it means "the entity is being synced".
    But $entity->isUninstalling()... well, the entity is not being uninstalled :-)
    No better suggestion for now though.

    Actually, I'm not sure I understand why that property / method is needed. Only FieldInstance and Language use it currently. What situation is this preventing ?

  5. +++ b/core/modules/field/config/schema/field.schema.yml
    @@ -56,6 +56,9 @@ field.field.*.*:
    +    dependencies:
    +      type: config_dependency
    +      label: 'Dependencies'

    Hmm - ConfigEntityBase::getExportProperties() takes care of saving $this->dependencies if it exists, and the patch updates all existing overrides of the method to include 'dependencies' too (without the "if it exists" part, though).

    So each config entity will need to explicitly add this to its schema as well ? (patch currently only takes care of field and views schemas)
    Wondering if there would be a less painful way ? Well, we already force each schema to re-define 'uuid' & 'langcode'...

  6. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstance.php
    @@ -367,6 +368,9 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
    +
    +    // Manage dependencies.
    +    $this->createDependency('entity', $this->field->getConfigDependencyName());

    OK, so the 'instance' has a dependency on its 'field'. This lets us display accurate info on what will get deleted on module uninstall confirmation form. Uninstalling 'image' deletes all image fields and their instances.

    Yet Field::preDelete() still has its custom code to manually delete instances when the field is deleted.

    Or, put another way : shouldn't it be automatic that all dependents of a config entity get deleted whenever that entity gets deleted - not just at uninstall time ?
    What's the meaning of "having a dependency on another entity" if you can still exist when that entity got deleted ?

  7. +++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallConfirmForm.php
    @@ -121,6 +121,33 @@ public function buildForm(array $form, array &$form_state) {
    +        $form['entities'][$entity_type_id] = array(
    +          '#theme' => 'item_list',
    +          '#title' => $entity_type->getLabel(),
    +          '#items' => array(),
    +        );

    I'm afraid getLabel() is not enough - there will be entities with the same label (e.g. most of the time, instances of the same field just reuse the same label). We'd need the id() too IMO.

  8. +++ b/core/modules/views/lib/Drupal/views/Entity/View.php
    @@ -276,6 +277,40 @@ public function getExportProperties() {
    +  public function preSave(EntityStorageControllerInterface $storage_controller) {
    +    parent::preSave($storage_controller);
    +    // Clear out the dependencies since we're going to recalculate them.
    +    $this->dependencies = array();

    It feels potentially confusing that some EntityTypes allow you to manually add $entity->addDependency() at runtime, while others will wipe and rebuild them themselves.

    Shouldn't it be the same behavior for all entity types ?

    (Also, feels weird that we have wrapper methods to add and remove dependencies, but that emptying is done by directly working on the ->dependencies property)

1. Need video evidence
2. Nice spot - pushed a patch to address and test this to the sandbox.
3. We need all the config entities unfortunately as things depends on config entities which themselves have no dependencies. Without these entities we can not build a graph. We only persist the graph and config entities in the ConfigDependencyManager for as long as it exists (rather than the whole request)
4. We trying to ensure that we don't raise exceptions during a delete. The language entity prevents deleting the default language since this would seriously mess up you site - unless you are uninstalling the language module.
5. Some call the parent function some don't - I think we we do is ok.
6. Yes we should use this system elsewhere.
7. Yep - I would actually like this to be a link to where the user can manage the thing.
8. I'm not sure I agree. Views is wiping the dependencies because it know it can recalculate them. Other entities can choose to do this differently - it should be up to the module that creates the entity type to implement this how they like.

Also we have to worry about secondary deletes :)

The language entity prevents deleting the default language since this would seriously mess up you site - unless you are uninstalling the language module.

IMO it would be fine to enforce this in the UI and not do anything at the API level - would save adding the odd uninstallation check.

We don't do things like preventing the deletion of nodes set as the front page or deleting users that authored thousands of nodes, and this isn't really that much different.

IMO it would be fine to enforce this in the UI and not do anything at the API level - would save adding the odd uninstallation check.

The UI already prevents you deleting it, I'm guessing it's there to prevent it when it would be triggered programmatically.

Right but is that really necessary? If you delete it programmatically you should be able to understand the consequences of doing so / avoid doing it in the first place.

@catch this is different - if you do this you site is totally kaput.

What if during uninstall we delete the default language configuration value before deleting the default language?

Also I think it's OK to let people do things via the API that completely kaput their site. We don't lock down db_truncate() either.

Not sure if it's relevant, but I see mention of "graph" here. If you need more graph capabilities, you may want to look at the Gliph graph library that Drupal includes — written by sdboyer :)

However, @alexpott stated in IRC that #2199483: Provide a default config_prefix based on entity type ID and provider is a hard blocker for this issue, so should this issue be postponed? If so, it would also be good to document exactly why here.

Issue summary:View changes

Issue summary:View changes

Issue summary:View changes

Awesome list in the issue summary.

I rescoped #2198429: Make deleted fields work with config synch based on our call earlier this week; is that the right way to go? I don't think it blocks this but adding that "deleted stuff" step should probably be beta-blocking as part of making config synch work.

StatusFileSize
new101.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 62,195 pass(es), 105 fail(s), and 4 exception(s).
[ View ]

New patch adds dependency management for

  • all config entity types that use EntityWithPluginBagInterface (block, filter format, image style, search page, action)
  • entities that implement EntityDisplayBase (form display and view display)
  • editor entities
  • rdf mappng entities

The config installer now installs default configuration in the order to guarantee dependencies exist.

Unfortunately an interdiff was not possible.

Status:Needs review» Needs work

The last submitted patch, 60: 2080823.60.patch, failed testing.

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityDependency.php
    @@ -0,0 +1,107 @@
    +   * @var array

    The array structure must be documented.

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityDependency.php
    @@ -0,0 +1,107 @@
    +   * @return array

    Same here.

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityDependency.php
    @@ -0,0 +1,107 @@
    +   * @return bool

    The return value must have a description.

  4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
    @@ -92,6 +92,14 @@ public function status();
    +   * @return bool

    The return value must have a description.

  5. +++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallConfirmForm.php
    @@ -121,6 +121,33 @@ public function buildForm(array $form, array &$form_state) {
    +          '#title' => $entity_type->getLabel(),

    It may be useful to link to the entities, so site admins can quickly inspect the data they are about to remove.

  6. +++ b/core/modules/views_ui/tests/Drupal/views_ui/Tests/ViewUIObjectTest.php
    @@ -47,7 +47,10 @@ public function testEntityDecoration() {
    +      // process. ConfigEntityInterface::getConfigDependencyName is only used

    The method name is missing brackets.

Also, why doesn't the patch add PHPUnit tests for the newly added code to entities? When #2134857: PHPUnit test the entity base classes is fixed, that won't even be hard to do anymore.

Status:Needs work» Needs review
StatusFileSize
new105.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,766 pass(es), 48 fail(s), and 32 exception(s).
[ View ]

@Xano this patch is no way near ready for that kind of review.

We have issues with secondary writes during module install now everything is being installed in order of dependencies!

Status:Needs review» Needs work

The last submitted patch, 64: 2080823.63.patch, failed testing.

Looks like this issue is going forward without #2198429: Make deleted fields work with config synch; should that updated issue happen alongside this one? After we do #1808248: Add a separate module install/uninstall step to the config import process?

Status:Needs work» Needs review
StatusFileSize
new4.34 KB
new110.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,896 pass(es).
[ View ]

Fixed failing tests - loads of work still needed

StatusFileSize
new119.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 61,676 pass(es), 43 fail(s), and 0 exception(s).
[ View ]

Patch attached to show where I'm at and to display something interesting. The search module creates entity view modes for node - but the search module does not depend on node. This is a problem - since we can not work out the node entity's provider if the node entity does not exist! To test this - install minimal, disable node, enable search.. KABOOOM! Also Drupal\system\Tests\Entity\ConfigEntityImportTest breaks because of this - and probably any test that enables search but node is going to have a problem.

Status:Needs review» Needs work

The last submitted patch, 68: 2080823.68.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new135.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,099 pass(es), 44 fail(s), and 6 exception(s).
[ View ]

Okay for now I've moved the node search display modes from the search module to the node module since this is where the plugin is. Discussed with @Gabor, @catch - potentially the only solution will be to have a node_search module.

The patch attached implements all of the dependencies in the table in the issue summary including theme dependencies and the removal of the all the breakpoint code to manage dependencies (see the lovely diff of breakpoint.module)

Attaching missing interdiff

StatusFileSize
new19.73 KB

#71 is a lie

StatusFileSize
new916 bytes
new134.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,095 pass(es), 44 fail(s), and 6 exception(s).
[ View ]

Removed the work in ConfigImporter as this should be done in #2030073: Config cannot be imported in order for dependencies.

One point to note:

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -169,6 +169,11 @@ public function disable(array $theme_list) {
+    // Remove configuration.
+    foreach ($theme_list as $key) {
+      \Drupal::service('config.manager')->uninstall('theme', $key);
+    }

We need to remove theme config on disable since we can get into config mismatch issues if a theme is upgraded and it's configuration is left lying around. Plus this allows us to keep active configuration nice and clean.

Issue summary:View changes

Known outstanding issues:

  • Review UI impact on the module uninstall confirm form
  • Add UI to theme disable confirm form
  • Test coverage
  • Views still do not have all their dependencies since discovering all the plugins involved in a view is tricky. @Damiankloip is looking at this
  • Form and view displays do not yet depend on their widget/formatter plugins

The last submitted patch, 70: 2080823.70.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 73: 2080823.73.patch, failed testing.

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new8.74 KB
new140.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,112 pass(es), 1 fail(s), and 511 exception(s).
[ View ]

Patch fixes

  • the custom block tests - this is not a config entity - custom block type is but that has no dependencies.
  • removes assumption that teaser view mode exists from node_add_body_field() - the teaser mode is created by Standard profile
  • fixes a block tests for new dependency on theme

It also dependencies on plugin providers to entity view/form displays based on the formatter/widget they are using.

Ha I thought there must be a theme disable confirm form - but nope there is not. Maybe adding one should be a critical follow up.

Issue summary:View changes
StatusFileSize
new1.66 KB
new141.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,088 pass(es), 1 fail(s), and 508 exception(s).
[ View ]

The current patch ensures that theme configuration and configuration entities dependent on the theme is removed when the theme is disabled. However there is no confirmation form to inform the user that data is going to be deleted. So I think this needs to be removed and left to #1067408: Themes need an installation status. In HEAD breakpoint currently removes config on theme disabling - this is incorrect. This patch will remove this and create dependencies on the theme.

Status:Needs review» Needs work

The last submitted patch, 79: 2080823.79.patch, failed testing.

The last submitted patch, 77: 2080823.77.patch, failed testing.

The last submitted patch, 77: 2080823.77.patch, failed testing.

StatusFileSize
new1.11 KB
new141.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,132 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Should be back to green. Removed too much of the CustomBlock entity so restored that... and entity_test_install() was creating a component on a form display for a field type that does not exist :)

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 83: 2080823.83.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.06 KB
new142.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,162 pass(es).
[ View ]

This time green for real - some more non existent plugins being used!!! This patch is going to keep everyone honest. No more assumptions allowed - if you have an unmet dependency you'll know :)

Issue summary:View changes
StatusFileSize
new66.59 KB

This patch looks amazing. I summarized the changes in this visual. Green things are new, blue things are changed old. The "paper slip" is about actual config changes. Actual document at https://docs.google.com/drawings/d/1MZcID-iTsQaDhYe_caBNU4-qbmAS2mS8zy3i... Added to the issue summary.

Issue summary:View changes

The current @todo list:

  • Decide whether dependency management occurs in preSave - maybe dependency calculation should be a separate method - this might make it easier to ensure the entire dependency array is recalculated on save. At the moment Views::preSave and others set dependencies to an empty array - I think we need to mandate this.
  • Views dependencies are not complete
  • Decide whether we have enough test coverage
  • Reviews

Regarding the todo: +1 on a separate method

Here's a first review, I'm not able to commit the small nitpicks in the sandbox today, the earliest on monday.

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/MultiFormTest.php
    @@ -48,7 +48,7 @@ function setUp() {
           ->save();

    I'm surprised by this change, text_default exists, or did you just do this because of the RebuildTest to make them consistent ?

  2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -105,25 +106,44 @@ public function installDefaultConfig($type, $name) {
    +      // Only import new config. However allow updates of config that is going

    Needs a comma after 'however' ?

  3. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -125,6 +134,16 @@ public function createSnapshot(StorageInterface $source_storage, StorageInterfac
    +    // Reverse the array to that entities are removed in the correct order of
    +    // dependence. This ensures that field instances are removed before fields.
    +    foreach (array_reverse($dependent_entities) as $entity) {
    +      $entity->setUninstalling(TRUE);

    We can probably remove the array_reverse now, since the introduction of setUninstalling()? Also, the comments makes it feel like we're only doing this for fields and instances only ;)

  4. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -137,4 +156,55 @@ public function uninstall($type, $name) {
    +      // It is possible that the a non configuration entity will be returned if

    'the a non' - ditch the 'the'

  5. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
    @@ -68,4 +68,35 @@ public function createSnapshot(StorageInterface $source_storage, StorageInterfac
    +   * Finds config entities that are dependent on the supplied extensions or entities.

    exceeds 80 chars

  6. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
    @@ -68,4 +68,35 @@ public function createSnapshot(StorageInterface $source_storage, StorageInterfac
    +   * Finds config entities that are dependent on the supplied extensions or entities.

    Same here

  7. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -268,4 +309,40 @@ public function url($rel = 'edit-form', $options = array()) {
    +    // explicitly declared the dependency.

    'declare'

  8. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityDependency.php
    @@ -0,0 +1,107 @@
    +   * Determines if the entity is dependent on the supplied extensions or entities.

    80 chars

  9. +++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/WizardTest.php
    --- /dev/null
    +++ b/core/modules/comment/tests/modules/comment_test_views/config/node.type.page.yml

    Can't we just call drupalCreateContentType() which is added in a lot of other places ?

  10. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -150,6 +151,30 @@ public function id() {
    +    foreach ($this->content as $field_name => $component) {
    +      $field_instance = Field::fieldInfo()->getInstance($this->targetEntityType, $this->bundle, $field_name);

    Hmm, so this only adds them for field instances ? We should probably account to for 'extra fields' which also have provider.

  11. +++ b/core/modules/node/node.module
    @@ -446,12 +446,18 @@ function node_add_body_field(NodeTypeInterface $type, $label = 'Body') {
    +    // The teaser view mode is created by the Standard profile are therefore

    'and' instead of 'are'
    Other than that, having code relying or 'listening' to a profile seems just completely wrong to me.

+++ b/core/modules/node/node.module
@@ -446,12 +446,18 @@ function node_add_body_field(NodeTypeInterface $type, $label = 'Body') {
+    // The teaser view mode is created by the Standard profile are therefore
Other than that, having code relying or 'listening' to a profile seems just completely wrong to me.

Additionally on this part of the review, short brain dump of an idea that both me and Alex had separately and very briefly talked about in IRC a couple of days ago. I'm not 100% sure anymore whether it was because of this part of the patch, but it's surely related.

The idea is simple: default configuration entities (and maybe also just simple settings) simply don't come with a module that is also the provider, or in another module that would need the other as a dependency.

As a consequence,

  • 'default' configuration is just a 'lie' and something that simply doesn't exist ...
  • The only place to provide default config is in installation profiles or in 'default configuration modules', or 'feature' modules which you create yourself easily in projects etc.
  • our core modules would then become clean and have no idea whatsoever about 'default' configuration, which would make the code pasted above in node_add_body_field in the current patch obsolete, and imho also simply not done.

To me this feels almost like a natural evolution in the CMI process, not sure how others feel about that idea. Definitely not something to fix in this patch, but for a follow up, but this is the idea. so .. shoot :)

So that would make forum a feature module? It needs default fields, vocabularies etc. If I keep going with the views conversions, it would make even more sense.

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -105,25 +106,44 @@ public function installDefaultConfig($type, $name) {
    +      $dependency_manager = new ConfigDependencyManager();
    +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -137,4 +156,55 @@ public function uninstall($type, $name) {
    +    $dependency_manager = new ConfigDependencyManager();
    +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -0,0 +1,174 @@
    +      $graph_object = new Graph($graph);
    ...
    +      $config = new ConfigEntityDependency($name, $config);

    In our new OO Paradigm, isn't a concrete new bad karma? We're tied to this concrete implementation. Playing devil's advocate - could any of these use a factory/service? We already have new Graph() in core, so probably ignore that.

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -268,4 +309,40 @@ public function url($rel = 'edit-form', $options = array()) {
    +    // A config entity is always dependent on it's provider. There is no need to
    +    // explicitly declared the dependency.
    +    // @see \Drupal\Core\Config\Entity\ConfigEntityDependency::hasDependency()

    There is no need to explicitly declare the dependency. (declare not declared)

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -268,4 +309,40 @@ public function url($rel = 'edit-form', $options = array()) {
    +      $this->dependencies[$type] = array($name);
    ...
    +      $this->dependencies[$type][] = $name;
    ...
    +    $key = array_search($name, $this->dependencies[$type]);

    would it be faster to key these by name instead? Would make unsetting faster.

  4. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -725,14 +725,16 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
    +      \Drupal::service('config.manager')->uninstall('module', $module);

    out of scope, but ouch

  5. +++ b/core/modules/breakpoint/breakpoint.module
    @@ -39,106 +39,6 @@ function breakpoint_help($path, $arg) {
    - * Implements hook_themes_disabled().
    - *
    - * @param array $theme_list
    - *   An array of theme names.
    - *
    - * @see _breakpoint_delete_breakpoints()
    - *
    - * @todo: This should be removed if https://drupal.org/node/1813110 is resolved.
    - */
    -function breakpoint_themes_disabled($theme_list) {
    -  _breakpoint_delete_breakpoints($theme_list, Breakpoint::SOURCE_TYPE_THEME);
    -}
    -
    -/**
    - * Implements hook_modules_uninstalled().
    - *
    - * @param array $modules
    - *   An array of the modules that were uninstalled.
    - *
    - * @see _breakpoint_delete_breakpoints()
    - *
    - * @todo: This should be removed if https://drupal.org/node/1813110 is resolved.
    - */
    -function breakpoint_modules_uninstalled($modules) {

    oh yeah! awesome cleanup

  6. +++ b/core/modules/system/lib/Drupal/system/Form/ModulesUninstallConfirmForm.php
    @@ -121,6 +121,33 @@ public function buildForm(array $form, array &$form_state) {
    +    $dependent_entities = \Drupal::service('config.manager')->findConfigEntityDependentsAsEntities('module', $this->modules);
    ...
    +        $entity_type = \Drupal::entityManager()->getDefinition($entity_type_id);

    Can these be injected?

#1915272: Move _breakpoint_delete_breakpoints() into BreakpointGroupStorageController was refactoring that breakpoint delete code, since you're removing it wholesale should we just close that issue?

StatusFileSize
new12.61 KB
new145.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,165 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

#90 @swentel

1. This is form widget not a field formatter afaics there is no text_default.
2. Fixed by moving comments around.
3. Nope - it's important that entities are uninstalled in the order of reverse dependency. Fixed comment.
4. Fixed
5. Fixed
6. Fixed
7. Fixed
8. Fixed
9. Sure - fixed.
10. We don't store a type key for extra fields - if we did then this would work. However this did make me realise that we need to add dependencies for fields that are hidden. Discussed in irc with @swentel - he said "yeah, we'll need to store the module/provider in the definitions (of the extra fields) so instead of invokeAll, we'll have to get the implementations and then store the module" - feels like a followup to me.
11. Fixed

#91, #92 @swentel, @larowlan

Yes I think we should have as few default config entities as possible in core modules and just have everything in standard profile or feature modules. But that's a separate issue for this :)

#93 @larowlan

1. These are value objects I don't think there is any need to add additional complexity here by making it swappable / injected / or anything else :)
2. Fixed
3. This was how I did it originally but entity ids have dots in them which is extremely problematic for our Config::get()
4. Yep the moduleHandler is a mess
5. :)
6. Yep - fixed.

#94 @timplunkett

Agreed - I'll close that as a dupe of this.

Status:Needs review» Needs work

The last submitted patch, 95: 2080823.95.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new30.22 KB
new150.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed to login to test site.
[ View ]

Moved dependency calculation out of preSave() into a new calculateDependencies() method that is called during preSave(). Whilst doing this I realised that the only usages of the public methods createDependency() and removeDependency() are in tests. Additionally since calculateDependencies() ensures that the dependencies are completely recalcuated on save there is little point to having a public method to add a dependency and removeDependency() makes no sense at all.

@swentel prefers addDependency() over createDependency() and so do I.

This refactor highlighted a problem with previous patches whereby setting $this->dependencies to an empty array would not actually reset the dependencies :)

Status:Needs review» Needs work

The last submitted patch, 97: 2080823.97.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new328 bytes
new149.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,327 pass(es).
[ View ]

My custom htaccess snuck in

Title:Create API to discover content or config entities soft dependencies and use this to present a confirm form on module uninstallCreate API to discover content or config entities' soft dependencies and use this to present a confirm form on module uninstall

Retitling. It's missing a possessive apostrophe.

StatusFileSize
new7.91 KB
new151.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,359 pass(es).
[ View ]

Rerolled and prepared the ground for unit testing of all the calculateDependencies() implementations - which is what I'm working on atm.

StatusFileSize
new49.65 KB
new193.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,432 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Added unit tests for all the calculateDependencies implementations except View, Tour, EntityDisplayBase and PictureMapping configuration entities because those classes are more than a bit difficult to unit test.

todo list

  • Review, improve and test user facing text on module uninstall form
  • Add test assertions to cover missing unit test coverage for View, Tour, EntityDisplayBase and PictureMapping configuration entities

Status:Needs review» Needs work

The last submitted patch, 103: 2080823.103.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.49 KB
new195.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,343 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Fix fails in the CKEditorTest - a static caching issue.

Just had a quick read through the code...

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityDependency.php
    @@ -0,0 +1,107 @@
    +class ConfigEntityDependency {

    Should we not provide an interface for this, even though it might not be strictly pluggable?

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityDependency.php
    @@ -0,0 +1,107 @@
    +  /**
    +   * Gets the configuration entity's configuration object name.
    +   *
    +   * @return string
    +   *   The configuration object name.
    +   */
    +  public function getName() {
    +    return $this->name;
    +  }
    +
    +  /**
    +   * Gets the configuration entity's configuration dependency name.
    +   *
    +   * @return string
    +   *   The configuration dependency name for the entity.
    +   */
    +  public function getConfigDependencyName() {
    +    return $this->getName();
    +  }

    Doesn't seem to be much difference between the two, at least not from looking straight at the definitions.
    Is it meant to be just a semantical difference, even through internally it happen to be the same property? Maybe this could use an explanation in the doc blocks there.

  3. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigDependencyTest.php
    @@ -0,0 +1,169 @@
    +    $this->assertTrue(isset($dependents['system.site']), 'Simple configuration which ');

    which...? :)

Further, I think it would be good design to provide a set of higher interfaces starting from something like interface EntityDependencyInterface and abstract EntityDependencyBase that class ConfigEntityDependency could implement and inherit from. Because there's definitely some code in there that could be nicely reused by the contrib modules like Entity Dependency and Deploy.
But, that should not at all hold up this patch from moving forward as-is. I'd be happy to file that patch as a backward compatible feature request for Drupal 8.1 or 8.2 or so...

Status:Needs review» Needs work

The last submitted patch, 105: 2080823.105.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new196.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,420 pass(es).
[ View ]
  1. As this is a value object I haven't really seen the need. I'd rather wait to see how a backward compatible patch worked out.
  2. There was a point during this patch's life when the UUID was used in the ConfigDependencyName but this meant that we could not use the system for default configuration. So now they are the same - removing getName since the other function maps to ConfigEntityInterface::getConfigDependencyName()
  3. Fixed :)

The fail in #105 for a segfault in Drupal\update\Tests\UpdateContribTest

The patch attached also:

StatusFileSize
new15.68 KB
new209.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,512 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
  • Added tests for View, PictureMapping and EntityDisplay - completing the test coverage (afaik).
  • Added sorts to ConfigEntityBase::addDependency() to ensure consistent ordering of the dependencies values.

todo list

  • Review, improve and test user facing text on module uninstall form

Status:Needs review» Needs work

The last submitted patch, 109: 2080823.109.patch, failed testing.

This looks great, I am always happy when we can inform users better when doing destructive actions!

My only comment would be that the fieldset has no useful label, keep in mind that fieldsets should when possible always describe their contents. There are a couple solutions; "To be deleted configuration", "Configuration deletions", "Configuration that will be removed" etc.

Status:Needs work» Needs review
StatusFileSize
new165.08 KB
new42.34 KB
new6.02 KB
new212.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,523 pass(es).
[ View ]

Updated uninstall confirm form with suggestion from @Bojhan and added tests - nothing left @todo here!

Module uninstall confirmation form (closed)


Module uninstall confirmation form (open)

Issue summary:View changes

Status:Needs review» Needs work

The last submitted patch, 112: 2080823.112.patch, failed testing.

Status:Needs work» Needs review

112: 2080823.112.patch queued for re-testing.

TrackerTest failed failing to write a configuration file - so it looks like testbot diskspace.

StatusFileSize
new11.64 KB
new212.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,474 pass(es).
[ View ]

Only got part way through, but posting what I have so far:

  1. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -137,4 +157,55 @@ public function uninstall($type, $name) {
    +      // dependents of the system module are calculated since system.site has
    +      // a uuid key.

    Should system.site.uuid seems like it shouldn't be in config but rather $settings (with a comment never to change it). Or rename to site_uuid? I'm wondering whether we want to allow this at all or should just reserve the key. Problem isn't introduced here so just follow-up if we want to revisit that I think.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
    @@ -68,4 +68,35 @@ public function createSnapshot(StorageInterface $source_storage, StorageInterfac
    +   * Finds config entities that are dependent on extensions or entities.

    I got a bit thrown by 'module', 'theme', 'entity' as the three types of dependency. All dependencies boil down to these, but some are via plugins or similar that in turn are provided by a module (if we solve #2212151: Updating active configuration based on API changes (i.e. plugins) then it'd be possible for a plugin provider to move from one module to another - dependency would still be there, but for a different module).

    This is fine, but it feels like it's missing some high level documentation on how we got to those three kinds of dependency.

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -0,0 +1,174 @@
    +  protected function getGraph() {

    This means all the other methods have to call getGraph() instead of relying on the property. Why not move the logic to setData()?

    Either way, shouldn't setData() reset $this->graph (whether to null or the new graph)?.

StatusFileSize
new2.63 KB
new214.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,531 pass(es).
[ View ]
  1. Yep - I pondered about changing the UUID key in system.site - when we added the uuid to system.site the reason I chose config over settings was because I wanted to be be able to use existing configuration in the installer to create a synced site. I think this is still desirable even if it does not happen in core or Drupal 8 - so I vote for it staying in configuration - happy to change the name of it though. However have keys reserved only for configuration entities feels brittle. And I'm happy that we had this instance to catch this.
  2. This patch could do with a docs pass :) - I've added some documentation to the ConfigDependencyManager object about the dependency system.
  3. It is possible that the graph will not be needed - i.e. if you find dependencies for a module without any. Added a reset to the setData() method.

StatusFileSize
new214 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,552 pass(es).
[ View ]
new18.99 KB

Wow, wow, wow! Impressive work! I got through about 60% of the patch. I only fixed the things where I was certain my proposed change was correct.

  1. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -137,4 +157,55 @@ public function uninstall($type, $name) {
    +      // Group by entity type so we can use load multiple.

    "use load multiple" sounds very strange. Changing it to "use load_multiple" would already help, but is still not ideal. Not worth spending more time on though probably.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
    @@ -68,4 +68,35 @@ public function createSnapshot(StorageInterface $source_storage, StorageInterfac
    +   * Finds config entities that are dependent on extensions or entities.
    ...
    +   *   The type of dependency being checked. Either 'module', 'theme', 'entity'.

    I think "entity" is confusing, since it could be a content entity. We only allow dependencies on *config* entities. I understand that "entity" is shorter than "config_entity", but I think the latter would be better because it leaves no room for ambiguity.

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -0,0 +1,208 @@
    + * example, node types are created before their fields and the fields are

    s/their fields/fields are assigned to them/
    ?

  4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -0,0 +1,208 @@
    + * Dependencies are written to the configuration entity so that they can be

    What does it mean to have "dependencies written to the configuration entity"? Does that mean that configuration entity YML files are dynamically modified whenever a new module/theme/config entity is added to the system that depends on it?

    If so, I think that could be worded more clearly.

  5. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -0,0 +1,208 @@
    + * This method should be called from the entity's implementation of

    It says just "entity", so this could be either a config or content entity? If so, I think it'd be better if this were mentioned specifically.

  6. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -0,0 +1,208 @@
    +   * @var array

    Why not Drupal\Component\Graph\Graph?

  7. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -0,0 +1,208 @@
    +    if ($type == 'module' || $type ==  'theme') {
    +      $dependent_entities = array_filter($this->data, function (ConfigEntityDependency $entity)  use ($type, $name) {
    +        return $entity->hasDependency($type, $name);
    +      });
    +    }

    This block of code could be moved into the "else" block a few lines down, that would make the code much clearer. Or is there a particular reason for it to be this way?

  8. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -0,0 +1,208 @@
    +    // are last but entities with the same weight are alphabetically ordered in
    +    // the same way file system reads often are.

    The description here of what the specific alphabetical ordering is, is … special :P Can we make this less ambiguous?

  9. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -64,6 +64,20 @@
    +   * Whether the config is deleted through the uninstall process.

    s/is deleted/is being deleted/
    ?

  10. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -264,4 +318,46 @@ public function url($rel = 'edit-form', $options = array()) {
    +      if (count($this->dependencies) > 1) {

    Not >1, but >0 or >=1?

  11. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityDependency.php
    @@ -0,0 +1,99 @@
    + * Class ConfigEntityDependency

    Description missing.

  12. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
    @@ -92,6 +92,14 @@ public function status();
    +   * Returns whether the configuration entity is deleted through the import

    s/is deleted/is being deleted/
    ?

    s/import process/uninstall process/?

  13. +++ b/core/lib/Drupal/Core/Config/Schema/core.data_types.schema.yml
    @@ -136,3 +136,24 @@ route:
    +# Config dependencies.
    +config_dependency:

    Why make the key singular?

  14. +++ b/core/modules/block/tests/modules/block_test/config/block.block.test_block.yml
    @@ -18,3 +18,8 @@ visibility:
    +  theme:
    +    - stark

    Because this is a test block, it happens to be that we control that it indeed can only be used in the Stark theme. But I think this generally shouldn't be hardcoded, right?

  15. +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Tests/BreakpointThemeTest.php
    @@ -51,10 +51,6 @@ public function testThemeBreakpoints() {
    -
    -    // Disable the test theme and verify the breakpoint group is deleted.
    -    theme_disable(array('breakpoint_test_theme'));
    -    $this->assertFalse(entity_load('breakpoint_group', $breakpoint_group_obj->id()), 'breakpoint_group_load: Loading a deleted breakpoint group returns false.', 'Breakpoint API');

    Why disable this and another similar assertion further down?

  16. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -143,6 +144,40 @@ public function id() {
    +  public function calculateDependencies() {

    I think one thing is still missing here: dependencies on modules that provide formatters and widgets, which could be contrib modules.

  17. +++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.php
    @@ -287,6 +284,20 @@ public function testRenameDeleteBundle() {
    +      'module' => array('Core', 'text')

    "Core" is a module?

  18. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -273,11 +274,13 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
    -      return $this->preSaveNew($storage_controller);
    +      $this->preSaveNew($storage_controller);

    Good catch :)

StatusFileSize
new215.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,555 pass(es).
[ View ]
new15.11 KB

Thank @Wim Leers!

  1. Tried to improve this in attached patch
  2. At some point it is likely that we will have configuration entities depend on content entities and vica versa. So your point is a good one. At one point in the patch I had the config dependency name be entity_type:UUID but using the configuration entity configuration name means that this would no longer support content entities. Hmmm what to do? I guess we could swap to config_entity as a key - let's see how others feel.
  3. This paragraph is outlining the order that config creations during an import will occur and fields are only related to 1 node type (or whatever) but the point is the fields are theirs :)
  4. Changed to Dependencies are stored to the configuration entity's configuration object. If a installing a module or theme would add dependencies to existing configuration entities just be being installed then it would need to re-save the configuration entities itself - this should be very rare indeed.
  5. Agreed
  6. Because it is an array not an instance of Drupal\Component\Graph\Graph :)
  7. Good point - it was this way because I tackled module dependency first
  8. Sure
  9. Fixed
  10. Only need to sort if we have 2... sorting 1 is... pointless :)
  11. Fixed
  12. Fixed
  13. Good point - fixed
  14. This is a block instance it has to be tied to a theme which it is.
  15. Because breakpoint was incorrectly removing theme configuration on theme disable - we don't do this. Theme configuration removal is dependent on #1067408: Themes need an installation status so we can uninstall them.
  16. That functionality is there... added a comment to make this explicit.
  17. It provides the string field - this is tricky - unsure of what to do here. We could special case Core in the addDependency code and say it's silly to add a dependency on it.
  18. :)

Went through this, quite a patch ;) Make sure to read everything first, I found answers to my own questions myself in some cases.

I also didn't read anything in the issue and only skimmed the issue summary. Overall, this looks good, only wondering about the performance impact of all the entity loading that's going on. The abstraction of he dependency ID is nice, but it's also kind of stupid that we have to load all these entities just to get a string with the config prefix and id back? Could this maybe be a static method, or something somewhere else? Is there a reason to change that ID?

Also, thinking a bit more about performance, it shouldn't be necessary to calculate dependencies during a config sync, should it? That means we could wrap it in a !isSyncing() and avoiding to recalculate in that case?

  1. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -137,4 +157,56 @@ public function uninstall($type, $name) {
    +    // is a config entity. Only configuration entities can be depended on so we
    +    // can ignore everything else.
    +    $data = array_filter($this->activeStorage->readMultiple($this->activeStorage->listAll()), function($config) {
    +      return isset($config['uuid']);
    +    });
    +    $dependency_manager->setData($data);

    At least the system.site.yml has a uuid too, is that a problem?

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -0,0 +1,206 @@
    +   *
    +   * @param array $entities_to_check
    +   *   The entities to supply dependencies for.

    This is an entity type, not an entity, correct? Maybe use $entity_type_ids or something instead?

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
    @@ -0,0 +1,206 @@
    +   *
    +   * @return self

    return $this.

  4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -241,6 +272,29 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
    +    // @todo When \Drupal\Core\Config\Entity\EntityWithPluginBagInterface moves
    +    //   to a trait, switch to class_uses() instead.
    +    if ($this instanceof EntityWithPluginBagInterface) {

    Wouldn't that mean that we're then checking for a specific implementation and not an "interface"? Why not do a method_exists() instead?

  5. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
    @@ -92,6 +92,14 @@ public function status();
       /**
    +   * Returns whether the configuration entity is being deleted by the uninstall
    +   * process.
    +   *
    +   * @return bool
    +   */
    +  public function isUninstalling();

    Is this something that modules need to check?

    If yes, it should explain what that means, just like isSyncing() should (we spent a lot of time on that in https://drupal.org/node/2157467). If not here, then somewhere else.

  6. +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/BreakpointGroupInterface.php
    @@ -51,7 +51,7 @@ public function addBreakpoints($breakpoints);
        *
    -   * @return array
    +   * @return \Drupal\breakpoint\Entity\Breakpoint[]
        *   The array of breakpoints for the breakpoint group.

    Should use the interface when changing it anyway..

  7. +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.php
    @@ -91,7 +92,7 @@ class BreakpointGroup extends ConfigEntityBase implements BreakpointGroupInterfa
       /**
        * Overrides Drupal\config\ConfigEntityBase::__construct().
        */
    -  public function __construct(array $values, $entity_type) {
    +  public function __construct(array $values, $entity_type = 'breakpoint_group') {
         parent::__construct($values, $entity_type);
       }

    Not sure why we are doing this here? Seems like the method is unnecessary anyway?

  8. +++ b/core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointGroupConfigEntityUnitTest.php
    @@ -0,0 +1,159 @@
    +    // procedural code.
    +    $this->entity = $this->getMockBuilder('\Drupal\breakpoint\Entity\BreakpointGroup')
    +      ->setConstructorArgs(array($values, $this->entityTypeId))
    +      ->setMethods(array('getBreakpoints'))
    +      ->getMock();

    We are setting it here, so it shouldn't be necessary?

  9. +++ b/core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointGroupConfigEntityUnitTest.php
    @@ -0,0 +1,159 @@
    +    $breakpoint = $this->getMockBuilder('\Drupal\breakpoint\Entity\Breakpoint')
    +                       ->disableOriginalConstructor()->getMock();

    Would using the interface here make this easier?

  10. +++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/WizardTest.php
    @@ -33,6 +33,11 @@ public static function getInfo() {
    +  public function setUp() {
    +    parent::setUp();
    +    $this->drupalCreateContentType(array('type' => 'page', 'name' => t('Basic page')));
    +  }
    +

    Minor: Should we move the addDefaultField() call to here as well, so that those two things are together just like in CommentTestBase?

    This also needs an @inheritdoc now..

  11. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigDependencyTest.php
    @@ -0,0 +1,170 @@
    +/**
    + * @file
    + * Contains Drupal\config\Tests\ConfigDependencyTest.

    Leading \ missing. Sorry ;)

  12. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigDependencyTest.php
    @@ -0,0 +1,170 @@
    +    $this->installConfig(array('system'));
    +    $config_manager = \Drupal::service('config.manager');
    +    $dependents = $config_manager->findConfigEntityDependents('module', array('system'));
    +    $this->assertTrue(isset($dependents['system.site']), 'Simple configuration system.site has a UUID key even though it is not a configuration entity and therefore is found when looking for dependencies of the System module.');
    +    // Ensure that calling
    +    // \Drupal\Core\Config\ConfigManager::findConfigEntityDependentsAsEntities()
    +    // does not try to load system.site as an entity.
    +    $config_manager->findConfigEntityDependentsAsEntities('module', array('system'));

    Ah, I guess that answers my own question, this is the expected behavior then.

    Is there anything we can assert here, or are we simply relying on it throwing exceptions when it would do the wrong thing?

  13. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -143,6 +144,42 @@ public function id() {
    +      // If the target entity type uses bundles then depend on the bundle entity.
    +      $bundle_entity = \Drupal::entityManager()->getStorageController($bundle_entity_type_id)->load($this->bundle);
    +      $this->addDependency('entity', $bundle_entity->getConfigDependencyName());

    It's not *strictly* required that bundles are an entity type (entity_test uses simple state variables). Maybe reword the comment a bit so that it says something like "... uses entities to manage its bundles" or so?

  14. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -143,6 +144,42 @@ public function id() {
    +      // Create a dependency on the module that provides the formatter or
    +      // widget.
    +      if (isset($component['type'])) {
    +        $definition = $this->pluginManager->getDefinition($component['type']);
    +        $this->addDependency('module', $definition['provider']);

    We now have formatters and widgets in Core, e-mail for example. This would then add module: Core I think?

  15. +++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.php
    @@ -188,16 +191,10 @@ public function testFieldComponent() {
    -    // Check that specifying an unknown formatter (e.g. case of a disabled
    -    // module) gets stored as is in the display, but results in the default
    -    // formatter being used.
    -    $display->setComponent($field_name, array(
    -      'type' => 'unknown_formatter',
    -    ));
    -    $options = $display->getComponent($field_name);
    -    $this->assertEqual($options['type'], 'unknown_formatter');
    -    $formatter = $display->getRenderer($field_name);
    -    $this->assertEqual($formatter->getPluginId(), $default_formatter);
    +    // Check that the display has dependencies on the field and the module that
    +    // provides the formatter.
    +    $dependencies = $display->calculateDependencies();

    So far, this was explicitly supported as the formatter/widgets was considered a non-required dependency, there was a recent comment from someone (swentel?) about that somewhere, but can't find it right now.

    Not saying that we can't change that or shouldn't, just pointing out that this explicitly like this, unlike field type providers.

  16. +++ b/core/modules/language/lib/Drupal/language/Entity/Language.php
    @@ -96,7 +96,7 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
         foreach ($entities as $entity) {
    -      if ($entity->id() == $default_language->id) {
    +      if ($entity->id() == $default_language->id && !$entity->isUninstalling()) {
             throw new DeleteDefaultLanguageException('Can not delete the default language');
           }

    k, there are cases where it needs to be checked :)

    Any chance to unify those two flags/methods? $entity->iKnowWhatImDoing() ;)

  17. +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/EmailFieldRdfaTest.php
    @@ -53,7 +53,7 @@ public function setUp() {
       public function testAllFormatters() {
         // Test the plain formatter.
    -    $this->assertFormatterRdfa('text_plain', 'http://schema.org/email', $this->testValue);
    +    $this->assertFormatterRdfa('string', 'http://schema.org/email', $this->testValue);
         // Test the mailto formatter.

    Huh.... ah, the default fallback. Nice.

  18. +++ b/core/modules/views/lib/Drupal/views/Entity/View.php
    @@ -22,7 +22,7 @@
      *   label = @Translation("View"),
      *   controllers = {
    - *     "storage" = "Drupal\views\ViewStorageController",
    + *     "storage" = "Drupal\Core\Config\Entity\ConfigStorageController",
      *     "access" = "Drupal\views\ViewAccessController"
      *   },

    This is the default storage controller for @ConfigEntityType, so you can remove the explicit definition.

  19. +++ b/core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.php
    @@ -81,7 +81,7 @@ function testConfigurationEntityCRUD() {
         $this->assertTrue($this->entityType instanceof EntityTypeInterface, 'The View info array is loaded.');
         // Confirm we have the correct controller class.
    -    $this->assertTrue($this->controller instanceof ViewStorageController, 'The correct controller is loaded.');
    +    $this->assertTrue($this->controller instanceof ConfigStorageController, 'The correct controller is loaded.');

    Seems like this test is now pretty useless, why not just remove the assertion?

  20. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
    @@ -149,6 +149,13 @@ class ViewUI implements ViewStorageInterface {
       /**
    +   * Whether the config is being deleted through the uninstall process.
    +   *
    +   * @var bool
    +   */
    +  private $isUninstalling = FALSE;

    Wasn't sure why we bother to actually test this, but it's just following isSyncing. Not really necessary, you're adding other empty methods too

StatusFileSize
new14.48 KB
new219.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,491 pass(es).
[ View ]

Performance

With regard to loading everything - we have to load everything to get the dependencies array too - I think this is unavoidable. You are right we don't have to recalculate during an import - nice catch. Implemented this in the patch attached.

Numbered review

  1. It is a handled problem. @catch has proposed changing the UUID key name in system.site - however I have added tests to ensure that a UUID key in a simple config file does not break everything.
  2. This is an array of full configuration names for the entities you want to check ie. config_prefix + . + entity ID
  3. fixed
  4. This is copied from the @todo in ConfigEntityBase::preSave()
  5. fixed
  6. fixed
  7. A lot of other entity types provide a default type argument - it's done here to make the ResponsiveImageMappingEntityTest mock of the breakpoint group easy
  8. See answer to #7
  9. Yep fixed
  10. fixed
  11. fixed
  12. Yep we throw an exception so there is nothing to assert
  13. fixed
  14. Yes discussed with @Wim Leers too - patch attached adds logic to prevent dependencies on Core and tests it.
  15. Hmmm I understood that this code was protecting us if a module that supplied a formatter or widget was uninstalled - this would now remove the display.
  16. :) maybe in a follwoup
  17. This patch keeps everything honest :)
  18. fixed
  19. fixed
  20. The whole ViewsUI -> ViewStorageInterface -> ConfigEntityInterface is very ugly but is out-of-scope here.

Reviewed the change notice, added the missing reference to this issue, looks great! (just made the code snippets use php tags to have code highlighting).

Did some performance tests to be able to understand the performance cost of this change. I used view displays as we have a few of them and they have lots of dependencies:

<?php
use Drupal\Component\Utility\Timer;
$displays = entity_load_multiple('entity_view_display');
Timer::start('save');
$count = count($displays);
foreach (
$displays as $display) {
 
$display->save();
}
$end = Timer::stop('save');
print
"Resaved " . count($displays) . ' displays in ' . $end['time'] . "ms\n";
?>

$ drush scr config_save.php

HEAD:
Resaved 9 displays in 22.74ms
Resaved 9 displays in 23.79ms
Resaved 9 displays in 21.89ms
Resaved 9 displays in 21.2ms
Resaved 9 displays in 23.16ms
Resaved 9 displays in 24.01ms
Resaved 9 displays in 20.94ms

With Patch:
Resaved 9 displays in 41.39ms
Resaved 9 displays in 39.76ms
Resaved 9 displays in 42.12ms
Resaved 9 displays in 40.16ms
Resaved 9 displays in 41.91ms
Resaved 9 displays in 37.86ms

So this does make it twice as slow on my laptop (someone wants to try this on a different system?)

I did have a look at xhprof, most of the time is spent loading additional config entities, I also had some cache writes due to list calls, the problem there is that after every write, we reset the whole list cache, so the next display entity has to update that again. This is actually caused by the existing UUID check in ConfigEntityBase::presave(), but EntityDisplayBase previously didn't call parent::preSave(). And that call is more expensive than the new calculateDependencies() method. So EntityViewDisplay was a bit an unfair example but on the other side, that is the actual regression for them.

Anyway, I guess there isn't really anything to optimize, other maybe looking into the config list cache and performance of loading config by UUID but those are pre-existing issues in HEAD that we just happen to run into here.

+1 to RTBC from my side, have nothing else to complain about. This will need an OK from other reviews as well, I don't have an overall grasp of the whole thing here.

One last thing here: The migration system currently has it's own dependency system, which does work differently. Should we refactor that based on this later? It has advanced concepts like optional dependencies (run first when existing, but ignore otherwise)? It applies to running migrations, not copying them around, so probably should stay separate? Either way, it will result in a conflict because it also uses "dependencies" right now as the key.

123: 2080823.123.patch queued for re-testing.

So the current us of dependencies actually does not block this patch since the Migrate API and migrations in 8.x do not use it. @sun has suggested changing the key we add on this issue to requires. I'm not a fan of this at the moment since we still have dependencies in .info.yml.

It would be fantastic to proceed with this patch asap.

Status:Needs review» Reviewed & tested by the community

It looks good from my side as well. Based on the above reviews as well, it look all good to go now :)

(Note I reviewed the whole architecture above already and created the figure explaining what it does in the issue summary :) All concerns are resolved and this has a verrrry long dependency chain as well.)

StatusFileSize
new219.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,501 pass(es).
[ View ]
new1023 bytes

Tiny patch context conflict with #2219925: Rename ConfigEntityInterface::getExportProperties() to toArray() in ConfigEntityInterface.

Status:Reviewed & tested by the community» Fixed

I've looked at this several times now and I think it's good as well. We might want to open a separate issue to discuss isSyncing vs. isUninstalling (and I still feel like the language presave preventing delete is heavy-handed), but none of these are really new issues, and this unblocks lots and lots and lots of other issues.

Committed/pushed to 8x, thanks!

  • Commit 8cbab14 on 8.x by catch: Issue #2080823 by alexpott, swentel, Wim Leers: Create API to discover...

Hurray! :) Awesome work, Alex!

#2007248: When CKEditor module is uninstalled, the right text editor config entities are affected but their labels are missing was solved automatically thanks to this issue. However, I did find one small problem there: text editor entities slated for deletion are found correctly, but their labels are missing. I will fix it in that issue though, and that of course doesn't warrant a rollback. It's just an FYI, because others might encounter the same problem elsewhere.

Status:Fixed» Closed (fixed)

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