#25229 closed task (blessed) (fixed)
Add Inline Docs for Hooks
Reported by: | rzen | Owned by: | DrewAPicture |
---|---|---|---|
Milestone: | 3.7 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Inline Docs | Keywords: | |
Focuses: | Cc: |
Description (last modified by )
This trac ticket corresponds with a make/core blog post of the same name, an initative we're kicking off in 3.7, as discussed in this earlier make/core post.
Get Involved!
We'd love your help contributing! You can get started by following the "how to contribute" section and leaving a comment on the make/core post.
Keeping Discussions Focused
Any discussion about the specifics of a patch itself (e.g. "it should say this") should happen here on trac, like it always does. Any discussion about the broader scope of what we’re trying to do (e.g. "all hook docs should do this") should take place on the make/core P2.
Keeping Your Patch Clean
You may have claimed several files, but it would probably be best if you submitted individual patches for each file separately. That makes the changeset easier to grok, and the patch easeir to apply.
Attachments (73)
Change History (209)
#2
@
11 years ago
- Milestone changed from Awaiting Review to 3.7
- Type changed from enhancement to task (blessed)
#5
@
11 years ago
@rzen: wp-comments-post.diff. Looks like your @param
lines need periods :)
#15
follow-up:
↓ 16
@
11 years ago
I wasn't sure of the best way to handle the use of the
do_action('comment_form');
in the deprecated wp-includes/theme-compat/ files. I added a note in the long description that this use of the action is in a deprecate file. Better to use the @deprecated tag in this case?
#16
in reply to:
↑ 15
@
11 years ago
Replying to natejacobs:
I wasn't sure of the best way to handle the use of the
do_action('comment_form');in the deprecated wp-includes/theme-compat/ files. I added a note in the long description that this use of the action is in a deprecate file. Better to use the @deprecated tag in this case?
This is actually a //duplicate_hook
situation. comment_form
is a hook included in the comment_form()
function.
#24
@
11 years ago
I have a couple of questions regarding the diff I submitted.
First, regarding the nav_menu_link_attributes
filter: the first argument is an array that I feel could benefit from having it's structure documented similar to the way Argument arrays are supposed to be documented. So I documented it like it was an Arguments array, but I wanted to know if that is how it should be done?
Second, I wanted to know if I'm documenting the $args
parameter properly, with a @see
pointing towards the originating function.
And last, do we have a standard for how to document a parameter that is a field in an object? For example, some actions have a parameter like $foo->bar
, so do we document it as @param type $foo->bar description.
?
Thanks for all the help,
Faison
@
11 years ago
Second Pass, forgot to add description for walker_nav_menu_start_el's first parameter
@
11 years ago
Adds filter documentation for wp-includes/author-template.php. Also updates outdated function documentation.
#27
follow-up:
↓ 28
@
11 years ago
Question: For a duplicate hook, how do we know which instance of the hook should be documented and which should be marked duplicate? Also, I know the standard being used is // Duplicate hook
, but would it make sense to have some reference to where it is documented so someone could more easily find it?
#28
in reply to:
↑ 27
@
11 years ago
Replying to dustyf:
Question: For a duplicate hook, how do we know which instance of the hook should be documented and which should be marked duplicate? Also, I know the standard being used is
// Duplicate hook
, but would it make sense to have some reference to where it is documented so someone could more easily find it?
The standard being used is very specifically //duplicate_hook
(no space, underscore between the words).
And you'd want to search for multiplies of the same hook tag and svn blame those lines to find the "first" use of it in terms of earliest revision. The first use gets a docblock, the second (third, fourth, etc) get //duplicate_hook
.
#31
@
11 years ago
http.php.2.diff looks pretty good. Looks like you still need to do the allowed origins for the allowed_http_origins
hook. Take a look at the hooks example for how to do this.
@
11 years ago
wp-activate.php inline hook documentation, and minor corrections on version for other documentation.
#33
@
11 years ago
options.patch looks good to me. Nice first patch @siobhyb!
#34
follow-up:
↓ 35
@
11 years ago
Question. If a hook is in the middle of a block of HTML do you comment above the html or inside of it? I assume above it as it would be cleaner.
Ex. wp-admin/user-new.php
<form action="" method="post" name="adduser" id="adduser" class="validate"<?php do_action('user_new_form_tag');?>>
Another question. What if the hook is embedded in a long conditional?
if ( is_multisite() && current_user_can( 'promote_users' ) && ! wp_is_large_network( 'users' ) && ( is_super_admin() || apply_filters( 'autocomplete_users_for_site_admins', false ) ) ) { wp_enqueue_script( 'user-suggest' ); }
#35
in reply to:
↑ 34
;
follow-up:
↓ 37
@
11 years ago
Replying to bftrick:
Question. If a hook is in the middle of a block of HTML do you comment above the html or inside of it? I assume above it as it would be cleaner.
[...]
Another question. What if the hook is embedded in a long conditional?
From a technical standpoint, WP-Parser#28 is tracking the implementation. For conditionals, you should be able to put it before the entire conditional, and likewise for embedded HTML, as it just holds on to the tag until the next action/filter it sees.
#37
in reply to:
↑ 35
@
11 years ago
Replying to rmccue:
Replying to bftrick:
Question. If a hook is in the middle of a block of HTML do you comment above the html or inside of it? I assume above it as it would be cleaner.
[...]
Another question. What if the hook is embedded in a long conditional?
From a technical standpoint, WP-Parser#28 is tracking the implementation. For conditionals, you should be able to put it before the entire conditional, and likewise for embedded HTML, as it just holds on to the tag until the next action/filter it sees.
thanks for the clarification! :)
#38
@
11 years ago
Weekly Triage/Recap – Sept. 11
After yesterdays weekly inline-docs chat, we decided to start doing a weekly triage of patches on this ticket to both:
- Ensure submitters get feedback on remaining patches
- Committers know what's ready to go in
Patches committed
Patch | File | Changeset |
---|---|---|
wp-comments-post.diff | wp-comments-post.php | [25249], [25251] |
admin-footer.diff | wp-admin/admin-footer.php | [25252] |
index.diff | wp-admin/index.php | [25250] |
wp-trackback.diff | wp-trackback | [25253] |
xmlrpc.diff | xmlrpc.php | [25281] |
wp-comment.diff | wp-admin/comment.php | [25282] |
custom-background.diff | wp-admin/custom-background.php | [25283] |
wp-db.diff | wp-includes/wp-db.php | [25284] |
rss.diff | wp-includes/rss.php | [25286] |
http.php.4.diff | wp-includes/http.php | [25302] |
options.patch | wp-admin/options.php | [25372] |
Contributions by: rzen, natejacobs, bananastalktome, nacin, bftrick, dustyf, tw2113, siobhyb
Patches in review
ms.diff for wp-admin/includes/ms.php — @enej
@since
tags should use the three-digit x.x.x style.- For the
new_admin_email_content
andnew_user_email_content
filters, I'd suggest pullout the email text string into a variable before the filter doc blocks, then referencing that directly in the@param
lines - For short descriptions, focus more on what's being filtered, not why.
// For instance, this: "Allows you to modify the tables that you want to be dropped when the blog is deleted." //should be more like: "Filter the tables to drop when a blog is deleted." //This: "Allows you to modify the upload base directory that is going to be deleted when the blog is deleted." // should be more like: "Filter the upload base directory to be deleted when a blog is deleted."
wp-activate.diff for wp-activate.php — @nullvariable
- Remove the action long descriptions as they are unnecessary and redundant.
- @since versions should be in the x.x.x 3-digit style
- Remove the functional doc changes, just focus on the actions here :)
- Side note: @MU shouldn't ever be changed since it signifies that the function came from WPMU
admin-ajax.diff for wp-admin/admin-ajax.php — @nullvariable
- Just
//duplicate_hook
will suffice for theadmin_init
hook - Short descriptions should start a capital letter and end with a period.
- Add a blank link between the long description and the
@link
@since
should use the 3-digit x.x.x style
Patches ready for commit
Patch | File | Props | Changeset | Notes |
---|---|---|---|---|
siobhyb | [25411] | Committed by helen | ||
Faison | [25410] | wp_nav_menu filter | ||
natejacobs | [25423] | Committed by SergeyBiryukov | ||
NikV | [25424] | Committed by SergeyBiryukov | ||
Frank Klein | [25429] | Committed by SergeyBiryukov | ||
mordauk | [25434] | Committed by SergeyBiryukov | ||
mordauk | [25435] | Committed by SergeyBiryukov | ||
mordauk | [25455] | Committed by SergeyBiryukov | ||
bftrick | [25470] | Committed by SergeyBiryukov | ||
dustyf | [25479] | Committed by SergeyBiryukov | ||
dllh | [25474] | Committed by westi | ||
betzster | [25475] | Committed by westi | ||
wp-links-opml.3.diff | wp-links-opml.php | netweb | Ready for commit |
#40
@
11 years ago
Faison - for the future, please make patches from either the root of develop or src, and strip trailing whitespace. :)
#44
@
11 years ago
@Frank Klein: author-template.2.patch is looking better. Some notes:
- For the
'the_author_' . $field
filter,$field
is part of the filter hook, not a parameter. So maybe just explain that that portion of the hook name is dynamic in a long description @param string HTML link
needs a period at the end$author_id The ID of a user.
should probably sub 'user' for 'author'- The
$is_multi_author
parameter description should probably be something more like "Whether $is_multi_author should evaluate as true"
Thanks for splitting the original patch apart :)
#45
@
11 years ago
@bftrick: user-new.diff is a good start.
- All short descriptions for hook docs need a period at the end
- Typo in "sanitzed", should probably also mention the value is unslashed here
- "Adds additional tags to the adduser form" should probably be something more like "Fires inside the adduser form tag"
Thanks for the patches.
#48
@
11 years ago
Inline docs for wp-admin/includes/comment.php. Just one for the
comment_edit_pre` filter.
http://core.trac.wordpress.org/attachment/ticket/25229/comment-php.patch
#49
follow-up:
↓ 51
@
11 years ago
@mordauk:
comment-php.patch — Per the inline docs standards:
- Need a period on the short description
- A blank line after the
@since
line - A period at the end of the description on the
@param
line.
- Describe when the action fires and where. "Bottom of export filters form", should be more like "Fires after the export filters form"
- Periods after short descriptions
- Blank line after
@since
lines @since
value should follow the 3-digit, x.x.x style
General comment: Clarity is preferred over succinctness.
#50
@
11 years ago
author-template.4.patch is ready for commit. Props @Frank Klein.
#51
in reply to:
↑ 49
@
11 years ago
Replying to DrewAPicture:
@mordauk:
comment-php.patch — Per the inline docs standards:
- Need a period on the short description
- A blank line after the
@since
line- A period at the end of the description on the
@param
line.
- Describe when the action fires and where. "Bottom of export filters form", should be more like "Fires after the export filters form"
- Periods after short descriptions
- Blank line after
@since
lines@since
value should follow the 3-digit, x.x.x styleGeneral comment: Clarity is preferred over succinctness.
Patches refreshed, thanks!
#53
follow-ups:
↓ 54
↓ 56
@
11 years ago
@mordauk:
In the future, please don't overwrite old patches with the new ones, makes it hard to track progress and context of comments.
Both comment-php.patch and export-php.patch are looking pretty good. One thing left:
- When you're documenting a filter, if you find yourself saying "The whatever value", in the short description, it should really be "Filter the whatever value". Yes, the
apply_filters()
call does in-effect evaluate to that value, but the point is that's it's a filter doc, and not just a statement of value :)
#54
in reply to:
↑ 53
;
follow-up:
↓ 55
@
11 years ago
Replying to DrewAPicture:
@mordauk:
In the future, please don't overwrite old patches with the new ones, makes it hard to track progress and context of comments.
How do you name the subsequent patches? foo-1.diff
?
#55
in reply to:
↑ 54
@
11 years ago
Replying to bftrick:
How do you name the subsequent patches?
foo-1.diff
?
Just don't check the "Replace existing attachment" checkbox. Trac will then name it foo.2.diff
, etc.
#56
in reply to:
↑ 53
@
11 years ago
Replying to DrewAPicture:
@mordauk:
In the future, please don't overwrite old patches with the new ones, makes it hard to track progress and context of comments.
Both comment-php.patch and export-php.patch are looking pretty good. One thing left:
- When you're documenting a filter, if you find yourself saying "The whatever value", in the short description, it should really be "Filter the whatever value". Yes, the
apply_filters()
call does in-effect evaluate to that value, but the point is that's it's a filter doc, and not just a statement of value :)
Done, thanks for the extra feedback :)
#60
follow-up:
↓ 69
@
11 years ago
@bftrick – user-new.2.diff So close!
- I'd like you to add "Default is _whatever_" for the
autocomplete_users_for_site_admins
andshow_password_fields
filter hooks, if you could. Then this one is ready to go in!
#65
follow-up:
↓ 66
@
11 years ago
@mordauk:
wp-includes-load.php.diff, just remove the empty line after the @since. The empty line only applies if there are other tags/other stuff in the doc block following it.
#66
in reply to:
↑ 65
@
11 years ago
Replying to DrewAPicture:
@mordauk:
wp-includes-load.php.diff, just remove the empty line after the @since. The empty line only applies if there are other tags/other stuff in the doc block following it.
Refreshed.
#67
@
11 years ago
@SergeyBiryukov, @helen wp-includes-load.php.2.diff looks ready for commit, props @mordauk
#69
in reply to:
↑ 60
;
follow-up:
↓ 70
@
11 years ago
Replying to DrewAPicture:
@bftrick – user-new.2.diff So close!
- I'd like you to add "Default is _whatever_" for the
autocomplete_users_for_site_admins
andshow_password_fields
filter hooks, if you could. Then this one is ready to go in!
Alright just attached a new diff. Hopefully I understood you correctly. :)
#70
in reply to:
↑ 69
@
11 years ago
Replying to bftrick:
Replying to DrewAPicture:
Alright just attached a new diff. Hopefully I understood you correctly. :)
Yep, looks good.
Edit, I'm going to make one small change. @bftrick, for future reference, any filter hook with 'pre_' prefixed on the front allows filtering a value before it is stored in the database.
#71
@
11 years ago
user-new.4.diff is ready for commit. Props bftrick.
#74
follow-ups:
↓ 81
↓ 111
@
11 years ago
wp-admin-plugin-install.25229.diff — @naomicbush:
- Add spacing to filters and actions per coding standards while you're documenting them, e.g.
apply_filters( 'hook', 'callback' )
vs apply_filters('hook', 'callback')`. 'install_plugins_pre_' . $tab
, rather than generalizing that install or plugin information are tabs, specify that the action fires before each tab is loaded. Add a long description if you think it will make it clearer.- Other than that, looks good.
wp-admin-includes-plugin-install.25229.diff — @naomicbush:
- "Requested information" is a little too succinct. Maybe be a little more descriptive.
- Space out actions and filters per coding standards
- "Merely add a result" isn't going to work. Each filter or action should be viewed as independent. "Merely add a result" makes sense in the context of the file and other hooks but not independently.
- Remove all functional docs changes. Let's just stick with hooks and filters for now. If you'd like to submit a functional docs patch separately, create a new ticket on the Inline Docs component and we'll discuss those there.
wp-links-opml.diff — @netweb:
opml_head
action: Space it out per coding standards, and drop the?>
to a new line. Also, the short description should describe when it fires, e.g. "Fires in the OPML head." or something- Anytime you find yourself writing "Allows you to filter something", just use "Filter something". "Allows you to" makes an implication about how, instead of what
@param
lines 54, 69 need a period after the short description- Spacing issues with the docblock for
link_title
L#64
#75
follow-up:
↓ 77
@
11 years ago
wp-includes-feed-rdf.diff — @dustyf:
- Space out actions and filters per coding standards while you're there documenting them (it's a great opportunity), e.g.
do_action( 'rdf_ns' )
vsdo_action('rdf_ns')
- Change the tense of "Fired" to "Fires" in you short descriptions
- Nice work on this one! Make these changes and it's ready to go in :)
wp-includes-canonical.diff — @dustyf:
- Space out actions and filters per coding standards while you're there.
- There's no hard-and-fast rule for when to wrap lines, such as in a long description, but generally I try to keep it at about 10-12 words per line. So when you wrap it, the line should start with a
*
and a space and then the rest of the sentence. Like this: https://gist.github.com/DrewAPicture/5e1e395744060e50ba84 - Fix the above and I'd say it's ready to go in
wp-includes-feed-rss.2.diff — @dustyf:
- Change tense of "Fired" to "Fires" in both short descriptions
- That's it for this one, nice work.
wp-includes-feed-rss2.diff — @dustyf:
- s/Fired/Fires in short descriptions
- Space out the actions per coding standards, e.g.
do_action( 'rss2_item' );
vsdo_action('rss2_item');
- This goes for this patch and any future ones: Anytime you find yourself writing "Allows filtering of whatever" just change that to "Filter whatever". It's about documenting what rather than what's possible.
@since
should use the 3-digit, x.x.x style
wp-includes-feed-atom.php — @dustyf:
- Can you regenerate the patch in diff format?
wp-includes-feed-rss2-comments.diff — @dustyf:
- s/Fired/Fires
- Space out actions and filters per coding standards
- Space out the
@param
lines to align the types, variables and short descriptions, like this: https://gist.github.com/DrewAPicture/e8412b856fd7305f8ac6 - Other than that, looks good
#76
follow-up:
↓ 78
@
11 years ago
attachment:post-thumbnail-template.php.diff - @NikV
- All patches should be made from either the WP root directory (svn diff wp-includes/post-thumbnail-template.php > yourfile.diff), or trunk root (svn diff src/wp-includes/post-thumbnail-template.php > yourfile.diff)
- Preferably name your patches the entire WP directory path to the file, and leave off the .php extension in the diff filename (so wp-includes/post-thumbnail-template.php would be wp-includes-post-thumbnail-template.diff)
- Parameters such as $size here should use a @see reference to a function's docblock that describe in more detail the options for $size. For instance, this can accept either a string keyword for a size, or a two-item array representing width and height in pixels. Ideally we reference the a canonical doc so as not to repeat ourselves of the options.
- "Begins to fetch the post thumbnail HTML" and "Stops fetching the post thumbnail HTML." would be better phrased as "Before fetching the post thumbnail HTML." and "After fetching the post thumbnail HTML." Actions are discrete points of execution, so using a verb like "begins" is misleading - the action is not doing anything.
- "The ID of the post where the post thumbnail will be displayed." is confusing. Here let's be brief and say "Post ID." as it does in the function docblock for get_the_post_thumbnail().
- s/iamge/image
- "The ID of a post thumbnail." should be "The post thumbnail ID." We're not talking about a generic post thumbnail, it is a specific one that has been supplied. Ditto for "The size of a post thumbnail. "
- Thanks for the work, look forward to the next draft!
#77
in reply to:
↑ 75
@
11 years ago
Replying to DrewAPicture:
- Space out actions and filters per coding standards while you're there documenting them (it's a great opportunity), e.g.
do_action( 'rdf_ns' )
vsdo_action('rdf_ns')
I actually thought about this after I submitted. I'll be sure to fix it when I resubmit.
wp-includes-feed-atom.php — @dustyf:
- Can you regenerate the patch in diff format?
Wow, I think it was late. Not sure why I submitted this :)
I'll get the changes up before the ticket Triage tomorrow so it can get in.
@
11 years ago
Better patch for wp-admin/includes/plugin.php. Adds @see references where parameters come from calling functions and removes inadvertent/inaccurate parameter docs from the uninstall_$file action.
#78
in reply to:
↑ 76
;
follow-up:
↓ 82
@
11 years ago
Replying to ericlewis:
attachment:post-thumbnail-template.php.diff - @NikV
- All patches should be made from either the WP root directory (svn diff wp-includes/post-thumbnail-template.php > yourfile.diff), or trunk root (svn diff src/wp-includes/post-thumbnail-template.php > yourfile.diff)
- Preferably name your patches the entire WP directory path to the file, and leave off the .php extension in the diff filename (so wp-includes/post-thumbnail-template.php would be wp-includes-post-thumbnail-template.diff)
- Parameters such as $size here should use a @see reference to a function's docblock that describe in more detail the options for $size. For instance, this can accept either a string keyword for a size, or a two-item array representing width and height in pixels. Ideally we reference the a canonical doc so as not to repeat ourselves of the options.
- "Begins to fetch the post thumbnail HTML" and "Stops fetching the post thumbnail HTML." would be better phrased as "Before fetching the post thumbnail HTML." and "After fetching the post thumbnail HTML." Actions are discrete points of execution, so using a verb like "begins" is misleading - the action is not doing anything.
- "The ID of the post where the post thumbnail will be displayed." is confusing. Here let's be brief and say "Post ID." as it does in the function docblock for get_the_post_thumbnail().
- s/iamge/image
- "The ID of a post thumbnail." should be "The post thumbnail ID." We're not talking about a generic post thumbnail, it is a specific one that has been supplied. Ditto for "The size of a post thumbnail. "
- Thanks for the work, look forward to the next draft!
How would I format the @see? Would I do it only for the $size?
#81
in reply to:
↑ 74
@
11 years ago
Replying to DrewAPicture:
wp-links-opml.diff — @netweb:
opml_head
action: Space it out per coding standards, and drop the?>
to a new line. Also, the short description should describe when it fires, e.g. "Fires in the OPML head." or something- Anytime you find yourself writing "Allows you to filter something", just use "Filter something". "Allows you to" makes an implication about how, instead of what
@param
lines 54, 69 need a period after the short description- Spacing issues with the docblock for
link_title
L#64
Thanks for the feedback :) Fixed in http://core.trac.wordpress.org/attachment/ticket/25229/wp-links-opml.2.diff
#82
in reply to:
↑ 78
@
11 years ago
Replying to NikV:
Replying to ericlewis:
attachment:post-thumbnail-template.php.diff - @NikV
- All patches should be made from either the WP root directory (svn diff wp-includes/post-thumbnail-template.php > yourfile.diff), or trunk root (svn diff src/wp-includes/post-thumbnail-template.php > yourfile.diff)
- Preferably name your patches the entire WP directory path to the file, and leave off the .php extension in the diff filename (so wp-includes/post-thumbnail-template.php would be wp-includes-post-thumbnail-template.diff)
- Parameters such as $size here should use a @see reference to a function's docblock that describe in more detail the options for $size. For instance, this can accept either a string keyword for a size, or a two-item array representing width and height in pixels. Ideally we reference the a canonical doc so as not to repeat ourselves of the options.
- "Begins to fetch the post thumbnail HTML" and "Stops fetching the post thumbnail HTML." would be better phrased as "Before fetching the post thumbnail HTML." and "After fetching the post thumbnail HTML." Actions are discrete points of execution, so using a verb like "begins" is misleading - the action is not doing anything.
- "The ID of the post where the post thumbnail will be displayed." is confusing. Here let's be brief and say "Post ID." as it does in the function docblock for get_the_post_thumbnail().
- s/iamge/image
- "The ID of a post thumbnail." should be "The post thumbnail ID." We're not talking about a generic post thumbnail, it is a specific one that has been supplied. Ditto for "The size of a post thumbnail. "
- Thanks for the work, look forward to the next draft!
How would I format the @see? Would I do it only for the $size?
Never mind. Patch will be submitted soon.
#83
follow-up:
↓ 87
@
11 years ago
wp-admin-includes-plugin.2.diff — @dllh:
wp-includes-class-wp-admin-bar.php.patch — @betzster:
It's totally fine to use @since Unknown
, but if you're specifying an actual version, please make sure it's correct.
admin_bar_init
andadd_admin_bar_menus
actions were both introduced in 3.1 along with the admin bar itself (see [15862] and [16038]), not in 3.0.2 or 3.0.4.activate_*
anddeactivate_*
were added in 2.0 (see [2697]), not in 3.0.2.activate_plugin
,activated_plugin
and their "deactivate" counterparts were added in 2.9 (see [11909]), not in 3.0.2 or 3.3.2.uninstall_*
was added in 2.7 (see [8585]), not in 2.6.1.
Minor releases generally don't contain new hooks.
#86
@
11 years ago
It's also a good idea to wait for DrewAPicture's confirmation before committing any of the patches here.
#87
in reply to:
↑ 83
@
11 years ago
Replying to SergeyBiryukov:
wp-admin-includes-plugin.2.diff — @dllh:
It's totally fine to use@since Unknown
, but if you're specifying an actual version, please make sure it's correct.
Yikes, I saw green in the blame and thought I'd found the addition, but I'll bet that was a move and not an add. Will do better next time or specify Unknown.
#88
@
11 years ago
Ugh, forget wp-admin_plugin-install.php.patch. Duplicate.
#89
@
11 years ago
@netweb – wp-links-opml.2.diff:
- If you can indent lines 33-38 (the code inside the opening <?php and closing ?> tags) I think this is all set!
@a.hoereth – plugin-editor.php.patch:
- Short description could instead be written as "Filter the file type extensions that are editable in the plugin editor."
- Long description becomes unnecessary with the above edit
- @since tags should use the three-digit x.x.x style, 2.8.0 in this case.
@
11 years ago
Second pass at wp-includes/feed-rdf.php - Added spaces throughout the file to meet coding standards, changed tense of short comment.
#90
follow-up:
↓ 91
@
11 years ago
@dustyf – wp-includes-feed-rdf.2.diff:
- You went above and beyond for the spacing alterations! They all look good to me, and I think this is ready to go in.
- For your other patches, though, focus your edits around the hooks themselves (rather than everything) so the patch is easier to grok and has smaller margin for errors.
#91
in reply to:
↑ 90
@
11 years ago
Replying to rzen:
@dustyf – wp-includes-feed-rdf.2.diff:
- You went above and beyond for the spacing alterations! They all look good to me, and I think this is ready to go in.
- For your other patches, though, focus your edits around the hooks themselves (rather than everything) so the patch is easier to grok and has smaller margin for errors.
We should only be spacing out the actual action or filter lines we're documenting. Any other spacing changes will unnecessarily update the blame records for those lines.
#92
@
11 years ago
wp-includes-feed-rdf.3.diff removes the extra whitespacing and is ready for commit, props @dustyf.
#94
@
11 years ago
wp-links-opml.3.diff is ready for commit, props @netweb.
#95
follow-up:
↓ 98
@
11 years ago
wp-admin-includes-plugin.3.diff is ready for commit.
It has some fixes for wp-admin/includes/plugin.php following [25474] and [25477].
@
11 years ago
Second Pass at wp-includes/canonical.php - Also fixes spacing on functions, hooks, etc and after ! in entire document
#97
@
11 years ago
wp-admin-includes-ms.2.diff is ready for commit. Props @enej and @DrewAPicture.
#98
in reply to:
↑ 95
;
follow-up:
↓ 99
@
11 years ago
Replying to DrewAPicture:
wp-admin-includes-plugin.3.diff is ready for commit.
It has some fixes for wp-admin/includes/plugin.php following [25474] and [25477].
$network_deactivating
parameter for deactivate_plugin
, deactivate_*
, and deactivated_plugin
hooks references a non-existing function (@see deactivate_plugin()
). Should we correct that as well?
#99
in reply to:
↑ 98
;
follow-up:
↓ 103
@
11 years ago
Replying to SergeyBiryukov:
Replying to DrewAPicture:
wp-admin-includes-plugin.3.diff is ready for commit.
It has some fixes for wp-admin/includes/plugin.php following [25474] and [25477].
$network_deactivating
parameter fordeactivate_plugin
,deactivate_*
, anddeactivated_plugin
hooks references a non-existing function (@see deactivate_plugin()
). Should we correct that as well?
Yeah, that's probably a good idea :) Sorry, I missed that.
#100
@
11 years ago
@dustyf – wp-includes-canonical.2.diff:
- Per @DrewAPicture and my comments on wp-includes-feed-rdf.2.diff, can you resubmit a patch with *only* the hook-related edits? All other edits are out-of-scope for this ticket and its patches.
#102
follow-up:
↓ 107
@
11 years ago
Hooks that come from WordPress MU should have @since MU
rather than 3.0.0. I've corrected them in [25481].
#103
in reply to:
↑ 99
;
follow-up:
↓ 104
@
11 years ago
Replying to DrewAPicture:
Yeah, that's probably a good idea :) Sorry, I missed that.
How does wp-admin-includes-plugin.4.diff look?
#104
in reply to:
↑ 103
@
11 years ago
Replying to SergeyBiryukov:
Replying to DrewAPicture:
Yeah, that's probably a good idea :) Sorry, I missed that.
How does wp-admin-includes-plugin.4.diff look?
Sure, that works.
#106
@
11 years ago
Here are the results of our triage today, which wasn't much of a triage becasue DrewAPicture stayed pretty heavily on top of everything this week. Props, Drew!
26 Patches Committed:
Patch | File | Props | Changeset |
---|---|---|---|
nav-menu-template.2.diff | wp-includes/nav-menu-template.php | Faison | [25410] (helen) |
options-writing.patch | wp-admin/options-writing.php | siobhyb | [25411] (helen) |
shortcodes.diff | wp-includes/shortcodes.php | natejacobs | [25423] (SergeyBiryukov) |
ms-delete-site.php1.diff | wp-admin/ms-delete-site.php | NikV | [25424] (SergeyBiryukov) |
author-template.4.patch | wp-includes/author-template.php | Frank Klein | [25429] (SergeyBiryukov) |
comment-php.2.patch | wp-admin/includes/comment.php | mordauk | [25434] (SergeyBiryukov) |
export-php.2.patch | wp-admin/export.php | mordauk | [25435] (SergeyBiryukov) |
wp-includes-load.php.2.diff | wp-includes/load.php | mordauk | [25455] (SergeyBiryukov) |
user-new.4.diff | wp-admin/user-new.php | bftrick | [25470] (SergeyBiryukov) |
wp-admin-includes-plugin.2.diff | wp-admin/includes/plugin.php | dllh | [25474] (westi), [25477] (SergeyBiryukov) |
wp-includes-class-wp-admin-bar.php.patch | wp-includes/class-wp-admin-bar.php | betzster | [25475] (westi), [25478] (SergeyBiryukov) |
wp-includes-feed-rdf.3.diff | wp-includes-feed-rdf.php | dustyf | [25479] (SergeyBiryukov) |
wp-links-opml.3.diff | wp-links-opml.php | netweb | [25480] (SergeyBiryukov) |
wp-admin-includes-ms.2.diff | wp-admin/includes/ms.php | enej, DrewAPicture | [25481] (SergeyBiryukov) |
wp-admin-includes-plugin.4.diff | wp-admin/includes/plugin.php | DrewAPicture, SergeyBiryukov | [25482] (SergeyBiryukov) |
wp-admin-includes-plugin-install.25229.2.diff | wp-admin/includes/plugin-install.php | naomicbush | [25511] (DrewAPicture) |
wp-admin-plugin-install.25229.2.diff | wp-admin/plugin-install.php | naomicbush | [25512] (DrewAPicture) |
post-thumbnail-template.php.diff | wp-includes/post-thumbnail-template.php | NikV | [25513] (DrewAPicture) |
wp-includes-feed-rss.2.diff | wp-includes/feed-rss.php | dustyf | [25529] (DrewAPicture) |
wp-includes-feed-rss2.diff | wp-includes/feed-rss2.php | dustyf | [25530] (DrewAPicture) |
wp-includes-feed-atom.php] | wp-includes/feed-atom.php | dustyf | [25531] (DrewAPicture) |
wp-includes-feed-rss2-comments.diff | wp-includes/feed-rss2-comments.php | dustyf | [25532] (DrewAPicture) |
plugin-editor.php.patch | wp-admin/plugin-editor.php | a.hoereth | [25534] (DrewAPicture) |
none | wp-includes/canonical.php | dustyf | [25535] (DrewAPicture) |
wp-activate.diff | wp-activate.php | nullvariable | [25537] (DrewAPicture) |
admin-ajax.diff | wp-admin/admin-ajax.php | nullvariable | [25538] (DrewAPicture) |
Patches Awaiting Edits:
None''
Patches Awaiting Review:
None!
Patches Ready for Commit:
None
Workflow Notes
During the inline-docs meeting today we discussed that, going forward, all new files will be handled via their own tickets, rather than tracked in this single massive ticket. I'll be updating the make/core post to reflect the new workflow steps, so please look there for further instruction.
Those of you with patches attached to this ticket (naomicbush, dustyf, NikV, a.hoereth) will continue to submit those patches here. All new files will become their own tickets and be patched there.
Thanks everyone for your contributions so far!
#107
in reply to:
↑ 102
@
11 years ago
Replying to SergeyBiryukov:
Hooks that come from WordPress MU should have
@since MU
rather than 3.0.0. I've corrected them in [25481].
wp-admin-includes-ms.3.diff
improves on [25481] by making it clear whether the hooks fire before of after the related event occurs.
#109
@
11 years ago
Some more fixes in wp-admin-includes-ms.4.diff ("User ID of the user" seemed a bit redundant).
#111
in reply to:
↑ 74
@
11 years ago
Replying to DrewAPicture:
wp-admin-plugin-install.25229.diff — @naomicbush:
- Add spacing to filters and actions per coding standards while you're documenting them, e.g.
apply_filters( 'hook', 'callback' )
vs apply_filters('hook', 'callback')`.'install_plugins_pre_' . $tab
, rather than generalizing that install or plugin information are tabs, specify that the action fires before each tab is loaded. Add a long description if you think it will make it clearer.- Other than that, looks good.
wp-admin-includes-plugin-install.25229.diff — @naomicbush:
- "Requested information" is a little too succinct. Maybe be a little more descriptive.
- Space out actions and filters per coding standards
- "Merely add a result" isn't going to work. Each filter or action should be viewed as independent. "Merely add a result" makes sense in the context of the file and other hooks but not independently.
- Remove all functional docs changes. Let's just stick with hooks and filters for now. If you'd like to submit a functional docs patch separately, create a new ticket on the Inline Docs component and we'll discuss those there.
Thanks for the feedback, DrewAPicture. Patches revised.
#113
in reply to:
↑ 112
@
11 years ago
Replying to onalioa:
Please ask your question on the WordPress Support Forums. Trac is for core development.
#123
@
11 years ago
- Resolution set to fixed
- Status changed from new to closed
I think we've wrapped up all outstanding patches here.
See comment:38 and comment:106 for the lists of committed patches and contributors.
Please create new tickets for new files per the new workflow outlined on the make/core post.
#124
follow-up:
↓ 128
@
11 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Looks like a couple of patches were missed. Both were reviewed in comment:38:
- wp-activate.diff — @nullvariable
- admin-ajax.diff — @nullvariable
#126
@
11 years ago
- Owner set to DrewAPicture
- Resolution set to fixed
- Status changed from reopened to closed
In 25538:
#128
in reply to:
↑ 124
;
follow-up:
↓ 129
@
11 years ago
Replying to DrewAPicture:
Looks like a couple of patches were missed. Both were reviewed in comment:38:
- wp-activate.diff — @nullvariable
- admin-ajax.diff — @nullvariable
eh my bad! Just saw that.
#129
in reply to:
↑ 128
@
11 years ago
Replying to nullvariable:
Replying to DrewAPicture:
Looks like a couple of patches were missed. Both were reviewed in comment:38:
- wp-activate.diff — @nullvariable
- admin-ajax.diff — @nullvariable
eh my bad! Just saw that.
No sweat, we got them in. Thanks for the patches, hope to see more from you in separate tickets :)
Second pass at ./wp-comments-post.dff