Problem/Motivation
#2392319: Config objects (but not config entities) should by default be immutable introduced ConfigFormBaseTrait, which uses a non-API approach to find a configuration factory within the object it is used, and forces developers to implement a method that exposes the names of config objects that must be mutable when retrieved through ::config().
This approach can cause problems, because objects using this trait can store their configuration factory in a property that isn't named configFactory, or have it returned by a method called getConfigFactory() instead of configFactory(). They can also provide a method called configFactory() that takes required parameters, or that returns something that is not a configuration factory. At this point we might as well use service location rather than trying to retrieve a dependency through methods like these.
The purpose of the trait is disputable as well, because its only achievement is to abstract the difference between ConfigFactoryInterface::get() and ConfigFactoryInterface::getEditable() away from developers who will not learn the difference between mutable and immutable configuration like this. On top of that the abstraction is downright unnecessary, because ConfigFactoryInterface::get() returns ImmutableConfig instances which throw exceptions when any mutation action is performed on them.
Proposed resolution
There are two options:
- Remove the trait and make all forms call
ConfigFactoryInterface::getEditable()directly. This improve developer experience and reduces complexity. - As an alternative, use service location to retrieve the config factory and make
getEditableConfigNames()non-abstract and return an empty array by default, so developers who do not wish to use this trait can safely ignore it without having to implement an unnecessary method.
Remaining tasks
None.
User interface changes
None.
API changes
None, or a small one to revert a DX regression.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | drupal_2494719_1.patch | 4.02 KB | xano |
Comments
Comment #1
xanoThis patch takes approach 2 to solve this issue. I'd personally rather see we take approach 1, but that would mean a BC break (and a bigger DX improvement than approach 2).
Comment #3
xanoComment #5
pcambraI think this is very similar to #2561329: Make Config forms easier by making getEditableConfigNames optional
Comment #6
tim.plunkettAbstract classes and methods are not magic. Traits are not magic.
And I don't see how they are "error prone" if PHP tells you *immediately* what you are missing and how to fix it. Additionally, any competent IDE will have already warned you that your ConfigFormBase subclass is incomplete.
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!
Comment #23
smustgrave commentedReading the comments and description kinda read like a feature request vs a needed task. And since there's been no follow up going to close out. Can always be re-opened.
Thanks all