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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mrsinguyen’s picture

Status: Active » Needs review
FileSize
1.82 KB

Attached patch, remove unused local variable

mrsinguyen’s picture

Assigned: mrsinguyen » Unassigned
adsw12’s picture

Assigned: Unassigned » adsw12

I 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))

boran’s picture

Assigned: adsw12 » Unassigned

Reviewing 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

adsw12’s picture

FileSize
68.89 KB

The 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.

boran’s picture

Status: Needs review » Reviewed & tested by the community

All ok, configuration mangement is really cool: see the screenshot in #5 :-)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Component/Diff/DiffEngine.php
@@ -306,7 +306,7 @@ function _diag($xoff, $xlim, $yoff, $ylim, $nchunks) {
-        while (list ($junk, $y) = each($matches)) {
+        while (list ( , $y) = each($matches)) {

@@ -314,7 +314,7 @@ function _diag($xoff, $xlim, $yoff, $ylim, $nchunks) {
-        while (list ($junk, $y) = each($matches)) {
+        while (list ( , $y) = each($matches)) {

To 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.

mrsinguyen’s picture

Title: Remove Unused local variable $junk from /core/lib/Drupal/Component/Diff/DiffEngine.php » Remove Unused local variable from /core/lib/Drupal/Component/Diff/DiffEngine.php
Status: Needs work » Needs review
FileSize
1.09 KB
areke’s picture

I 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.

xjm’s picture

Title: Remove Unused local variable from /core/lib/Drupal/Component/Diff/DiffEngine.php » Remove Unused local variable from /core/lib/Drupal/Component
Priority: Normal » Minor
Issue summary: View changes

@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.

parthipanramesh’s picture

Status: Needs review » Needs work

patch does not apply.

fabricebernhard’s picture

Title: Remove Unused local variable from /core/lib/Drupal/Component » Remove Unused local variable from /core/lib/Drupal/Component/Diff/DiffEngine.php
Related issues: +#2080055: Remove Unused local variable $v_result from /core/lib/Drupal/Component/Archiver/ArchiveTar.php

I 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.

fabricebernhard’s picture

Ok I think there is a clean solution to removing this $junk variable : replacing the convoluted "while( list(...) = each(...) )" by a foreach.

Patch is attached.

         reset($matches);
-        while (list ($junk, $y) = each($matches)) {
+        foreach($matches as $y) {

I am writing a unit test for this Diff class. Should I create a specific issue for the unit test?

fabricebernhard’s picture

I actually realised the reset becomes useless with the introduction of foreach. I changed the patch in consequence.

         $matches = $ymatches[$line];
-        reset($matches);
-        while (list ($junk, $y) = each($matches)) {
+        foreach($matches as $y) {
           if (empty($this->in_seq[$y])) {
             $k = $this->_lcs_pos($y);
             USE_ASSERTS && assert($k > 0);
@@ -318,7 +317,7 @@ function _diag($xoff, $xlim, $yoff, $ylim, $nchunks) {
             break;
           }
         }
-        while (list ($junk, $y) = each($matches)) {
+        foreach($matches as $y) {
           if ($y > $this->seq[$k-1]) {
fabricebernhard’s picture

Issue summary: View changes
fabricebernhard’s picture

Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#2156677: Add PHPUnit test coverage for /core/lib/Drupal/Component/Diff/DiffEngine.php

The test was added in the related issue: https://drupal.org/node/2156677

Status: Needs review » Needs work

The last submitted patch, 14: drupal-core-remove-unused-local-variable-2079863-14.patch, failed testing.

fabricebernhard’s picture

Status: Needs work » Needs review
lhangea’s picture

Is 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.

YesCT’s picture

YesCT’s picture

Title: Remove Unused local variable from /core/lib/Drupal/Component/Diff/DiffEngine.php » Remove Unused local variable from /core/lib/Drupal/Component Diff/DiffEngine.php Utility/Crypt.php Transliteration/PHPTransliteration.php Utility/UserAgent.php Gettext/PoHeader.php
Issue summary: View changes
Status: Needs review » Needs work
Related issues: +#2080051: Remove Unused local variable $default from /core/lib/Drupal/Component/Gettext/PoHeader.php, +#2080391: Remove Unused local variables from multiple core files

@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

YesCT’s picture

Issue summary: View changes

just html fix in summary

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.56 KB
3.52 KB

this 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.

Status: Needs review » Needs work

The last submitted patch, 23: drupal-core-remove-unused-local-variable-2079863-23.patch, failed testing.

Chernous_dn’s picture

Status: Needs work » Needs review
Issue tags: +dcdn-sprint
FileSize
3.5 KB

I fixed unused local variable from /core/lib/Drupal/Component Diff/DiffEngine.php Utility/Crypt.php Transliteration/PHPTransliteration.php Utility/UserAgent.php Gettext/PoHeader.php.

filijonka’s picture

Status: Needs review » Reviewed & tested by the community

based on the summary this looks good for me

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2079863-25.patch, failed testing.

filijonka’s picture

Status: Needs work » Needs review

25: 2079863-25.patch queued for re-testing.

YesCT’s picture

YesCT’s picture

@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.

filijonka’s picture

Status: Needs review » Closed (won't fix)
alexpott’s picture

Status: Closed (won't fix) » Needs work

Let's just remove the bits that touch the Diff component and move on with this.

YesCT’s picture

Title: Remove Unused local variable from /core/lib/Drupal/Component Diff/DiffEngine.php Utility/Crypt.php Transliteration/PHPTransliteration.php Utility/UserAgent.php Gettext/PoHeader.php » Remove Unused local variable from /core/lib/Drupal/Component Utility/Crypt.php Transliteration/PHPTransliteration.php Utility/UserAgent.php Gettext/PoHeader.php
filijonka’s picture

Status: Needs work » Needs review
FileSize
2.48 KB

thanks @alexpott for correting me :) obviously we can't ignore the other files. here's a new patch.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

looks good.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 2079863-34.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

34: 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.

[09:07:09] Command [cd sites/default/files/checkout && drush si -y --db-url=mysql://drupaltestbot-my:Mp4kFIQ8LWDc@localhost/drupaltestbotmysql  --account-name=admin --account-pass=drupal --account-mail=admin@example.com 2>&1] failed
  Duration 3 seconds
  Directory [/var/lib/drupaltestbot]
  Status [255]
 Output: [You are about to create a /var/lib/drupaltestbot/sites/default/files/checkout/sites/default/settings.php file and empty any Config directories and DROP all tables in your 'drupaltestbotmysql' database. Do you want to continue? (y/n): y
Starting Drupal installation. This takes a few seconds ...                  [ok]
PHP Fatal error:  Call to a member function getProvider() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/entity/lib/Drupal/entity/EntityDisplayModeBase.php on line 88

Fatal error: Call to a member function getProvider() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/entity/lib/Drupal/entity/EntityDisplayModeBase.php on line 88
Drush command terminated abnormally due to an unrecoverable error.       [error]
Error: Call to a member function getProvider() on a non-object in
/var/lib/drupaltestbot/sites/default/files/checkout/core/modules/entity/lib/Drupal/entity/EntityDisplayModeBase.php,
line 88].
YesCT’s picture

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

that should have stayed at (#35) rtbc (was an unrelated failure)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Hm. 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!

  • Commit c8d2e23 on 8.x by webchick:
    Issue #2079863 by filijonka, mrsinguyen, fabricebernhard, Chernous_dn,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.