Config forms have easy access to configuration objects via the config() method, which is good. However, you need to flag them as editable in a separate method, which is not even remotely obvious. If you don't know you need it, you get an error that your class is not defining a method that is off in a trait somewhere; at least it gives you an error, but it makes little sense.
Especially when you consider that all that method does it swap a single method call, the natural question is "why do I need to do that? Why can't I just swap the method call myself, and then my code is more self-evident?" Specifically, right now the behavior of config() is unpredictable; it could give you the editable version of a config object or not, depending on another method that depending on your form could be over 100 lines away. That's bad DX.
Add an editableConfig() method to ConfigFormBaseTrait that gets, as the name implies, an editable config. Move getEditableConfigNames() to ConfigFormBase, and give it a default empty implementation.
Needs a test or two, but waiting on approval.
None.
One additional method is provided in ConfigFormBaseTrait to get at an editable config object without needing to register it separately. No current code is affected as config() continues to work exactly as it does now.
None.
| Issue category | Task because it's just a DX improvement |
|---|---|
| Issue priority | Normal because nothing is broken currently; this is just a DX improvement. |
| Comment | File | Size | Author |
|---|---|---|---|
| get-editable-config.patch | 2.52 KB | Crell |
Comments
Comment #3
legolasboI've had a look at why this fails. The problem is in the last line of this snippet
ConfigFormBaseTraitTest::testConfigline 40. Unfortunately my xdebug is not working correctly from the cli so I am unable to determine WHY it is failing.Comment #4
Crell commentedIt appears that PHPUnit is not mocking the getEditableConfigNames() properly after this patch. Why, I don't know, but it's returning [], even if the expects()... call is there. That almost feels like a PHPUnit bug but I'm not certain. Perhaps some upstream investigation is necessary, or perhaps switch the test to use Prophecy?
legolasbo, do you want to take a stab at the latter, assuming it's a PHPUnit bug?
Comment #5
legolasbo@Crell,
I would love to help out, but I am currently buried in client work, which I don't expect to lift for a few weeks.
Comment #11
dave reidThis information is actually very useful to have for other modules to make use of, the only downside being it's not public. I would encourage this to be closed because I'd much rather have this stay and issues like #2095289: Make getEditableConfigNames() public and use it in config_translation make a case for it's existence.
Comment #21
smustgrave commentedThank you for creating this issue to improve Drupal.
We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!