Updated: Comment #N
Problem/Motivation
ConfigStorageController::save() has this code:
if ($id !== $entity->id()) {
// Renaming a config object needs to cater for:
// - Storage controller needs to access the original object.
// - The object needs to be renamed/copied in ConfigFactory and reloaded.
// - All instances of the object need to be renamed.
$config = $this->configFactory->rename($prefix . $id, $prefix . $entity->id());
}
This happens *before* EntityInterface::preSave() is called, which prevents any checks for the existing config, to avoid overwriting it.
However, moving this after the preSave() call breaks ConfigEntityBase::preSave(), which makes false assumptions when checking for existing entities.
This made it impossible to write an alternate entity storage that didn't also move (or delete!) the original entity before this check was run.
Proposed resolution
Move the renaming code in ConfigStorageController after preSave, right before the actual saving.
Fix the logic in ConfigEntityBase::preSave() to allow renames
Remaining tasks
N/A
User interface changes
N/A
API changes
Internal bug fixes, no API changes
Comment | File | Size | Author |
---|---|---|---|
#1 | config-entity-rename-2217975-1-FAIL.patch | 1.31 KB | tim.plunkett |
#1 | config-entity-rename-2217975-1-PASS.patch | 2.24 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettThe diff looks weird because it doesn't show the code I actually moved (the rename) but instead looks like I moved the preSave code. Same difference.
The failing patch just fixes the rename part without adjusting ConfigEntityBase's logic.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedlooks good to me, doing storage operations before preSave is nasty.
Comment #4
tim.plunkettFAIL patch failed, PASS patch passed.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedyay, timplunkett++
Comment #6
sunGreat find + fix!
It looks like we do not have any proper test coverage for these internal code paths and logic flows at all yet, and adding a dedicated/special test to just assert this
preSave()
aspect would be way too narrow.Also note that the fail patch in #1 actually failed.
Therefore, let's move forward here without test coverage. We should establish proper test coverage on the expected code flows of all config entity operations in a separate issue instead.
Comment #7
webchickHm. Really not crazy about putting this in without test coverage. Let's get a major follow-up to add these in a holistic way.
Committed and pushed to 8.x.
Comment #8
tim.plunkettAdded #2220451: Expand coverage of config entity renames, thanks!
Comment #9
tim.plunkettMeant to do this as well