Updated: Comment #N

Problem/Motivation

The title might read a bit odd, because it might seem like a reasonable assumption to make, but we don't enforce that anywhere.

Proposed resolution

Check the interface before calling importCreate/importDelete/importUpdate

Remaining tasks

Decide if we want to force this somewhere instead (like in \Drupal\Core\Config\Entity\ConfigEntityType maybe?)

User interface changes

N/A

API changes

Not yet, but maybe

CommentFileSizeAuthor
#15 config-import-2218003-15.patch7.85 KBtim.plunkett
#15 interdiff.txt1.17 KBtim.plunkett
#12 interdiff.txt1.47 KBtim.plunkett
#11 config-import-2218003-11.patch7.67 KBtim.plunkett
#10 config-import-2218003-10.patch7.34 KBtim.plunkett
#9 config-importer-2218003-9.patch1.74 KBtim.plunkett
#9 config-import-2218003-1.patch1.16 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Related issues: +#2208617: Add key value entity storage

We don't have another suitable entity storage controller that could be used to prove this is broken, but #2208617: Add key value entity storage adds one...

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
config-import-2218003-1.patch1.16 KB

Duh, forgot the patch

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

I think there's a related issue about config entities being tied to config storage controller...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue tags: +Configurables

Tagging

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Berdir, that was the original title of #1862300: Move ConfigStorageController::getQuery() to EntityStorageControllerInterface, which was reduced in scope. But I missed this part.

alexpott, I know you disagreed with the direction of the patch, but deleting it? ;)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

@tim.plunkett I didn't disagree with the direction :) and I certainly did not mean to delete it :)

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -315,7 +316,8 @@ protected function importInvokeOwner($op, $name) {
+      $handled_by_module = $entity_storage instanceof ConfigStorageControllerInterface ? $entity_storage->$method($name, $new_config, $old_config) : FALSE;

Doing this means that the config object will be updated by the config system instead :( see ConfigImporter::process(). Thinking about it the ConfigImporter::process() code looks extremely dodgy. I don't think if the configuration entity system does not implement the ConfigStorageControllerInterface then we should just jam the changes in using the simple config system.

  protected function process($op, $name) {
    if (!$this->importInvokeOwner($op, $name)) {
      $this->importConfig($op, $name);
    }
  }

EDIT: for clarity of dodgyness

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Those methods look really misplaced on the ConfigStorageControllerInterface — I don't see how invoking import callbacks/hooks is related to a storage controller at all?

To my knowledge, ConfigImporter was only introduced later on.

Why don't we simply move them entirely into ConfigImporter? At least that sounds like a much more appropriate place?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Those methods are the new version of hook_config_import_*(), changed in https://drupal.org/node/1806178. Those hooks were invoked from the original ConfigStorageController, IIRC.

Of the 3 methods, only importDelete() is ever overridden, by FieldInstanceConfigStorageController. That would need to be resolved first...

And at that point, ConfigImporter would need to check for ConfigEntityInterface first. And still call out to the storage controller to load or create...

I'm not sure that changing that flow is in scope here.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
config-import-2218003-1.patch1.16 KB
config-importer-2218003-9.patch1.74 KB

Well, this is what it would look like if we don't fall through to vanilla config import if the name matches an entity type.

I think either this or #2 are all that's in scope for this issue, and further changes here should be a follow-up.

Uploading both.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
config-import-2218003-10.patch7.34 KB

This is after a discussion with @DamZ and @beejeebus in IRC.

We move the import* methods to a new interface, and check that.
If it's not implemented, then we throw an exception.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
config-import-2218003-11.patch7.67 KB

Doc tweaks to explain that this is not optional.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
interdiff.txt1.47 KB

Ugh, missed interdiff

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -315,9 +319,13 @@ protected function importInvokeOwner($op, $name) {
+        throw new EntityStorageException('The entity storage does not support imports');

    It would be great to explain which entity storage / which entity type does not support imports. 

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ImportableEntityStorageInterface.php
@@ -0,0 +1,56 @@
+ * Contains \Drupal\Core\Config\Entity\ImportableEntityStorageInterface.

    For a short short moment I thought that this interface could be also usable for something like menu items, served from some static yml file, though yeah nevermind.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -315,9 +319,13 @@ protected function importInvokeOwner($op, $name) {
+        throw new EntityStorageException('The entity storage does not support imports');

    It would be great to explain which entity storage / which entity type does not support imports. 

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ImportableEntityStorageInterface.php
@@ -0,0 +1,56 @@
+ * Contains \Drupal\Core\Config\Entity\ImportableEntityStorageInterface.

    For a short short moment I thought that this interface could be also usable for something like menu items, served from some static yml file, though yeah nevermind.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
interdiff.txt1.17 KB
config-import-2218003-15.patch7.85 KB

Good idea!

The entity storage "Drupal\foo\FooStorage" for the "filter_format" entity type does not support imports

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community

i think this is ready :-)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit 9f07ce4 on 8.x by catch: 
    Issue #2218003 by tim.plunkett: ConfigImporter assumes that a...

Status: Fixed » Closed (fixed)

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