Problem/Motivation
Corollary to #2239065: Overridden config bleeding through to configuration forms, but not as easy to fix.
The loading of the config entity happens in \Drupal\Core\ParamConverter\EntityConverter.
This currently applies overrides, so going to an entity form and hitting save will change your stored config.
Proposed resolution
LocaleAdminPathConfigEntityConverter currently handles this but only for language overrides. We want this for ALL overrides, move it to \Drupal\Core
Remaining tasks
N/A
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff.txt | 1.7 KB | tim.plunkett |
#14 | config-2251915-31.patch | 9.57 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettThe test here should be good, my attempted fix is a bit questionable...
I had to update a couple things in ConfigTest to get everything to work.
Comment #3
Gábor HojtsyThere is in fact LocaleAdminPathConfigEntityConverter, which is supposed to solve this but:
- language overrides have since been moved to the language module instead of locale, so this is misplaced
- other kinds of overrides still apply if you don't have language/locale modules enabled we'd still need a generic solution
So I agree a generic solution is needed, but that should remove / adapt the converter already in core instead of adding yet one more way :)
Comment #4
tim.plunkettI was taking a very similar approach, but it didn't occur to me that we could just adjust the configFactory directly, duh!
LocaleAdminPathConfigEntityConverter is exactly what we need, we just need it to run always, and at the right time (after other route subscribers like the AdminRouteSubscriber).
The interdiff is against the test only patch, it passes with no other tweaks.
This seems a little heavy handed, but until #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects is decided, this matches our expectations perfectly.
Comment #5
dawehnerIt would be great to document why we need a higher priority.
Comment #6
tim.plunkettI copied that priority from the existing definition, and there are detailed docs on the class itself.
But I added a one-liner that should help.
Comment #7
dawehnerGreat, thank you!
Comment #8
tim.plunkett#2136559: Config entity admin lists show configuration with overrides (eg. localized) was the original issue I was looking for, marking as a dupe.
And for future reference, #2099541: ConfigEntities should not load the Entity translated on Edit Forms is where this code was added originally.
Comment #9
Gábor HojtsyWait a minute, this does indeed solve this in a more general way now for admin forms, but not for admin listings. If @tim.plunkett was to mark #2136559: Config entity admin lists show configuration with overrides (eg. localized) as a duplicate, then this needs to solve the listings too, no?
Comment #10
tim.plunkettI misunderstood that other issue. I reopened it, it is a very different problem-space.
Thanks @Gábor Hojtsy!
Comment #11
Gábor HojtsyRight, that is about entity *listings* on the admin interface. Titling this back to the simpler case it is then :) I think it means then that #2136559: Config entity admin lists show configuration with overrides (eg. localized) is also critical.
Comment #12
catchMaking sure Alex gets a chance to look at this before it goes in.
Comment #13
alexpottLooks good a couple of minor nits.
bleed through? How about "Test that config overrides do not (appear on|effect) entity forms."
site name?
Comment #14
tim.plunkettFixed, thanks.
I also cleaned up other parts of the test that were shamelessly copy/pasted from the other issue.
Comment #15
alexpottCommitted 4ad6fc9 and pushed to 8.x. Thanks!
Fixed up function docblock
Comment #17
Gábor HojtsyYay, its great to see again a more generalised solution for what we made up for languages only :) Tagging with D8MI too.
Comment #18
Gábor Hojtsy