WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#35781 closed defect (bug) (fixed)

Incorrect use of PHP method compact

Reported by: maweder Owned by: SergeyBiryukov
Milestone: 4.4.3 Priority: normal
Severity: normal Version: 4.4
Component: Mail Keywords: fixed-major needs-unit-tests
Focuses: Cc:

Description

This is a follow-up to #18926.

$mail_error_data = compact( 'to', 'subject', 'message', $headers, $attachments );

instead of ...

$mail_error_data = compact( $to, $subject, $message, $headers, $attachments );

Attachments (3)

35781.patch (514 bytes) - added by Ankit K Gupta 2 years ago.
added patch
35781.1.patch (516 bytes) - added by Ankit K Gupta 2 years ago.
Updated patch file
35781.2.patch (985 bytes) - added by shahpranaf 2 years ago.

Download all attachments as: .zip

Change History (18)

#1 @dd32
2 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

The existing format is how PHP's compact() method works - http://php.net/compact

#2 @johnbillion
2 years ago

  • Keywords needs-patch added
  • Milestone set to 4.4.3
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Version changed from trunk to 4.4

This is a valid bug. The current code uses variable references instead of variable names.

@Ankit K Gupta
2 years ago

added patch

#3 @Ankit K Gupta
2 years ago

@maweder Welcome to the trac! Thanks for reporting the bug, I have added patch as per your suggestion.

#4 @Ankit K Gupta
2 years ago

  • Keywords has-patch added; needs-patch removed

#5 @Ankit K Gupta
2 years ago

  • Keywords reporter-feedback added

@Ankit K Gupta
2 years ago

Updated patch file

#6 @SergeyBiryukov
2 years ago

Introduced in [34221].

#7 follow-up: @SergeyBiryukov
2 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from reopened to closed

In 36688:

Mail: Correct compact() usage in wp_mail().

Props Ankit K Gupta, maweder.
Fixes #35781 for trunk.

#8 @SergeyBiryukov
2 years ago

  • Keywords fixed-major added; reporter-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#9 in reply to: ↑ 7 @maweder
2 years ago

Replying to SergeyBiryukov:

In 36688:

Mail: Correct compact() usage in wp_mail().

Props Ankit K Gupta, maweder.
Fixes #35781 for trunk.

Hi Sergey, the change in 36688 is again incorrect.

From http://php.net/manual/en/function.compact.php: Each parameter can be either a string containing the name of the variable, or an array of variable names. The array can contain other arrays of variable names inside it; compact() handles it recursively.

$headers and $attachments, these are arrays and not names of variables. The correct code:

$mail_error_data = compact( 'to', 'subject', 'message', $headers, $attachments );

Thanks & Greets

@shahpranaf
2 years ago

#10 @shahpranaf
2 years ago

@maweder Firstly, Welcome to the WP trac.

Yes you are correct. 'headers' and 'attachments' are arrays and not the strings. I've done changes at both places i.e inside wp_mail() function also.

Thanks

#11 @ocean90
2 years ago

  • Keywords needs-unit-tests added

"needs-unit-test" so it doesn't happen again.

#12 follow-up: @dd32
2 years ago

  • Keywords needs-unit-tests removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Not to sound like a broken record, but the current code in trunk is correct.

$to = 'to@example.com';
$from = 'from@example.com';
$headers = array( 'Header', 'Two Header' );
$Header = 'Should not appear';

var_dump( compact( 'to', 'from', 'headers' ) );

will result in :

array (size=3)
  'to' => string 'to@example.com' (length=14)
  'from' => string 'from@example.com' (length=16)
  'headers' => 
    array (size=2)
      0 => string 'Header' (length=6)
      1 => string 'Two Header' (length=10)

Whereas var_dump( compact( 'to', 'from', $headers ) ); would result in:

array (size=3)
  'to' => string 'to@example.com' (length=14)
  'from' => string 'from@example.com' (length=16)
  'Header' => string 'Should not appear' (length=17)

The array handling in compact() is to pass an array of variable names, when the variable you want in the output is itself a array it should be specified as a string.

#13 @johnbillion
2 years ago

  • Keywords needs-unit-tests added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Confirmed, [36688] fixed the issue and the values passed to the wp_mail_failed action are as expected. A unit test would help us out here though.

#14 in reply to: ↑ 12 @maweder
2 years ago

My mistake. You're right.

Replying to dd32:

Not to sound like a broken record, but the current code in trunk is correct.

$to = 'to@example.com';
$from = 'from@example.com';
$headers = array( 'Header', 'Two Header' );
$Header = 'Should not appear';

var_dump( compact( 'to', 'from', 'headers' ) );

will result in :

array (size=3)
  'to' => string 'to@example.com' (length=14)
  'from' => string 'from@example.com' (length=16)
  'headers' => 
    array (size=2)
      0 => string 'Header' (length=6)
      1 => string 'Two Header' (length=10)

Whereas var_dump( compact( 'to', 'from', $headers ) ); would result in:

array (size=3)
  'to' => string 'to@example.com' (length=14)
  'from' => string 'from@example.com' (length=16)
  'Header' => string 'Should not appear' (length=17)

The array handling in compact() is to pass an array of variable names, when the variable you want in the output is itself a array it should be specified as a string.

#15 @SergeyBiryukov
2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 37081:

Mail: Correct compact() usage in wp_mail().

Merges [36688] to the 4.4 branch.

Props Ankit K Gupta, maweder.
Fixes #35781.

Note: See TracTickets for help on using tickets.