Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#37059 closed enhancement (fixed)

Allow post to be passed to get_the_modified_time & get_the_modified_date

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: good-first-bug has-patch has-unit-tests
Focuses: Cc:

Description (last modified by peterwilsoncc)

The signatures of get_the_time and get_the_modified_time differ. Let's make them match

The current signatures are:

get_the_time ( string $d = '', int|WP_Post $post = null )

get_the_modified_time ( string $d = '' )

get_the_date ( string $d = '', int|WP_Post $post = null )

get_the_modified_date ( string $d = '' )

~

Note: added the date functions per @Soean's comment below.

Attachments (9)

37059.patch (1.9 KB) - added by Soean 8 years ago.
37059.2.patch (2.2 KB) - added by Soean 8 years ago.
37059.diff (2.0 KB) - added by lukecavanagh 8 years ago.
Patch and unit test
37059.3.diff (3.4 KB) - added by lukecavanagh 8 years ago.
Patch
37059.4.diff (5.3 KB) - added by Soean 8 years ago.
Refresh patch after review
37059.5.diff (5.2 KB) - added by Soean 8 years ago.
Refresh patch after @DrewAPicture review
37059.3.patch (706 bytes) - added by sebastian.pisula 8 years ago.
37059.2.diff (1.3 KB) - added by peterwilsoncc 8 years ago.
37059.6.diff (4.1 KB) - added by peterwilsoncc 8 years ago.

Download all attachments as: .zip

Change History (30)

#1 @boonebgorges
8 years ago

  • Component changed from General to Posts, Post Types
  • Description modified (diff)
  • Keywords good-first-bug needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

#2 @boonebgorges
8 years ago

  • Description modified (diff)

#3 @lukecavanagh
8 years ago

Let me work on a patch.

@Soean
8 years ago

@Soean
8 years ago

#4 @Soean
8 years ago

  • Keywords has-patch added; needs-patch removed

Same difference in the date functions.

get_the_date( $d = '', $post = null )  
get_the_modified_date($d = '') 
Last edited 8 years ago by Soean (previous) (diff)

@lukecavanagh
8 years ago

Patch and unit test

@lukecavanagh
8 years ago

Patch

#5 @lukecavanagh
8 years ago

The patch added in the documentation that was added, as well as changing the null value to be false.

Last edited 8 years ago by lukecavanagh (previous) (diff)

#6 @boonebgorges
8 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Owner set to peterwilsoncc
  • Status changed from new to reviewing

You have your patch, sir :)

#7 @peterwilsoncc
8 years ago

  • Description modified (diff)
  • Summary changed from Allow post to be passed to get_the_modified_time to Allow post to be passed to get_the_modified_time & get_the_modified_date

#8 @peterwilsoncc
8 years ago

  • Keywords needs-refresh added

Thanks for the patches @Soean and @lukecavanagh.

Just a few notes to get the patch commit ready. I'm basing them off the latest patch, 37059.3.diff

This looks like a lot but it's all really minor (and I'm verbose).

In the source code:

  • In the docs:
    • add a second @since tag noting the new argument (similar to the language_attributes function in the same file).
    • align the @param variables and descriptions.
  • Add the get_post & check for a valid post similar to the check in get_the_time
  • The filters in the functions we're aiming to match also accept the post as an argument, let's match them too (noting the update in the docs).
  • There's a couple of coding standards (whitespace) fixes required in get_the_modified_date's first call to get_post_modified_time
  • [edit] all the code inside the if...else statements is changing, so it's a good opportunity to add braces to meet the coding standards.

In the tests:

  • Coding standards (whitespace) the closing brace of the first new function onwards are incorrectly indented.
  • Add some documentation for each function, including:
    • explanation
    • since tags
    • this ticket number
  • ...with_post_id is clearer for people scanning the code.

As I say, all quite minor.

Pete

Last edited 8 years ago by peterwilsoncc (previous) (diff)

@Soean
8 years ago

Refresh patch after review

#9 @Soean
8 years ago

Thanks @peterwilsoncc for your great review.

This is my first patch and your feedback helps me a lot.
So I hope I get all your points in 37059.4.diff .

#10 @peterwilsoncc
8 years ago

  • Keywords needs-refresh removed

#11 @DrewAPicture
8 years ago

  • Milestone changed from Future Release to 4.6

@Soean: Thanks for the update. Here are some further notes on 37059.4.diff:

  • Let's add backticks around $post in the changelog entries for both functions. The should also be structured as sentences, albeit short:
    * @since 4.6.0 The `$post` parameter was added.
    
  • We also need changelog entries for the new parameters added to the two filters
  • Also, at the point where $post is passed to the filters, it's only a WP_Post object
Last edited 8 years ago by DrewAPicture (previous) (diff)

@Soean
8 years ago

Refresh patch after @DrewAPicture review

#12 @Soean
8 years ago

Thanks @DrewAPicture for your notes, I updated the patch and uploaded 37059.5.diff

#13 @DrewAPicture
8 years ago

  • Keywords commit added

@peterwilsoncc: All yours :-)

#14 @peterwilsoncc
8 years ago

Thanks for the refreshes @Soean, I'll commit this Friday Australian time and props you and @lukecavanagh.

This is my first patch and your feedback helps me a lot.

Looking forward to many more, I'm glad Drew and I could help :)

#15 @peterwilsoncc
8 years ago

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

In 37738:

Posts: Add $post parameter to modified date and time functions.

Unifies the APIs for getting a post's modified date or time with getting a post's date or time.

Adds the $post parameter to the functions get_the_modified_date and get_the_modified_time.

Adds the $post parameter to the filters get_the_modified_date and get_the_modified_time.

Props Soean, lukecavanagh.
Fixes #37059.

#16 follow-up: @sebastian.pisula
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I think that in get_the_modified_date() this code

$post = get_post( $post );

	if ( ! $post ) {
		return false;
	}

is unnecessary because in get_post_modified_time check valid $variable and function get_post_modified_time() is called always

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


8 years ago

#18 @peterwilsoncc
8 years ago

  • Keywords commit removed

Post commit [37738] @azaozz and I had a brief chat in Slack re backcompat.

Previously, if called before the global $post object was set, the value false would be run through the filter. Now, if the post is called before the global $post object is set, the function returns false and bypasses the filter.

37059.2.diff restores the old behavior of passing a failure to get the post though the filter.

I'm genuinely undecided on how much it matters, the old behavior could only have occurred before the posts_selection action.

#19 @peterwilsoncc
8 years ago

In 37059.6.diff:

  • maintains back-compat by passing failures to get a post through filters for both get_the_modified_date and get_the_modified_time
  • adds unit test to ensure failures are filtered
  • added tests for get_modified_date to match those for get_modified_time.

#20 in reply to: ↑ 16 @peterwilsoncc
8 years ago

Replying to sebastian.pisula:

I think that in get_the_modified_date() this code

$post = get_post( $post );

	if ( ! $post ) {
		return false;
	}

is unnecessary because in get_post_modified_time check valid $variable and function get_post_modified_time() is called always

I'm going to keep the ! $post check in both get_the_modified_date and get_the_modified_time. You're correct, get_post_modified_time does the same check but when the outcome of a function call is easily premepted it can be bypassed.

#21 @peterwilsoncc
8 years ago

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

In 37866:

Posts: Fix back-compat for filters in get the modified time and date functions after [37738].

When no valid post exists for get_the_modified_time or get_the_modified_date calls, the result (false) is passed through the functions respective filters to maintain back-compat.

Introduces unit tests to ensure filters are applied and for the get_the_modified_date function.

Fixes #37059.

Note: See TracTickets for help on using tickets.