Part of meta-issue #2002650: [meta, no patch] improve maintainability by removing unused local variables
Updated: Comment #15
Component
/core/lib/Drupal/Component
Diff/DiffEngine.php
Not being done here because #1848266: Convert Diff into a proper, PSR-0-compatible PHP component is re-writing DiffEngine anyway
File /core/lib/Drupal/Component/Diff/DiffEngine.php
Initially the list was:
Line 309: Unused local variable $junk
Line 317: Unused local variable $junk
Line 1258: Unused local variable $i
Line 1274: Unused local variable $i
The code has changed since then, the actual list is:
Line 313: Unused local variable $junk
Line 321: Unused local variable $junk
Proposed solution: refactoring while(list(...) = each(...)) into a clean and simple foreach, removing de facto the unneeded $junk variable (and making the code much simpler)
Utility/Crypt.php
line 33 $has_openssl
Transliteration/PHPTransliteration.php
line 87 $to_add
pattern is
$to_add = '';
if ($code == -1) {
$to_add = $unknown_character;
}
else {
$to_add = $this->replace($code, $langcode, $unknown_character);
}
So, if we agree we do not need an initial value that is never used, we can remove the initialization which phpstorm reports as an unused var.
Utility/UserAgent.php
line 94 $generic_tag
pattern is
$generic_tag = '';
if (strlen($langcode) > 7 && (substr($langcode, 0, 7) == 'zh-hant' || substr($langcode, 0, 7) == 'zh-hans')) {
$generic_tag = substr($langcode, 0, 7);
}
else {
$generic_tag = strtok($langcode, '-');
}
if (!empty($generic_tag) && !isset($ua_langcodes[$generic_tag])) {
Gettext/PoHeader.php
line 231 $default
#2080051: Remove Unused local variable $default from /core/lib/Drupal/Component/Gettext/PoHeader.php was closed and part done in #2080391: Remove Unused local variables from multiple core files. $default is still left.
pattern is
$default = 0;
if ($element_stack !== FALSE) {
for ($i = 0; $i <= 199; $i++) {
$plurals[$i] = $this->evaluatePlural($element_stack, $i);
}
$default = $plurals[$i - 1];
$plurals = array_filter($plurals, function ($value) use ($default) {
return ($value != $default);
});
$plurals['default'] = $default;
Archiver/ArchiverTar.php
not to be done. see #2080055: Remove Unused local variable $v_result from /core/lib/Drupal/Component/Archiver/ArchiveTar.php
Comment | File | Size | Author |
---|---|---|---|
#34 | 2079863-34.patch | 2.48 KB | filijonka |
#1 | drupal-core-remove-unused-local-variable-2079863.patch | 1.82 KB | mrsinguyen |
#5 | Diff-DiffEngine.png | 68.89 KB | adsw12 |
#8 | drupal-core-remove-unused-local-variable-2079863-8.patch | 1.09 KB | mrsinguyen |
#13 | drupal-core-remove-unused-local-variable-2079863-13.patch | 962 bytes | fabricebernhard |
Comments
Comment #1
mrsinguyen CreditAttribution: mrsinguyen commentedAttached patch, remove unused local variable
Comment #2
mrsinguyen CreditAttribution: mrsinguyen commentedComment #3
adsw12 CreditAttribution: adsw12 commentedI followed these steps below:
1) Patch is verified
2) Patch is cleaned and applied
Next steps
3) Verifying variables are outside the scope
4) Verifying PHP function with empty parametre (list ( , $y) = each($matches))
Comment #4
boran CreditAttribution: boran commentedReviewing the syntax change:
- Reading example #1 in http://php.net/manual/en/function.list.php, replacing list ($junk, $y) with list ( , $y) is allowed
>> ok
Comment #5
adsw12 CreditAttribution: adsw12 commentedThe configuration manager works fine with the new patch. I could "Export" and "Import" config.tar.gz. Also, I could see my changes on "Synchronize configuration" page. See attached file.
Comment #6
boran CreditAttribution: boran commentedAll ok, configuration mangement is really cool: see the screenshot in #5 :-)
Comment #7
alexpottTo be honest here I think the result code is more confusing. $junk is tell the developer we're ignoring something. The point is we are not using it.
Comment #8
mrsinguyen CreditAttribution: mrsinguyen commentedComment #9
areke CreditAttribution: areke commentedI do agree with alexpott in that the code is much more confusing without the variable $junk. Because of this, the second patch is what should be used; unfortunately, that patch no longer applies. It looks like the function render() was removed from DiffEngine.php so I don't think this file needs to be changed at all.
Comment #10
xjm@areke, are you saying it's no longer an issue?
Can we check for any unused local variables in Drupal\Component? If there are none remaining then this issue can be closed wontfix.
Comment #11
parthipanramesh CreditAttribution: parthipanramesh commentedpatch does not apply.
Comment #12
fabricebernhard CreditAttribution: fabricebernhard commentedI have inspected the code with PHPStorm.
There are still unused variable in :
- ArchiveTar.php. But this is addressed in issue 2080055
- PHPTransliteration.php. Does not seem to be mentioned anywhere. Should a specific issue be created ?
- DiffEngine.php. The current issue, which is why I renamed the issue to something specific
As for the $junk variable, there must be a way to refactor the code to remove it in a clear way. Looking into it.
Comment #13
fabricebernhard CreditAttribution: fabricebernhard commentedOk I think there is a clean solution to removing this $junk variable : replacing the convoluted "while( list(...) = each(...) )" by a foreach.
Patch is attached.
I am writing a unit test for this Diff class. Should I create a specific issue for the unit test?
Comment #14
fabricebernhard CreditAttribution: fabricebernhard commentedI actually realised the reset becomes useless with the introduction of foreach. I changed the patch in consequence.
Comment #15
fabricebernhard CreditAttribution: fabricebernhard commentedComment #16
fabricebernhard CreditAttribution: fabricebernhard commentedThe test was added in the related issue: https://drupal.org/node/2156677
Comment #18
fabricebernhard CreditAttribution: fabricebernhard commented14: drupal-core-remove-unused-local-variable-2079863-14.patch queued for re-testing.
Comment #19
lhangea CreditAttribution: lhangea commentedIs there something else to be done to this issue ? It seems like all the unused local variables have been removed. If there's nothing more to be done pls someone change it to 'reviewed and tested' as the last patch applies cleanly or otherwise update the issue with to dos.
Comment #20
YesCT CreditAttribution: YesCT commented14: drupal-core-remove-unused-local-variable-2079863-14.patch queued for re-testing.
Comment #21
YesCT CreditAttribution: YesCT commented@lhangea anyone who does a review and finds no problems can mark something rtbc. (so you can also.)
you did a nice summary of what you looked at, that helps people know what you did as part of your review. and if they disagree with the rtbc, they can change the status back to needs work or needs review and give reasons why they did. using this process, we get better at doing reviews because we see what other people checked also.
I read back through the comments and in #10 @xjm asked if there were any other unused vars under /core/lib/Drupal/Component
I did an inspect code and there are. (needs work to remove those)
updating the issue summary
Comment #22
YesCT CreditAttribution: YesCT commentedjust html fix in summary
Comment #23
YesCT CreditAttribution: YesCT commentedthis takes care of the other unused in the rest of Component
the openssl one is pretty clear.
about the default, add_to and generic_tag:
If this is green, then, I think what we need is a review that has an opinion on if we should remove the initialization that is overwritten later (so no used).
If we can point to another unused var issue that already was committed doing that, that would give confidence in this.
If we can point to another issue that was asked *not* to remove a var like that, that would tell us to leave these here.
Comment #25
Chernous_dn CreditAttribution: Chernous_dn commentedI fixed unused local variable from /core/lib/Drupal/Component Diff/DiffEngine.php Utility/Crypt.php Transliteration/PHPTransliteration.php Utility/UserAgent.php Gettext/PoHeader.php.
Comment #26
filijonka CreditAttribution: filijonka commentedbased on the summary this looks good for me
Comment #28
filijonka CreditAttribution: filijonka commented25: 2079863-25.patch queued for re-testing.
Comment #29
YesCT CreditAttribution: YesCT commented23: drupal-core-remove-unused-local-variable-2079863-23.patch queued for re-testing.
Comment #30
YesCT CreditAttribution: YesCT commented@Chernous_dn What is the difference between your patch and the one in #23?
I did an interdiff
[ For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff | Microbranching workflow: http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-... ]
to see:
#25 did fix:
"Control statements should have one space between the control keyword and opening parenthesis, to distinguish them from function calls."
from
https://drupal.org/coding-standards#controlstruct
... but introduced another whitespace thing before the {, so adding that back.
(see interdiff-2079863-25-30.txt)
I'm not sure about the adding is the reset($matches);
Maybe this was related to the fail in #23... but looking at those fails they were (mostly)
copy(/var/lib/drupaltestbot/sites/default/files/checkout/sites/simpletest/796436/settings.php): failed to open stream: No such file or directorycopy('/var/lib/drupaltestbot/sites/default/files/checkout/sites/default/default.settings.php', '/var/lib/drupaltestbot/sites/default/files/checkout/sites/simpletest/796436/settings.php')
So sending that for a retest to see if it was unrelated.
Comment #31
filijonka CreditAttribution: filijonka commentedActually closing this issue and also the related #2156677: Add PHPUnit test coverage for /core/lib/Drupal/Component/Diff/DiffEngine.php due to this issue #1848266: Convert Diff into a proper, PSR-0-compatible PHP component.
Comment #32
alexpottLet's just remove the bits that touch the Diff component and move on with this.
Comment #33
YesCT CreditAttribution: YesCT commentedComment #34
filijonka CreditAttribution: filijonka commentedthanks @alexpott for correting me :) obviously we can't ignore the other files. here's a new patch.
Comment #35
YesCT CreditAttribution: YesCT commentedlooks good.
Comment #37
YesCT CreditAttribution: YesCT commented34: 2079863-34.patch queued for re-testing.
[edit]
This was green before.
Fail was installation failure.
Putting error here so we can track if it's a random fail similar to others.
Comment #38
YesCT CreditAttribution: YesCT commentedComment #39
YesCT CreditAttribution: YesCT commentedthat should have stayed at (#35) rtbc (was an unrelated failure)
Comment #40
webchickHm. That $has_openssl looks very deliberate, but I couldn't find any other instances though the entire code base. Weird. Well, I guess we'll find out soon if it broke anything. :)
Committed and pushed to 8.x. Thanks!