WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#25393 closed enhancement (fixed)

Hook Docs: wp-login.php

Reported by: ShinichiN Owned by: DrewAPicture
Milestone: 3.7 Priority: normal
Severity: normal Version:
Component: Inline Docs Keywords: has-patch
Focuses: Cc:

Description

Adding inline documentation to wp-login.php

Attachments (7)

wp-login.diff (16.7 KB) - added by ShinichiN 6 years ago.
wp-login-2.diff (14.9 KB) - added by ShinichiN 6 years ago.
wp-login-3.diff (15.3 KB) - added by kpdesign 6 years ago.
Second pass
wp-login-4.diff (15.9 KB) - added by ShinichiN 6 years ago.
I modified wp-login.php following DrewAPicture's review.
wp-login-5.diff (15.9 KB) - added by ShinichiN 6 years ago.
New patch including the Changeset r25619
25393.diff (16.5 KB) - added by DrewAPicture 6 years ago.
Iterating on .5
25393.2.diff (16.9 KB) - added by DrewAPicture 6 years ago.
Final

Download all attachments as: .zip

Change History (26)

@ShinichiN
6 years ago

#1 @ShinichiN
6 years ago

I'm making another patch file. I'm sorry for bothering...

wp-login.diff above was from wp 3.6.1 and was old.

@ShinichiN
6 years ago

#2 @ShinichiN
6 years ago

wp-login-2.diff is a svn diff file.

I didn't know how to research the WP version of the do_action( 'login_form_' . $action ).

action hooks

login_enqueue_scripts
login_head
login_footer
lostpassword_post
retrieve_password
retrieve_password_key
login_init
'login_form_' . $action
lost_password
lostpassword_form
validate_password_reset
register_form
login_footer
login_form

filter hooks

shake_error_codes
login_headerurl
login_headertitle
login_body_class
login_message
login_errors
login_messages
allow_password_reset
retrieve_password_title
retrieve_password_message
post_password_expires
lostpassword_redirect
register
wp_signup_location
registration_redirect
login_redirect
wp_login_errors

#3 @ShinichiN
6 years ago

  • Keywords has-patch added

#4 @DrewAPicture
6 years ago

  • Owner set to kpdesign
  • Status changed from new to reviewing

@kpdesign
6 years ago

Second pass

#5 follow-up: @DrewAPicture
6 years ago

  • Keywords needs-patch added; has-patch removed

wp-login-3.diff is getting there. Unfortunately, there's still some work to do yet. Here are my notes:

  • "password retrieving process." should be "password retrieval process."
  • In my opinion, // Misspelled and deprecated should be in a doc block even if the action is deprecated.
  • allow_password_reset filter: "Filter password reset restriction boolean." is pretty vague. Can you better clarify what is filterable here? Also, "True as default." should be a short description of what the parameter is. It's fine to append the default to the end, but we should know what it signifies
  • "Fires before inserting the new md5 key into database." should be clearer. What md5 key are we talking about? The activation key?
  • login_init: what does "the main action of wp-login.php" refer to exactly?
  • lostpassword_redirect: I suggest splitting the follow out to a variable first, then using the variable in the filter and documenting it, respectively: !empty( $_REQUEST['redirect_to'] ) ? $_REQUEST['redirect_to'] : ''. Perhaps $lostpassword_redirect or something
  • lostpassword_form: The short description, "Fires in the lostpassword form to add extra hidden input.", makes an assumption about what specific type of action will happen here. Maybe something like this instead? "Fires in the lostpassword form before the hidden fields."
  • register: I suggest splitting the following out to a variable above the filter: printf( '<a href="%s">%s</a>', esc_url( wp_registration_url() ), __( 'Register' ) )
  • register_form: "form to add extra input." should be plural: "form to add extra inputs."
  • login_redirect: Split the following out to a variable above the filter, same as lostpassword_redirect: isset( $_REQUEST['redirect_to'] ) ? $_REQUEST['redirect_to'] : ''.

If you have questions about any of the suggestions I listed or need help, let me know. Happy to help :)

#6 in reply to: ↑ 5 @ShinichiN
6 years ago

I'm glad to reveive your review, thank you!

I'm going to work on this tonight, japan time:)

@ShinichiN
6 years ago

I modified wp-login.php following DrewAPicture's review.

#7 @ShinichiN
6 years ago

  • Keywords has-patch added; needs-patch removed

@ShinichiN
6 years ago

New patch including the Changeset r25619

#8 @ShinichiN
6 years ago

wp-login-5.diff includes

  • patch wp-login-3.diff
  • DrewAPicture's review
  • helen's patch r25619

Thank you!

@DrewAPicture
6 years ago

Iterating on .5

#9 @DrewAPicture
6 years ago

  • Milestone changed from Awaiting Review to 3.7
  • Owner changed from kpdesign to DrewAPicture

@DrewAPicture
6 years ago

Final

#10 follow-up: @DrewAPicture
6 years ago

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

In 25701:

Inline documentation for hooks in wp-login.php.

Props ShinichiN, kpdesign.
Fixes #25393.

#11 in reply to: ↑ 10 ; follow-up: @tivnet
6 years ago

Replying to DrewAPicture:

This one is wrong

* @param int  $user_data->ID The ID of the user attempting to reset a password.

because it re-declares $user_data as int (->ID is ignored)

#12 in reply to: ↑ 11 ; follow-up: @DrewAPicture
6 years ago

Replying to tivnet:

Replying to DrewAPicture:

This one is wrong

* @param int  $user_data->ID The ID of the user attempting to reset a password.

because it re-declares $user_data as int (->ID is ignored)

Not sure I'm following your logic. The docblock redeclares $user_data as int or code redeclares it as int somewhere I'm not seeing? If it's the former, we could always store in variable before the filter is evaluated.

#13 in reply to: ↑ 12 ; follow-up: @tivnet
6 years ago

Replying to DrewAPicture:

The docblock applies @param int to the $user_data for the code following it.

In general, this use of docblocks is illegal, according to

http://www.phpdoc.org/docs/latest/for-users/phpdoc/basic-syntax.html#which-elements-can-have-a-docblock

So, why not to keep is as just comments, without docblock @-isms? Using @s will only disturb phpdocumentor, IMO.

#14 in reply to: ↑ 13 ; follow-up: @DrewAPicture
6 years ago

Replying to tivnet:

Replying to DrewAPicture:

The docblock applies @param int to the $user_data for the code following it.

In general, this use of docblocks is illegal, according to

http://www.phpdoc.org/docs/latest/for-users/phpdoc/basic-syntax.html#which-elements-can-have-a-docblock

So, why not to keep is as just comments, without docblock @-isms? Using @s will only disturb phpdocumentor, IMO.

We use a customized documentation schema that draws inspiration from phpdoc but doesn't follow it to a tee. A notable difference, for instance, is that phpdoc doesn't provide any support for an action and filter-driven architecture, such as WordPress is.

The style of documentation you see here for actions and filters is consistent with, and adheres to, the standards outlined in the document linked above.

If you'd like to join the discussion, the inline-docs team meet weekly on Wednesdays in #wordpress-sfd on Freenode, at 18:00 UTC.

Last edited 6 years ago by DrewAPicture (previous) (diff)

#15 in reply to: ↑ 14 ; follow-up: @tivnet
6 years ago

Replying to DrewAPicture:

Do what you want, but remember that this makes only trouble for those who use good IDEs (which you have listed in the Standards page). As I said, non-standard use of docblocks breaks the code validation.

#16 in reply to: ↑ 15 ; follow-up: @DrewAPicture
6 years ago

Replying to tivnet:

Replying to DrewAPicture:

Do what you want, but remember that this makes only trouble for those who use good IDEs (which you have listed in the Standards page). As I said, non-standard use of docblocks breaks the code validation.

FWIW, I use PhpStorm and I haven't had a single problem.

Again, if you have concerns, you're more than welcome to voice them at the weekly chat. Having this discussion in one of a multitude of hook docs tickets isn't really going to be the most effective way to do that. I appreciate the input though :)

#17 in reply to: ↑ 16 ; follow-up: @tivnet
6 years ago

Replying to DrewAPicture:

Sorry, I won't be able to join the chat this Wed...

Some of the phpStorm inspections for wp-login.php:

  • PHPDoc comment does not match function or method signature (at line 25)
  • Field 'ID' not found in class (at line 325)
  • Method 'update' not found in class (at line 351) - this one because there is no /** @var wpdb $wpdb */

Now, this one concerns me even more:
@param bool $is_admin in vars.php
There is no such var here. This is all wrong...

#18 in reply to: ↑ 17 ; follow-up: @tmtoy
6 years ago

Replying to tivnet:

Now, this one concerns me even more:
@param bool $is_admin in vars.php
There is no such var here. This is all wrong...

Excuse me for cutting in...
This issue has been resolved already.
http://core.trac.wordpress.org/changeset/25718

#19 in reply to: ↑ 18 @tivnet
6 years ago

Replying to tmtoy:
Yes, saw that yesterday, thanks

Note: See TracTickets for help on using tickets.