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

Comments

tim.plunkett’s picture

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

Status: Active » Needs review
FileSize
1.16 KB

Duh, forgot the patch

Berdir’s picture

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

alexpott’s picture

Issue tags: +Configurables

Tagging

tim.plunkett’s picture

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

@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

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

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

FileSize
1.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,417 pass(es). View
1.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,330 pass(es). View

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

FileSize
7.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,425 pass(es). View

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

FileSize
7.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,421 pass(es). View

Doc tweaks to explain that this is not optional.

tim.plunkett’s picture

FileSize
1.47 KB

Ugh, missed interdiff

dawehner’s picture

  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

  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

FileSize
1.17 KB
7.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,360 pass(es). View

Good idea!

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

beejeebus’s picture

Status: Needs review » Reviewed & tested by the community

i think this is ready :-)

catch’s picture

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.