Follow-up from #2080343: Remove Unused local variables from system module.

CommentFileSizeAuthor
#17 2187477-17.patch15.95 KBvaibhavjain
#14 2187477-14.patch15.88 KBvisabhishek
#8 2187477-8.patch17.39 KBvaibhavjain
#6 2187477-6.patch20.33 KBlokapujya
#2 2187477-2.patch21.16 KBlokapujya
#1 2187477-1.patch0 byteslokapujya

Comments

lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented
StatusFileSize
new 2187477-1.patch0 bytes

Remove unused use statements and extra semi colons from system module.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented
StatusFileSize
new 2187477-2.patch21.16 KB
lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented

Status: Needs review » Needs work

The last submitted patch, 2: 2187477-2.patch, failed testing.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented
Status: Needs work » Needs review
lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented
StatusFileSize
new 2187477-6.patch20.33 KB
lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented
Assigned: lokapujya » Unassigned
vaibhavjain’s picture

vaibhavjain
he/him
Primary language Hindi
Location Delhi
commented
StatusFileSize
new 2187477-8.patch17.39 KB
lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented

Thanks for the reroll. Please add a comment to describe what changed or specify that it was a straight reroll. Also, The previous patch had fixed some double semi-colons; I think we should keep those changes.

vaibhavjain’s picture

vaibhavjain
he/him
Primary language Hindi
Location Delhi
commented

lokapujya, this was a straight reroll, some lines were added above it and some were removed already which were removed in this patch.
For the double semicolon; I thought it was a bug, and I removed those.
I will add those and attach the patch again.

Just for my information, why do we need these double semicolons ?

vaibhavjain’s picture

vaibhavjain
he/him
Primary language Hindi
Location Delhi
commented

Hey Lokapujya,

Just checked on the double semicolon thing in files - "core/modules/system/lib/Drupal/system/Tests/Entity/EntityLanguageTestBase.php" and "core/modules/system/lib/Drupal/system/Form/RegionalForm.php".
For both the cases the double colon has already been removed, so no need to include that in the patch, and I believe the same patch as mentioned in #8 can work.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented

It looks good to me. I think a 3rd person should review the change. I pulled the changes over from https://drupal.org/node/2080343#comment-8386367 in order to separate the changes from that issue. Even though the change passes Simpletest, I'd like someone experienced with PHP OOP to say that it's ok to remove the use statements.

LinL’s picture

LinL commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies, tagging for reroll.

visabhishek’s picture

visabhishek commented
Status: Needs work » Needs review
StatusFileSize
new 2187477-14.patch15.88 KB

simply reroll

martin107’s picture

martin107 commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

patch applies cleanly, without error

does what is asked, and nothing more

+1

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2187477-14.patch, failed testing.

vaibhavjain’s picture

vaibhavjain
he/him
Primary language Hindi
Location Delhi
commented
Status: Needs work » Needs review
StatusFileSize
new 2187477-17.patch15.95 KB

Adding rerolled patch.

vastav’s picture

vastav commented

14: 2187477-14.patch queued for re-testing.

martin107’s picture

martin107 commented
Status: Needs review » Reviewed & tested by the community

clean reroll, back to rtbc

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit 64b838a on 8.x by webchick: 
    Issue #2187477 by lokapujya, vaibhavjain, visabhishek: Remove Unused use...

Status: Fixed » Closed (fixed)

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