I'm scanning the codebase with PHPStorm's code analysis tools tonight and I found three instances of the "Silly Arguments" rule where we are assigning a variable itself. I don't have any idea why were doing this so I thought I'd roll up a patch to test it.

In the case of the two issues I found in the date module I changed to logic so that the "silly argument" wasn't needed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cosmicdreams’s picture

FileSize
2.23 KB
cosmicdreams’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, SillyArguments.patch, failed testing.

jhodgdon’s picture

Component: documentation » other
Status: Needs work » Reviewed & tested by the community

This is not a docs issue. Test failure looks like a random glitch though. I'll hit retest. The patch appears fine to me, so if the tests pass, it should be good to go.

jhodgdon’s picture

#1: SillyArguments.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, SillyArguments.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review

#1: SillyArguments.patch queued for re-testing.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Failed because of #2057401: Make the node entity database schema sensible - should be fine now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, SillyArguments.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review

#1: SillyArguments.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, SillyArguments.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review

#1: SillyArguments.patch queued for re-testing.

cosmicdreams’s picture

Looks like we're good then? RTBC?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Berdir’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.php
@@ -428,7 +428,6 @@ public function submitForm(array &$form, array &$form_state) {
     for ($weight; $weight < count($tree); $weight++) {
       $term = $tree[$weight];
       if ($term->parent->value == 0 && $term->weight->value != $weight) {
-        $term->parent->value = $term->parent->value;
         $term->weight->value = $weight;
         $changed_terms[$term->id()] = $term;
       }

This might actually indicate a bug, so we shouldn't just remove it.

I know that re-ordering terms currently doesn't work and this might be the reason for it.

Berdir’s picture

Status: Reviewed & tested by the community » Needs review

Setting to needs review for now. I think there's a issue about the broken order stuff somewhere, maybe just cross-reference the issues and remove that part?

jhodgdon’s picture

Do you mean: #941266: Order of terms with same weight messed up after saving? Or can you find the issue you are referring to?

jhedstrom queued 1: SillyArguments.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: SillyArguments.patch, failed testing.

cosmicdreams’s picture

Title: Three "silly arguments" where we assign the value of a variable to itself » One "silly assignments" where we assign the value of a variable to itself
Issue summary: View changes
Status: Needs work » Needs review
FileSize
730 bytes

Wow, blast from the past. Ran the inspection again and found there is only one now. It's the one found in Datelist.php. Here's a fresh patch.

Berdir’s picture

Are you sure that is the right fix? The !empty() condition actually looks for a different key, so maybe we should assign that?

cosmicdreams’s picture

I get what you're saying, I'll see if I can step through the code and watch more closely what's going on.

cosmicdreams’s picture

I'm having trouble triggering that line of code. Will investigate the test cases to see what it's doing to test it.

jhedstrom’s picture

The Datelist class seems to use a mix of both #timezone and #date_timezone, but only #date_timezone is documented. Seems like an indicator of a bug, or an undocumented BC layer.

mgifford’s picture

Status: Needs review » Needs work

Needs re-roll.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

Silly indeed!

This line was removed when #2799987: Datetime and Datelist element's timezone handling is fragile, buggy and wrongly documented was committed in Drupal 8.7.x. That issue appears to handle the 'bigger' issue that that was just a small part of. See commit 5731bc7d. Therefor closing this as outdated.