Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#15 | config-import-2218003-15.patch | 7.85 KB | tim.plunkett |
#15 | interdiff.txt | 1.17 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettWe 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...
Comment #2
tim.plunkettDuh, forgot the patch
Comment #3
BerdirI think there's a related issue about config entities being tied to config storage controller...
Comment #4
alexpottTagging
Comment #5
tim.plunkettBerdir, 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? ;)
Comment #6
alexpott@tim.plunkett I didn't disagree with the direction :) and I certainly did not mean to delete it :)
Doing this means that the config object will be updated by the config system instead :( see
ConfigImporter::process()
. Thinking about it theConfigImporter::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.EDIT: for clarity of dodgyness
Comment #7
sunThose 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?Comment #8
tim.plunkettThose 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.
Comment #9
tim.plunkettWell, 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.
Comment #10
tim.plunkettThis 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.
Comment #11
tim.plunkettDoc tweaks to explain that this is not optional.
Comment #12
tim.plunkettUgh, missed interdiff
Comment #13
dawehnerIt would be great to explain which entity storage / which entity type does not support imports.
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.
Comment #14
dawehnerIt would be great to explain which entity storage / which entity type does not support imports.
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.
Comment #15
tim.plunkettGood idea!
The entity storage "Drupal\foo\FooStorage" for the "filter_format" entity type does not support imports
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedi think this is ready :-)
Comment #17
catchCommitted/pushed to 8.x, thanks!