WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 19 months ago

Last modified 19 months ago

#44052 closed defect (bug) (fixed)

Missing parameter type for `login_header()`

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

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@… 22 months ago.
44052.1.diff (936 bytes) - added by abdullahramzan 22 months ago.
44052.2.diff (1.2 KB) - added by abdullahramzan 22 months ago.
44052.3.diff (1.5 KB) - added by sebastien@… 21 months ago.
44052.3.2.diff (1.5 KB) - added by sebastien@… 21 months ago.
44052_4.diff (978 bytes) - added by sebastien@… 21 months ago.
44052.5.diff (987 bytes) - added by lbenicio 21 months ago.
44052.patch (891 bytes) - added by spyderbytes 20 months ago.
44052.6.diff (1.0 KB) - added by chetan200891 20 months ago.
Refreshed patch.

Download all attachments as: .zip

Change History (26)

#1 @desrosj
22 months 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
22 months ago

  • Keywords has-patch added; needs-refresh removed

#3 @desrosj
21 months ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#4 @flixos90
21 months 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@…
21 months ago

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

#6 @flixos90
21 months 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 21 months ago by flixos90 (previous) (next) (diff)

@lbenicio
21 months ago

#7 @sebastien@…
21 months ago

  • Keywords has-patch added; needs-patch removed

#8 @nikolam
20 months 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
20 months ago

  • Keywords 2nd-opinion added

#10 @desrosj
20 months 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
20 months ago

#11 @ocean90
20 months ago

  • Milestone changed from 4.9.7 to 4.9.8

4.9.7 has been released, moving to next milestone.

@chetan200891
20 months ago

Refreshed patch.

#12 @chetan200891
20 months ago

I have added refreshed patch 44052.6.diff

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


20 months ago

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


20 months ago

#15 @pbiron
19 months ago

  • Keywords needs-refresh removed

#16 @SergeyBiryukov
19 months 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
19 months 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.