Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chertzog’s picture

Status: Active » Needs review
FileSize
8.91 KB

Status: Needs review » Needs work

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

Bladedu’s picture

  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.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
510 bytes

Removed the unused local variable $style_name.

Bladedu’s picture

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.

johnmcc’s picture

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!)

johnmcc’s picture

Status: Needs work » Needs review
Bladedu’s picture

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.

johnmcc’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
9.25 KB

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.)
Bladedu’s picture

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

Bladedu’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for the patch :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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

YesCT’s picture

Issue summary: View changes