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.Problem/Motivation
If required config values are not set, errors will occur later on and are difficult to trace. For example, if $field_prefix is not set, then:
modules/field_ui/src/Form/FieldStorageAddForm.php
'#maxlength' => FieldStorageConfig::NAME_MAX_LENGTH - strlen($field_prefix),will fail when NULL to strlen.
Proposed resolution
From comments #11 and #13
>>> \Drupal::config('some_module.non_existent')
=> Drupal\Core\Config\ImmutableConfig {#6251}Should this throw an exception instead, and we need a separate explicit call to create a new config? This would warn users earlier that expected config is missing.
>>> \Drupal::config('field_ui.settings')->get('non_existent_key')
=> nullMaybe also this should throw an exception, if the config exists but the key does not, instead of falling back to NULL?
Remaining tasks
- Agree on an approach. Per #14, the proposed solution would require a deprecation first. But per #15 there are other things to consider.
- Write a patch with tests
- Review
- Commit
User interface changes
N/A
API changes
TBC
Data model changes
TBC
Release notes snippet
TBC
Original report by [username]
I had the case that $field_prefix was not set and therefore I got the issue with passing NULL to strlen.
modules/field_ui/src/Form/FieldStorageAddForm.php
'#maxlength' => FieldStorageConfig::NAME_MAX_LENGTH - strlen($field_prefix),| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 3321422-2.patch | 937 bytes | Prem Suthar |
Issue fork drupal-3321422
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
Prem Suthar CreditAttribution: Prem Suthar at Material for Drupal India Association commentedadd the patch For when $field_prefix value is empty then set Max length = 0.
Comment #4
Prem Suthar CreditAttribution: Prem Suthar at Material for Drupal India Association commentedComment #5
unkelhoebbi CreditAttribution: unkelhoebbi at Liip commented@Prem Suthar I've added an issue fork to this issue so we can work with merge requests instead of patches. If you want to use a diff file for your composer you can use https://git.drupalcode.org/project/drupal/-/merge_requests/2952.diff.
Comment #6
cilefen CreditAttribution: cilefen commentedWhy is
$field_prefixunset?Comment #7
longwaveI am guessing
field_ui.settingscould be missing entirely on some setups; there is only one config key and there is no UI for this. Defaulting to the empty string as in the MR seems safe enough; not sure this needs a test?Comment #8
larowlanWe should be able to rely on the presence of a config object, IE there's no default value for $config->get like there is for e.g the state API
My concern is if we babysit broken setups for config where does it end.
My thoughts is this should be marked a support request and we help the user fix their broken site, e.g create the missing config object
Comment #9
cilefen CreditAttribution: cilefen commentedI agree, we've been pushing back on these workarounds.
Comment #10
alexpottI agree with @larowlan we need to be able to rely on the existence of a module's configuration in a module's code. For me we should close as works as designed.
Comment #11
longwaveShould this throw an exception instead, and we need a separate explicit call to create a new config? This would warn users earlier that expected config is missing.
Comment #12
unkelhoebbi CreditAttribution: unkelhoebbi at Liip commentedI'd prefer the suggestion of longwave. I needed to debug the code to see what was actually missing. So it would have saved me some time if there was a proper exception.
Comment #13
longwaveMaybe also this should throw an exception, if the config exists but the key does not, instead of falling back to NULL?
Comment #14
alexpott@longwave I think that this is a great idea. immutable config being new makes no sense. I think we'd have to trigger a deprecation first and then an exception in the next major version.
We should re-focus the issue on doing that.
Comment #15
alexpottLooking at \Drupal\Core\Config\ConfigFactory::doGet() - the one fun thing will be overrides on a non-existing config object. Not 100% sure what we should do in that instance. It's a pretty strange thing to support.
Comment #16
quietone CreditAttribution: quietone at PreviousNext commented@Prem Suthar, making a duplicate patch from an MR does not advance an issue. There are contributor tasks on Drupal.org that provide the steps for making useful additions to an issue. Credit has been removed per How is credit granted for Drupal core issues.
Comment #19
pameeela CreditAttribution: pameeela commented