Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#25475 closed defect (bug) (fixed)

Hook Docs: wp-includes/cron.php

Reported by: tmtoy's profile tmtoy Owned by: drewapicture's profile DrewAPicture
Milestone: 3.8 Priority: normal
Severity: normal Version:
Component: Inline Docs Keywords: has-patch commit
Focuses: Cc:

Description

Attached patch has done for the all hooks in wp-includes/cron.php.

Attachments (3)

cron.diff (4.1 KB) - added by tmtoy 10 years ago.
cron-2.diff (5.6 KB) - added by tmtoy 10 years ago.
Fixed mistake /wp-includes/cron.php
25475.diff (2.9 KB) - added by DrewAPicture 10 years ago.

Download all attachments as: .zip

Change History (13)

@tmtoy
10 years ago

#1 @DrewAPicture
10 years ago

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

#2 @DrewAPicture
10 years ago

  • Milestone changed from Awaiting Review to 3.7
  • Owner changed from rzen to DrewAPicture
  • Type changed from enhancement to defect (bug)

#3 @DrewAPicture
10 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 3.7 to Awaiting Review

Thanks for the patch. Some notes on cron.diff:
Overall:

  • For filters, the suggested language for the short description is "Filter <this thing>", so for all of the filter docs in your patch, keep that in mind. Some examples: "Filter a single scheduled event", "Filter whether to SSL verify the cron request", etc.
  • For array hash notations, the opening line should use always use an @param tag. Parameters should use @type and be indented four spaces. See the inline docs template for the style we're using.
  • There should be a space between the array variable and the opening brace for hash notations. @param array $variable{ becomes @param array $variable {

schedule_event filter:

  • $event is an array cast to an object, so referring to it as an array in the hash notation is incorrect.
  • Another option would be to change the value of $event above the docs to just an array, and then cast the value of the filter as an object. The hash notation also needs a short description (indented four spaces followed by a blank line.)

https_local_ssl_verify filter:

  • Try to avoid jargon, in this case, "SecureSocketsLayer".
  • There's no need to assign the boolean value to a variable first, just reference the value directly in the parameter doc.

cron_request filter:

  • Break the array out of the filter line and assign it to a variable above the doc. This allows you to reference the variable containing the array in the doc.
  • Needs a short description for the hash notation (same as schedule_event).
  • Space between array{
  • Specify the default for boolean values at the end of the parameter descriptions

cron_schedules filter:

  • The hash notation opening line should use @param
  • Fix hash notation spacing
  • Use variable-style names instead of array keys. 'hourly' becomes $hourly, 'interval' becomes $interval, etc.

I'm happy to clarify anything that doesn't make sense. Since the hash notation style is very new, we're trying to be very particular about how everything comes together. Looking forward to your next patch!

#4 follow-up: @DrewAPicture
10 years ago

A new patch is still needed here with the adjustments requested in comment:3

#5 in reply to: ↑ 4 @tmtoy
10 years ago

Thanks for a polite reply!!

Sorry for late...
I challenged the new patch. It may be wrong also maybe.
But, I will continue to challenge as many times.


overall:

  • Modified to the short description that "Filter <this thing>".
  • Modified to the @param tag the opening line of array.
  • Modified to parameters use @type and be indented four spaces.
  • Added to space between the array variable and the opening brace for hash notations.

schedule_event filter:

  • Added to "clone" for array object.
  • Added to Short Description.

https_local_ssl_verify filter:

  • Deleted to assign the boolean value.
  • Added to Specify the default.

cron_request filter:

  • Broke the array out of the filter line and assigned it to a variable above.
  • Added to Short Description.
  • Modified to Space between array{array {
  • Added to Specify the default.

cron_schedules filter:

  • Modified to the @param tag the opening line of array.
  • Fixed hash notation spacing.
  • Modified to variable-style names instead of array keys.

@tmtoy
10 years ago

Fixed mistake /wp-includes/cron.php

@DrewAPicture
10 years ago

#6 @DrewAPicture
10 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 3.8

25475.diff should do it. In cron-2.diff there was a lot of unnecessarily changed code, incorrect spacing, etc.

One code change in the current patch is to use add_query_arg to simplify building the cron URL in the cron_request filter.

#7 @kpdesign
10 years ago

  • Keywords commit added

25475.diff looks good. Recommend commit.

#8 @DrewAPicture
10 years ago

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

In 26267:

Inline documentation for hooks in wp-includes/cron.php.

Props tmtoy for the initial patch.
Fixes #25475.

#9 @ocean90
10 years ago

[26267] has a typo see #26218.

#10 @nacin
10 years ago

add_query_arg() is a utility function but doesn't always need to be used. To eat the CPU cycles it brings, there should be some benefit to sanity (like escaping) and readability, but that wasn't the case here and I don't think it needed to change.

Regardless, let's avoid these kinds of functional changes during inline documentation commits. Not a *huge* deal as things can always be reverted, but it makes it much harder to review the commits. Thanks!

Note: See TracTickets for help on using tickets.