WordPress.org

Make WordPress Core

Opened 6 weeks ago

Last modified 26 hours ago

#44052 new defect (bug)

Missing parameter type for `login_header()`

Reported by: desrosj Owned by:
Milestone: 4.9.7 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: good-first-bug has-patch needs-refresh
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 (7)

44052.diff (436 bytes) - added by sebastien@… 6 weeks ago.
44052.1.diff (936 bytes) - added by abdullahramzan 6 weeks ago.
44052.2.diff (1.2 KB) - added by abdullahramzan 6 weeks ago.
44052.3.diff (1.5 KB) - added by sebastien@… 2 weeks ago.
44052.3.2.diff (1.5 KB) - added by sebastien@… 2 weeks ago.
44052_4.diff (978 bytes) - added by sebastien@… 2 weeks ago.
44052.5.diff (987 bytes) - added by lbenicio 2 weeks ago.

Download all attachments as: .zip

Change History (17)

@sebastien@…
6 weeks ago

#1 @desrosj
6 weeks 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
6 weeks ago

  • Keywords has-patch added; needs-refresh removed

#3 @desrosj
5 weeks ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#4 @flixos90
2 weeks 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@…
2 weeks ago

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

#6 @flixos90
2 weeks 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. See https://make.wordpress.org/core/handbook/tutorials/working-with-patches/
Last edited 2 weeks ago by flixos90 (previous) (diff)

@lbenicio
2 weeks ago

#7 @sebastien@…
13 days ago

  • Keywords has-patch added; needs-patch removed

#8 @nikolam
10 days 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
10 days ago

  • Keywords 2nd-opinion added

#10 @desrosj
26 hours 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.
Note: See TracTickets for help on using tickets.