Hi everyone.
First of all this is my first issue report identified at Amsterdam DrupalCon 2014.
In core/includes/install.inc I have identified several unidentified variables in the function drupal_rewrite_settings.
While these variables might(probably) get set through the process of the foreach loop the variables should still have some default values.
This mitigates false positives from static code analysis made by IDE's etc.
The first issue:
case 'right_bracket':
if ($value == ']') {
if (isset($current[$index])) {
The index variable is not set outside the scope and not defined in the case 'right_bracket'.
Second issue:
case 'candidate_right':
if (_drupal_rewrite_settings_is_simple($type, $value)) {
$value = _drupal_rewrite_settings_dump_one($current);
// Unsetting $current would not affect $settings at all.
unset($parent[$index]);
Again three variables is used without being defined first or outside the scope of the switch statement.
The three variables are $current, $index and $parent.
The relevant code was moved to \Drupal\Core\Site\SettingsEditor
TBA
Decide how to correctly define the possibly undefined variables
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | interdiff_23_24.txt | 21.47 KB | Aadhar_Gupta |
| #24 | 2349645-24.patch | 21.46 KB | Aadhar_Gupta |
Comments
Comment #1
RonnieJ commentedThis is my first patch.. Hopefully its enough(and good enough).
I simply set the variables to null before the foreach loop. If the loop behaves as expected it should overwrite the variables before used. Otherwise the variables is null.
Comment #2
RonnieJ commentedComment #3
jhedstromPatch still applies, and this makes sense to set defaults for these variables. I've added a beta phase evaluation to the issue summary.
Comment #4
jhedstromComment #5
jhedstromActually, the patch needs to be updated to meet coding standards. The 'nulls' should be all upper case, or perhaps be set to 0 since they are used as integers.
Comment #6
sushantpasteUpdated patch.
Comment #7
sushantpasteComment #13
quietone commentedI agree with the comment in #3 that having defaults make sense. The patch still applies and tests pass on 8.5 and 8.6
Comment #14
alexpottI'm not sure. Setting the values might hide useful errors. In the current design you should never use these variables before they are assigned. If it happens then you get a useful warning telling you that's what has happened. With this change that would go away. Ideally drupal_rewrite_settings() would be redesigned to be easier to bugfix and have less complexity.
Comment #22
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Still seems to be relevant but the bulk of the code was moved to SettingsEditor.php
Tagged for novice for beginner developers.
Comment #23
_utsavsharma commentedPatch for 10.1.x.
Comment #24
Aadhar_Gupta commentedApplying fix for 10.1.x and fixing custom commands
Comment #25
smustgrave commentedNot sure what happened but that patch is full of out of scope changes. Can compare the files sizes between the two and see
Comment #26
quietone commentedAccording to #14, this is not a novice issue. I am removing the tag. Setting to active since this needs a re-think.
Comment #27
alexpottWe also need a new issue summary and title given this now has nothing to do with install.inc
I think rather than focussing on the unused variables we should focus on the design of \Drupal\Core\Site\SettingsEditor and see if we can improve that. And as part of that solve the "variable might not be defined" warnings in the class.
Also this really is a task and not a bug.
Comment #29
quietone commented#3244570