Part of meta-issue #2002650: [meta] improve maintainability by removing unused local variables

File /core/modules/image/image.module

Line 251: Unused local variable $style_name

Files: 
CommentFileSizeAuthor
#10 2080391-remove-unused-variables-new.patch9.25 KBjohnmcc
PASSED: [[SimpleTest]]: [MySQL] 58,831 pass(es).
[ View ]
#10 interdiff.txt2.63 KBjohnmcc
#7 2080391-remove-unused-variables.patch842 bytesjohnmcc
PASSED: [[SimpleTest]]: [MySQL] 59,328 pass(es).
[ View ]
#5 drupal8.other_.2080391-5.patch510 bytesJeroenT
PASSED: [[SimpleTest]]: [MySQL] 58,676 pass(es).
[ View ]
#2 2080391-remove-unused-variables.patch8.91 KBchertzog
FAILED: [[SimpleTest]]: [MySQL] 59,157 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new8.91 KB
FAILED: [[SimpleTest]]: [MySQL] 59,157 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 2080391-remove-unused-variables.patch, failed testing.

  1. +++ b/core/modules/image/image.module
    @@ -239,8 +239,8 @@ function image_file_download($uri) {
         // Discard the first part of the path (styles).
         array_shift($args);
    -    // Get the style name from the second part.
    -    $style_name = array_shift($args);
    +    // Remove the style name from the path.
    +    array_shift($args);
         // Remove the scheme from the path.
         array_shift($args);

    This is doing 3 array_shift in a row. I think it would be better to use array_slice() passing as offset 0 and as length 3.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Form/FormTest.php
    @@ -238,7 +238,7 @@ function testRequiredCheckboxesRadio() {
       function testRequiredTextfieldNoTitle() {
         $form = $form_state = array();
    -    $form = form_test_validate_required_form_no_title($form, $form_state);
    +    form_test_validate_required_form_no_title($form, $form_state);

    I think that this part shouldn't be touched, because the form_test_validate_required_form_no_title() function does not get form as reference and $form is still used later on.

Status:Needs work» Needs review
StatusFileSize
new510 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,676 pass(es).
[ View ]

Removed the unused local variable $style_name.

Status:Needs review» Needs work

This last patch doesn't look good to me.

+++ b/core/modules/image/image.module
@@ -223,8 +223,6 @@ function image_file_download($uri) {
-    // Get the style name from the second part.
-    $style_name = array_shift($args);

Removing entirely this part will break the hook_implementation because the style name is not shifted out from the args anymore.

StatusFileSize
new842 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,328 pass(es).
[ View ]

I agree with #6 - we still need to remove the style name.

Here's a patch that takes into account #3, using array_slice rather than three array_shifts. (My first patch, be gentle!)

Status:Needs work» Needs review

Status:Needs review» Needs work

The usage of array_slice is ok, but if you look at patch #2, there were many others fixes which are omitted in this patch. Please re-submit it, and make sure to fix patch #2 with my second remark in comment #4.

Status:Needs work» Needs review
StatusFileSize
new2.63 KB
new9.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,831 pass(es).
[ View ]

Thanks for the feedback! I've reworked patch #2. Couple of things to note:

  • The patch wouldn't apply due to changes in content_translation.pages.inc, so I reworked the patch to take that into account.
  • Reworked image.module as discussed
  • I reworked database_test.module so that $query->addField was still happening. The previous patch removed this call altogether. I believe this was the cause of the previous patch failing, not the change to FormTest.php that was mentioned in #4. (If you still think that removing $form as per #4 is a problem, then by all means feed back to me again, but I can't see where this variable has been re-used.)

I'll take a second look later just to be sure, but so far it looks pretty good :)

Status:Needs review» Reviewed & tested by the community

Thank you for the patch :)

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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