Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#20 | 2104447_20.patch | 730 bytes | cosmicdreams |
Comments
Comment #1
cosmicdreams CreditAttribution: cosmicdreams commentedComment #2
cosmicdreams CreditAttribution: cosmicdreams commentedComment #4
jhodgdonThis 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.
Comment #5
jhodgdon#1: SillyArguments.patch queued for re-testing.
Comment #7
swentel CreditAttribution: swentel commented#1: SillyArguments.patch queued for re-testing.
Comment #8
swentel CreditAttribution: swentel commentedFailed because of #2057401: Make the node entity database schema sensible - should be fine now.
Comment #10
swentel CreditAttribution: swentel commented#1: SillyArguments.patch queued for re-testing.
Comment #12
jhodgdon#1: SillyArguments.patch queued for re-testing.
Comment #13
cosmicdreams CreditAttribution: cosmicdreams commentedLooks like we're good then? RTBC?
Comment #14
jhodgdonComment #15
BerdirThis 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.
Comment #16
BerdirSetting 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?
Comment #17
jhodgdonDo you mean: #941266: Order of terms with same weight messed up after saving? Or can you find the issue you are referring to?
Comment #20
cosmicdreams CreditAttribution: cosmicdreams commentedWow, 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.
Comment #21
BerdirAre you sure that is the right fix? The !empty() condition actually looks for a different key, so maybe we should assign that?
Comment #22
cosmicdreams CreditAttribution: cosmicdreams commentedI get what you're saying, I'll see if I can step through the code and watch more closely what's going on.
Comment #23
cosmicdreams CreditAttribution: cosmicdreams commentedI'm having trouble triggering that line of code. Will investigate the test cases to see what it's doing to test it.
Comment #24
jhedstromThe
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.Comment #25
mgiffordNeeds re-roll.
Comment #34
quietone CreditAttribution: quietone as a volunteer commentedSilly 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.