Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.In the Config class, Config::$name is supposed to be restricted in length and character set. But this is never checked until you try to save() the object, at which point it's too late to do anything about it.
This means it's possible to create and use a Config object with an invalid name. So for example getName() would return the invalid name, and setName() could be used to set a new invalid name. Any code using this object would be dealing with an invalid name. And the only place the name is checked is when you save() the object, which may be way downstream from where the Config was created.
A better way to do this would be to ensure that Config::$name always contains a valid value - we should never be able to construct a Config object with an invalid name, and we should never be able to setName() to an invalid name. In other words, validateName() should be called in the constructor and in setName(), not in save().
This can be fixed with a few lines of code. I also added some documentation comments to explain what constitutes a valid name, because currently that's only described in the code.
| Comment | File | Size | Author |
|---|---|---|---|
| #29 | config-validate-name-2150803-29.patch | 7.6 KB | willzyx |
| #29 | interdiff-26-29.txt | 626 bytes | willzyx |
| #22 | interdiff-19-22.txt | 2.98 KB | willzyx |
| #19 | config-validate-name-2150803-19.patch | 2.32 KB | deepakaryan1988 |
| #16 | config-validate-name-15.patch | 2.32 KB | ashutoshsngh |
Comments
Comment #1
TR CreditAttribution: TR commentedconfig-validate-name.patch queued for re-testing.
Comment #2
jhedstromPatch no longer applies.
Comment #3
tadityar CreditAttribution: tadityar commentedre-rolled the patch
Comment #5
tadityar CreditAttribution: tadityar commentedre-roll again
Comment #6
penyaskitoPatch #5 is not included the validation at setters (probably missing other things too).
Comment #7
tadityar CreditAttribution: tadityar commented@penyaskito the function setName($name) and validateName($name) doesn't exist in /core/lib/Drupal/Core/Config/Config.php anymore. What should I do? include them again?
Comment #8
swentel CreditAttribution: swentel commentedThe functions are in ConfigBase now. Not sure about moving it, although it probably can't hurt.
Comment #9
tadityar CreditAttribution: tadityar commentedre-roll with Updated the ConfigBase file
Comment #10
penyaskitoThese have tabs instead of spaces. Check https://www.drupal.org/coding-standards#indenting
Otherwise looks good to me.
Comment #11
tadityar CreditAttribution: tadityar commentedMy bad didn't notice that.. corrected the indentation :)
Comment #14
ashutoshsngh CreditAttribution: ashutoshsngh at Srijan | A Material+ Company commentedPath won't work,re-rolled the patch.
Comment #16
ashutoshsngh CreditAttribution: ashutoshsngh at Srijan | A Material+ Company commentedMissed one change in config.php file.And also fixed indent issues.
Comment #18
ashutoshsngh CreditAttribution: ashutoshsngh at Srijan | A Material+ Company commentedIt's working fine on my local i don't know why it's failing here.
Comment #19
deepakaryan1988Re-rolling the patch in comment #9
Comment #20
deepakaryan1988Comment #22
willzyx CreditAttribution: willzyx commentedI think we need to change also \Drupal\Tests\Core\Config\ImmutableConfigTest
Comment #23
TR CreditAttribution: TR commentedNow that Config has been refactored, the validation should be done in the base class (ConfigBase) constructor, that way derived classes will inherit this functionality. It makes no sense to defer the validation to the child class constructor when the setName/getName/validateName functions are all in ConfigBase.
The patch also needs a new test case where Config::setName() is used to try to set an invalid name, to show that this patch does the right thing and to make sure this continues to work in the future.
@willzyk: How are your changes to ImmutableConfigTest.php related to this issue?
Comment #24
willzyx CreditAttribution: willzyx commentedThese changes are needed because in
ImmutableConfigTest::setUp()the code below trigger the Config constructor passing an invalid name: 'test'$this->config = new ImmutableConfig('test', $storage, $event_dispatcher, $typed_config);so the plan is to add the constructor to abstract class
ConfigBase? anyhow in this way derived classes with their own constructor (almost all) will not inherit this functionality automatically and should explicitly invoke the parent constructorComment #25
TR CreditAttribution: TR commentedOK, thanks for clarifying that.
Well, yes. The alternative is just to assume that everyone who subclasses ConfigBase will know that they *have* to provide a subclass constructor, whether they need it or not, *and* have to implement the validation in that subclass constructor. We don't have any way to reliably convey this message, let alone enforce it.
You're correct that in PHP if a subclass provides its own constructor then it's responsible for invoking the parent constructor if it wants the parent to be initialized properly. This should be well-known (and second-nature) to anyone writing OO code and implementing a __construct() method in a subclass.
Comment #26
willzyx CreditAttribution: willzyx commentedI have taken into account #25 but I'm not convinced at all.
Moreover the changes that we are thinking to make could have a huge impact and create performance regressions so probably better have feedback from maintainers.
Comment #28
TR CreditAttribution: TR commentedPlease explain why you think this is a possibility - have you done any benchmarking? How many times is setName() normally called in a page load?
The fact that this patch has already exposed a core error - use of an invalid config name in ImmutableConfigTest - demonstrates my original claim that currently "... it's possible to create and use a Config object with an invalid name".
Comment #29
willzyx CreditAttribution: willzyx commentedfixed tests
the problem is not ::setName() but the constructor that is invoked for every config object and performs the validation for all configs, even those that do not need it (eg saved ones). My concern for performance may be overdone (constructor + ::setName() are called ~50 times for every page load as authenticated user on homepage in my installation) but make some benchmark may be useful
Comment #30
willzyx CreditAttribution: willzyx commentedComment #31
alexpottValidating the name on construct is unnecessarily expensive. Validating in the constructor means that all config names are validated even when we load existing configuration that has already been validated.
Comment #32
TR CreditAttribution: TR commentedI would argue that if there is a subset of Config objects that have already passed through validation, then that subset should be represented by a subclass of Config. The validation upon construction could then be bypassed in the constructor of the subclass, as an optimization, if warranted by performance benchmarks. To allow invalid objects to be constructed and used and not checked until you try to save them is really not an option IMO. But can you really say for sure that a config read from the file system has already been validated, and that no one has renamed it?
As an aside, it's not really clear to me why we make these specific validation requirements in the first place - why a maximum length of 250 characters? Isn't that totally arbitrary, or perhaps tied to just one specific storage type? There doesn't seem to be any documentation about why those checks are in there.
Comment #45
larowlan