WordPress.org

Make WordPress Core

Opened 6 weeks ago

Last modified 6 days 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 2nd-opinion
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@… 13 days ago.
44052.3.2.diff (1.5 KB) - added by sebastien@… 13 days ago.
44052_4.diff (978 bytes) - added by sebastien@… 13 days ago.
44052.5.diff (987 bytes) - added by lbenicio 10 days ago.

Download all attachments as: .zip

Change History (16)

@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
4 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
13 days 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@…
13 days ago

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

#6 @flixos90
13 days 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 13 days ago by flixos90 (previous) (diff)

@lbenicio
10 days ago

#7 @sebastien@…
9 days ago

  • Keywords has-patch added; needs-patch removed

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

  • Keywords 2nd-opinion added
Note: See TracTickets for help on using tickets.