Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
You can have default config entities with a wrong filename, which doesn't match what is actually the config entity ID. That can result in strange bugs, just spend too much time debugging a problem I had in a distribution where a test started failing with #2930996: Config installer doesn't install possible installable config.
Proposed resolution
Fail early and clear with an exception in such a situation.
alexpott: @berdir I'm pondering if we can do if ($entity->getConfigDependencyName() !== $name) in the ConfigInstaller.
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 2972003-3.patch | 3.94 KB | alexpott |
| #3 | 2-3-interdiff.txt | 2.49 KB | alexpott |
| #2 | 2972003-2.patch | 1.45 KB | alexpott |
Comments
Comment #2
alexpottTurns out we have a simpler and less fragile way to test this than (ab)using the getConfigDependencyName() method.
/me wonders what is going to break.
I've triggered a E_USER_WARNING because I think we should produce an error in this situation but I think it should be recoverable (at least in Drupal 8.x).
Comment #3
alexpottHere's a test and a fix for the one place found so far in core that had this wrong - it was test code.
Comment #5
berdirNothing to add, looks good.
Comment #7
larowlanCommitted as add2370 and pushed to 8.6.x.
I think throwing a new warning is something we'd only want to do in a minor, so opting not to backport.
Comment #8
alexpott+1 to only adding a new warning to the next minor.
Comment #9
quietone commentedLast night commerce_migrate tests started to fail because of this fix. So, I am just wondering if this should have a change record?
Comment #10
quietone commentedSet to NW to get answer to my question
Comment #11
larowlanSure
Comment #12
berdirWell, you're welcome to add one because I'm not sure what to write there? IMHO, the error message is clear and you don't need any other information to fix it. Nor will someone see the change record and actively look for misnamed configuration.
There is no reason why you'd do this on purpose, the only reason is typos/renames and now we are just more strict in validating it early, before it might have caused some weird side effects later, e.g. in the referenced issue.
It's also "only" a warning, so just breaks tests and not actual sites yet.
Comment #13
idebr commentedSearch API also had a failing test: #2973047: Automated test failure after #2972003 Verify that the file name matches the ID when installing configuration
Test result page: https://www.drupal.org/pift-ci-job/965328
Being unaware of this issue, the warning message could probably be more verbose:
The configuration file name "field.storage.entity_test.links" does not match the its specified ID "entity_test_mulrev_changed.links"
Regarding the change record: its content could be fairly straightforward:
Title: ConfigInstaller now checks if a configuration file name matches it specified id.
I agree with Berdir in #12 though, this issue is fairly straight forward to fix with the current error so I had not expected to find a change record.
Comment #14
drunken monkeyRegarding the Search API issue, there is actually a weird phenomenon: when you look at #2973047-2: Automated test failure after #2972003 Verify that the file name matches the ID when installing configuration, you'll see the patch invalidly changes the IDs to the complete config keys, to completely match the filename (minus ".yml"). However, it still passes – any explanation for that?
I first thought the change here might just use
strpos()or something like that, which might explain it – but with the equality check I see here, I don't really know why it wouldn't detect this error, too?Regarding change record: A CR would have been nice insofar as I, when I first saw that fail for a patch in an issue, spent a few minutes to find out whether the fail was actually caused by the patch or in HEAD. If I'd seen a CR about this before that, it might have saved me a few minutes of time.
So, here is my take on a change record. If someone else thinks it's OK like this, please publish!
As said, though, I'm not saying a CR is 100% necessary – but if it might save someone a few minutes of time, why not?
Comment #15
idebr commentedThe change record was published by Berdir, see https://www.drupal.org/node/2974112/revisions
Comment #17
chi commentedThe followup #2980170: Incorrect error message on configuration ID check.