Problem/Motivation
ConfigStorageController
allows a new entity to override an existing entity.
This can cause problems with modules, for example, if two installed modules provide the same default configuration, one of the configuration entities will be silently overwritten. The most immediate effect of making this change, is that various tests have had to be modified to accomodate this desired behavior:
BreakpointThemeTest, ConfigEntityTest, ConfigImportTest, ViewStorageTest, FieldTypeTest
Proposed resolution
ConfigStorageController
will not allow saving a "new" entity that overrides an existing entity. A developer would need to test whether or not that ID exists before performing an entity_create()->save(). If they don't an error message should be returned with an entity ID.
Work has been done to make sure that these calls were being correctly tested. A series of patches had failed to pass tests.
Remaining tasks
Patch conflicts with ModuleApiTest::testDependencyResolution() #1776830: Installation and uninstallation of configuration provided by a module that belongs to another module's API
API changes
To override attributes of an entity, a developer should use
$entity = entity_load('foo', 'foo'));
$entity->set('label', 'bar');
$entity->save();
Related Issues
- #1871696: Convert block instances to configuration entities to resolve architectural issues
- #1888218: Default configuration entities provided by a module should include the module name in the machine name
- #1889620: config_install_default_config() overwrites existing configuration
- #1609418: Configuration objects are not unique
- #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API
- #1998664: An existing block instance will be overwritten by a newly created block instance if the two share the same machine_name
Original report by tim.plunkett
Comment | File | Size | Author |
---|---|---|---|
#61 | interdiff-1887654-61.txt | 567 bytes | damiankloip |
#61 | 1887654-61.patch | 10.97 KB | damiankloip |
#46 | conf-1887654-46.patch | 3.36 KB | Berdir |
#35 | conf-1887654-35.patch | 11.45 KB | marthinal |
#30 | config-1887654-30.patch | 12.18 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettTagging.
Comment #2
tim.plunkettComment #4
xjmFor configurables, entity_create() wraps ConfigStorageController::create(), which has the comment:
This comment was introduced in #1798944: Convert config_test entity forms to EntityFormController (strange enough place to begin with). The patch in that issue originally had the following
@todo
:In #37 of that issue @catch asked that a followup be filed for the
@todo
, but instead the comment was simply removed.This bug is also causing oddities for testing CRUD hooks for #1871696: Convert block instances to configuration entities to resolve architectural issues.
I think
entity_create()
should fail if the entity exists already.Comment #5
tim.plunkettCurious about sun's thoughts here.
Comment #7
sunLooks good to me. Can we output the entity ID (without config prefix) in the exception message?
Comment #8
xjmThis bug also causes configuration entities to be silently overwritten if two installed modules provide the same default configuration. Here's an additional test. I also filed #1888218: Default configuration entities provided by a module should include the module name in the machine name.
I didn't try to fix the exceptions or #7; just noticed this in the process of filing blocks followups.
Comment #9
xjmComment #10
xjmOh, this should also be inside a try/catch.
Comment #12
tim.plunkettBreakpointThemeTest was manually entity_create-ing all of its default config (identical data), which was just a waste.
ConfigEntityTest had a whole section to ensure the exact broken behavior!
ConfigImportTest was missing the try/catch as xjm pointed out in #10
ViewStorageTest was creating a duplicate, but that wasn't needed for the intention of the assertion
FieldTypeTest was using views.view.test_field_type.yml, which was copied instead of moved during #1855228: Move Views core module tests to their respective modules
I also improved the assertion message, just trying to use it to track these 5 down was annoying without it.
Comment #13
damiankloip CreditAttribution: damiankloip commentedThis looks ready to go, except for 1 thing:
Would this expose the broken behaviour? This would only happen if we get an exception. Otherwise we wouldn't know about it. Maybe this should just set a variable or something instead?
Comment #14
tim.plunkettThat line actually could be removed, the important part is the two assertions directly afterward.
Comment #15
damiankloip CreditAttribution: damiankloip commentedYeah, that's kind of what it looks like. The assertions below will test the actual behavior really. Although, that is a nice one to have anyway, because I have found it never hurts to have extra assertions. One day they come in useful :) up to you.
Comment #16
tim.plunkettOkay, talked this over with @damiankloip, this is clearer.
Comment #17
xjmWow, I hope I didn't write that. Did we double-check that there's coverage elsewhere in the test for updating the object without using
entity_create()
incorrectly?Comment #18
sunClosely related: #1889620: config_install_default_config() overwrites existing configuration
Notes on this patch:
Speaking of "unique"... machine names are not really unique, by design. If we wanted to have real uniqueness, you know... #1609418: Configuration objects are not unique (feel free to call me parrot :P)
IIRC I authored this test code, in early stages of config entities, and I think it was supposed to verify that we're able replace a config entity with a new one.
However, replacing == updating — unless I'm missing something. Calling code should know when to create and when to update an entity.
So I think this removal is fine.
Can we rename this to
testUniqueID()
Let's also add a @todo to the method's phpDoc to add tests for renaming + saving an existing config entity to an ID that already exists. (blargh)
1) Can we use the regular exception assertion pattern here?
Copied from elsewhere:
2) The following assertions are a bit strange... that code wouldn't be reached normally, and if anything, then we'd want to reload the entity to confirm that it is still the same (instead of testing the in-memory object).
This test belongs into ConfigInstallTest, not ConfigImportTest.
Can we add a YAML comment here to clarify that this config object purposively duplicates config_test.dynamic.default?
package should be Testing
Comment #19
tim.plunkettThanks for the feedback!
Comment #20
tim.plunkettRerolled with feedback.
Comment #22
tim.plunkettThis is now blocked by #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API.
ModuleApiTest::testDependencyResolution() enables php.module, which ships with a default format. It is then uninstalled, but its config is not cleaned up. When it is reinstalled, module_enable attempts to copy over the format again, which breaks.
This fixes the other fail.
Comment #24
dcam CreditAttribution: dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
The summary may need to be updated with information from comments.
Comment #25
adriancotter CreditAttribution: adriancotter commentedI am going to write an issue summary
Comment #26
adriancotter CreditAttribution: adriancotter commentedLMK if anything doesn't add up in the issue summary (first one for me).
Comment #27
xjmRelated: #1998664: An existing block instance will be overwritten by a newly created block instance if the two share the same machine_name
Comment #27.0
xjmadding more information to the issue summary
Comment #27.1
xjmUpdated issue summary.
Comment #27.2
xjm.
Comment #28
tim.plunkettComment #30
tim.plunkettReroll
Comment #31
yched CreditAttribution: yched commentedThere are checks / exceptions that could be removed in Field / FieldInstance saveNew() if this is taken care of upstream.
Comment #32
EclipseGc CreditAttribution: EclipseGc commentedbasic logic looks pretty sane to me. Hope it passes.
Eclipse
Comment #34
chx CreditAttribution: chx commentedThis (in user module) tripped today a critical issue so it must be critical. Also, it's a gigantic WTF -- like, "who ate my entity?? WTF is going on here???"
Comment #35
marthinal CreditAttribution: marthinal commentedI made a few changes to the patch. For example at ViewStorageTest.php we are using the same id so, maybe is it possible to use load() method instead?
Comment #37
chx CreditAttribution: chx commentedPerhaps add a force to entity_create and if it's not being forced which should be the overwhelmingly default case (is there even any sense of it being TRUE??) and try to load and if the load succeeds then tell the caller somehow that we have an entity already. Challenging to get it right. Basically this issue should solve #2108419: user module overwrites role actions breaking config sync cleanly.
Comment #38
tstoecklerWell, you could pass TRUE for performance, if you don't want to hit the DB at all, like it is now.
chx convinced me in IRC, that this might actually make a lot of sense. At first I didn't like the idea of entity_create() doing anything other than creating an entity, specifically checking for existing entities. But it's true that the most common use-case is
And for this use-case, often times you don't really care whether or not there was an existing entity with the same ID already, you just want to make sure that there is one, afterwards, and that it has the values you specificed.
So ideally, IMO, we would have a wicked EntityFactory which would support stuff like
But until then, I think entity_create($force) makes a lot of sense.
Edit: forgot last sentence.
Comment #39
yched CreditAttribution: yched commentedWhy don't we do the check on save() ? This is where other checks already happen, and is also where the equivalent checks for Field / FieldInstance config entities (that would be deprecated by this patch) are currently made.
Comment #40
tstoecklerchx was advocating for that as well.
I could certainly live with it, but (as I said to chx in IRC) I feel like
should throw an exception, and not magically work. That might be just me, though.
Comment #41
yched CreditAttribution: yched commentedSure, I'm just saying we would throw an exception during the second save(), not the second entity_create(). It's *saving* we want to prevent.
Comment #42
chx CreditAttribution: chx commentedThrowing an exception is not helping import; not at all. It already happens in the following case: on user role creation we create an action (config entity). When we import, the action gets imported first, then the role which tries to recreate the action which exists with an obviously different UUID. Kaboom. This got a band aid by wrapping the entity_create()->save() in an !entity_load() but tstoeckler rightly points out that in contrib this will be a common experience and a giant WTF.
Comment #43
yched CreditAttribution: yched commentedThe right fix for #42 is not this patch, it's "do not trigger config writes during config import" (there's an existing issue for this, and the approach has been agreed during the CMI "hard issues" discussion in Prague - I'm not where I can easoly find the issue #, but it's linked from the gdoc notes for the meeting).
We should still prevent saving a "new" entity when it already exists, but very -1 on seeing it as the fix for #42, and I still think an exception in save() is the way to go. This is already where we check that we dont update an id with a different UUID. Let's not scatter related checks in various random places.
Comment #43.0
yched CreditAttribution: yched commentedUpdated issue summary.
Comment #44
blackra CreditAttribution: blackra commentedI hope that we don't need to worry about the race condition of the database changing under the entity between creation and save; the only likely scenarios I can think of would result in a uuid mismatch.
If we do need to take this into account then things become a little more complicated.
Comment #45
alexpottTagging
Comment #46
BerdirHere's a new approach, throwing the exception in save(), not in create(). This is no overhead as we don't do it for temporary entities and we already load that file anyway there.
Only updated ConfigEntityTest to test the new behavior, let's see what fails additionally...
Comment #48
BerdirOK, this is looking quite good. Fixing things..
- Remove the custom already-exists check for Field and FieldInstace, updated the tests for the new exception.
- CommentPreviewTest already is already using the standard profile (unfortunately), no need to copy anything from there
- ViewStorageTest uses data from an existing view including id and then changes the id, which acts as mix of a rename and a new entity. Using a new id there from the beginning solves that
- FieldTypeTest failed because there were two views named test_field_type and that test tried to enable both. Renamed the test view.
Comment #49
yched CreditAttribution: yched commented<3 @Berdir
Comment #50
damiankloip CreditAttribution: damiankloip commented(Sorry, cant see you on IRC now) I meant that one is a dupe. Something must have gone awry when we did the big port initially or something, or when we moved lots of stuff around.
We can just get rid of the file that's in views/tests/modules/views_test_config/test_views instead, as we don't need that. So then we don't need the rename either. Added plugin_id to the test_field_type view in node as it's missing from there (and was in the one we want to remove) :)
Other than that, I agree that this is looking good.
Comment #53
Berdir50: 1887654-50.patch queued for re-testing.
Comment #55
damiankloip CreditAttribution: damiankloip commentedRerolled
Comment #56
dawehnerCan we just reuse the variable?
Comment #57
damiankloip CreditAttribution: damiankloip commentedSure can.
Comment #58
BerdirI initially got very confused as to what $is_new actually refers to, the config file or the entity? Given that $config->isNew() is very easy to write, wondering if we shouldn't drop $is_new completely.
Comment #59
damiankloip CreditAttribution: damiankloip commentedI am down with that, Let's just remove.
Comment #61
damiankloip CreditAttribution: damiankloip commentedWhooops. Sorry :)
Comment #62
Damien Tournoud CreditAttribution: Damien Tournoud commentedSimplified summary.
Comment #63
Damien Tournoud CreditAttribution: Damien Tournoud commentedShould we also test the opposite
!$entity->isNew() && $config->isNew()
?Comment #64
yched CreditAttribution: yched commentedUpdated the summary to clarify that the check is made on save(), not on entity_create()
Comment #65
sunre: #63: As long as the config object does not exist, it should be writable. I'm not able to see a potential issue in case of the opposite condition (that wouldn't be caught elsewhere already) — if there is one, then I think we should discuss that in a separate issue.
Latest patch looks good to me.
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commentedyep, looks good.
a couple of docs nitpicks:
both of those comments could use some tidying up, but that can be done at commit time i guess, so leaving as RTBC.
Comment #67
catchGiving Alex a chance to look a this before it goes in. I reviewed older patches here but not the latest yet.
Comment #68
alexpottCommitted 261fb64 and pushed to 8.x. Thanks!
Fixed the comments in commit.