Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#44052 closed defect (bug) (fixed)

Missing parameter type for `login_header()`

Reported by: desrosj's profile desrosj Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.9.8 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: good-first-bug has-patch
Focuses: docs Cc:

Description

The login_header() function's third parameter is optional and accepts a WP_Error instance. The default value, though is a string, which is missing from the inline documentation.

To prevent someone from passing a string (which would not be added to a new WP_Error), if ( empty( $wp_error ) ) {} could be changed to if ( ! is_wp_error( $wp_error ) ) {}.

Attachments (9)

44052.diff (436 bytes) - added by sebastien@… 7 years ago.
44052.1.diff (936 bytes) - added by abdullahramzan 7 years ago.
44052.2.diff (1.2 KB) - added by abdullahramzan 7 years ago.
44052.3.diff (1.5 KB) - added by sebastien@… 7 years ago.
44052.3.2.diff (1.5 KB) - added by sebastien@… 7 years ago.
44052_4.diff (978 bytes) - added by sebastien@… 7 years ago.
44052.5.diff (987 bytes) - added by lbenicio 7 years ago.
44052.patch (891 bytes) - added by spyderbytes 6 years ago.
44052.6.diff (1.0 KB) - added by chetan200891 6 years ago.
Refreshed patch.

Download all attachments as: .zip

Change History (26)

@sebastien@…
7 years ago

#1 @desrosj
7 years ago

  • Keywords needs-refresh added

Thanks for the patch, @sebastien@….

The functions inline documentation also needs to be updated to indicate the $wp_error parameter could also be a string.

#2 @abdullahramzan
7 years ago

  • Keywords has-patch added; needs-refresh removed

#3 @desrosj
7 years ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#4 @flixos90
7 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 4.9.8 to 4.9.7

I think it makes more sense to leave the third parameter as a WP_Error only, but set its default value to null, which is the expected default value for optional object parameters. Since we create a new WP_Error if none is passed, we could adjust the documentation as follows: "Optional. The error to pass. Default is a new empty error."

#5 @sebastien@…
7 years ago

Added a new patch, hope I understood what @flixos90 said :-/

#6 @flixos90
7 years ago

@sebastien@…

Thanks for the updated patch! A few remarks:

  • The documentation for the parameter should continue to only state WP_Error as type. Also its description should be changed to use "Optional. The error to pass. Default is a new empty error."
  • Please remove line 41, as it introduces a parse error.
  • Please avoid unnecessary blank lines like in lines 46 and 47.
  • It seems your patch is not based on current trunk. Please ensure you create the diff file against the entirety and latest version of trunk.
Version 1, edited 7 years ago by flixos90 (previous) (next) (diff)

@lbenicio
7 years ago

#7 @sebastien@…
7 years ago

  • Keywords has-patch added; needs-patch removed

#8 @nikolam
6 years ago

Hello, I'm coming from WCEU!

All seems good to me. Code looks good, docs updated, tests written.

I don't think that there's anything I can do on this ticket. :)

#9 @abdullahramzan
6 years ago

  • Keywords 2nd-opinion added

#10 @desrosj
6 years ago

  • Keywords needs-refresh added; 2nd-opinion removed

None of the patches so far seem like a complete solution. The two main items here we should change to fix the original described problem:

  • The default for $wp_error should be changed to null to properly reflect the functions documentation. The @param tag description should also be changed to "Optional. The error to pass. Default is a new empty error."
  • The if ( empty( $wp_error ) ) conditional should be changed to check ! is_wp_error( $wp_error ) to ensure any passed value other than an instance of WP_Error is turned into one.

@spyderbytes
6 years ago

#11 @ocean90
6 years ago

  • Milestone changed from 4.9.7 to 4.9.8

4.9.7 has been released, moving to next milestone.

@chetan200891
6 years ago

Refreshed patch.

#12 @chetan200891
6 years ago

I have added refreshed patch 44052.6.diff

This ticket was mentioned in Slack in #core by chetan200891. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by joshuawold. View the logs.


6 years ago

#15 @pbiron
6 years ago

  • Keywords needs-refresh removed

#16 @SergeyBiryukov
6 years ago

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

In 43457:

Login and Registration: Set a better default value for $wp_error parameter in login_header().

To prevent someone from passing a string (which would not be added to a new WP_Error instance), check for is_wp_error() explicitly.

Props desrosj, chetan200891, spyderbytes, lbenicio, sebastien@…, abdullahramzan.
Fixes #44052.

#17 @SergeyBiryukov
6 years ago

In 43458:

Login and Registration: Set a better default value for $wp_error parameter in login_header().

To prevent someone from passing a string (which would not be added to a new WP_Error instance), check for is_wp_error() explicitly.

Props desrosj, chetan200891, spyderbytes, lbenicio, sebastien@…, abdullahramzan.
Merges [43457] to the 4.9 branch.
Fixes #44052.

Note: See TracTickets for help on using tickets.