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.

Files: 
CommentFileSizeAuthor
#20 2104447_20.patch730 bytescosmicdreams
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,606 pass(es).
[ View ]

Comments

cosmicdreams’s picture

StatusFileSize
new2.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch SillyArguments.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
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
StatusFileSize
new730 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,606 pass(es).
[ View ]

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.