Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#51184 closed defect (bug) (fixed)

get_the_date() checks $format only for empty variable and fails on false boolean - since WP 5.5

Reported by: wittich's profile wittich Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.5.1 Priority: normal
Severity: normal Version: 5.5
Component: Date/Time Keywords: has-patch has-unit-tests commit dev-reviewed fixed-major
Focuses: template, coding-standards Cc:

Description

Just run over a bug which was generated in WordPress 5.5, before it was working fine.

The function get_the_date() in the general-template.php is now comparing only for empty string and ignores a $format = false;

Compare https://github.com/WordPress/WordPress/blob/5.4-branch/wp-includes/general-template.php#L2531 vs. https://github.com/WordPress/WordPress/blob/5.5-branch/wp-includes/general-template.php#L2528

I think the correct solution would be something like that:

if ( '' === $format || false === $format )

Here is how to test the case

echo get_the_date( get_option( 'date_time' ) );
// Output: ""

This should output the date and not an empty result. The options 'date_time' doesn't exist.

Attachments (5)

51184.patch (3.5 KB) - added by akabarikalpesh 4 years ago.
I added patch for general-template.php and comment-template.php
51184-unit-tests.diff (2.5 KB) - added by akabarikalpesh 4 years ago.
patch for unit test.
51184_all.diff (5.3 KB) - added by wittich 4 years ago.
correct merged patch for src/ and tests/
51184.2.diff (3.3 KB) - added by SergeyBiryukov 4 years ago.
51166.3.diff (4.2 KB) - added by wittich 4 years ago.
Add test for get_the_modified_date and get_the_modified_time

Download all attachments as: .zip

Change History (40)

#1 @SergeyBiryukov
4 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.5.1
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted
  • Summary changed from get_the_date() checks $format only for emtpy variable and fails on false boolean - since WP 5.5 to get_the_date() checks $format only for empty variable and fails on false boolean - since WP 5.5

Hi there, welcome to WordPress Trac! Thanks for the report.

Introduced in [47808]. Some other functions are affected too:

  • get_the_date()
  • get_the_time()
  • get_comment_date()
  • get_comment_time()

This ticket was mentioned in PR #505 on WordPress/wordpress-develop by wittich.


4 years ago
#2

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

…iable and fails on false boolean - since WP 5.5

Introduced in [47808]. Some other functions are affected too:

get_the_date()
get_the_time()
get_comment_date()
get_comment_time()

Fixes #51184

Trac ticket:

#3 @wittich
4 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests removed
  • Resolution set to invalid
  • Status changed from accepted to closed

@SergeyBiryukov just tried my first patch... took me a while, pls have a look!

#4 follow-up: @Rarst
4 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

I don't think we should indicate or check for boolean there, because it's not meant to accept that at all. It's just _one_ way _how_ it can fail.

Technically empty() check would be better _equivalent_ for old code, but it's meh when input can be non-empty but also not string that we want (it's WordPress! inputs are garbage).

I would flip the statements and check for valid case first if( is_string( $format ) && '' !== $format ) and anything else would fall back to default format.

#5 in reply to: ↑ 4 @wittich
4 years ago

Replying to Rarst:

I would flip the statements and check for valid case first if( is_string( $format ) && '' !== $format ) and anything else would fall back to default format.

Good idea, should I try it and send another pull request? No clue how you guys handle it normally...

#6 @Rarst
4 years ago

Whatever most convenient for you. :) You can work in same PR, it will just get converted to a patch for SVN by a core committer in the end.

#7 @wittich
4 years ago

I updated my pull request https://github.com/WordPress/wordpress-develop/pull/507 don't know if that was to correct way. I added commit 217ca22.

I found now the logic better to read:

if ( is_string( $format ) && '' !== $format ) {
    // use $format
} else {
    // use default
}
Last edited 4 years ago by wittich (previous) (diff)

#8 @Rarst
4 years ago

Yeah, that was my suggestion, looks good. :) Please drop phpdoc changes, since we don't actually want or expect false to be passed there.

@akabarikalpesh
4 years ago

I added patch for general-template.php and comment-template.php

#9 @akabarikalpesh
4 years ago

  • Focuses template coding-standards added
  • Keywords has-patch needs-testing added; needs-patch removed

#10 @Rarst
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

For suggested condition the statements need to be reversed (since check is now for validity, not for invalidity). Pull request above already does that and has unit tests (I hadn't run them though, just looked at the code).

@akabarikalpesh
4 years ago

patch for unit test.

#11 @akabarikalpesh
4 years ago

  • Keywords needs-unit-tests added; has-unit-tests removed

#12 @akabarikalpesh
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#13 @wittich
4 years ago

I updated my pull request https://github.com/WordPress/wordpress-develop/pull/507

Don't know what @akabarikalpesh did different?!

@Rarst The test runs through with docker and npm run test, see here for the results of the test: https://github.com/WordPress/wordpress-develop/runs/1046094504

Hope that's it for #51184 was a fun experience to contribute.


PS: @akabarikalpesh your patch is wrong , you didn't switch the logic. Pls compare with my pull request. With your patch the test should fail, pls try to run the test...

Last edited 4 years ago by wittich (previous) (diff)

@wittich
4 years ago

correct merged patch for src/ and tests/

This ticket was mentioned in PR #507 on WordPress/wordpress-develop by wittich.


4 years ago
#14

Bugfix: changed from get_the_date() checks $format only for emtpy variable and fails on false boolean - since WP 5.5

  • flip the statements and check for valid case first
  • droped phpdoc changes
  • run php-codesniffer


Introduced in [47808]. Some other functions are affected too:


get_the_date()
get_the_time()
get_comment_date()
get_comment_time()


Fixes #51184

#15 follow-up: @Rarst
4 years ago

Aside on unit tests, I lost track if these should go into date folder or just tagged @group date, @group datetime if they are elsewhere (I thought groups were used on individual test too before, but only see it on classes?). It's not super consistent since Date/Time code is usually mixed into something else.

Last edited 4 years ago by Rarst (previous) (diff)

#16 in reply to: ↑ 15 @wittich
4 years ago

Replying to Rarst:

Aside on unit tests, I lost track if these should go into date folder or just tagged @group date, @group datetime if they are elsewhere (I thought groups were used on individual test too before, but only see it on classes?). It's not super consistent since Date/Time code is usually mixed into something else.

A quick grep -rnw ./tests/ -e "group date\|group datetime" shows that this groups a mainly used in ./tests/date/ so I guess it's not meant to be used in this case here.

@SergeyBiryukov anything missing including the patch?

#17 @SergeyBiryukov
4 years ago

In 48911:

Tests: Bring some consistency to Date/Time tests:

  • Move some tests from post.php to a more appropriate location in the date directory.
  • Rename date/postTime.php to date/getPostTime.php to match the function name.

Props Rarst.
See #51184.

#18 @SergeyBiryukov
4 years ago

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

In 48912:

Date/Time: Make sure get_the_date() and related functions return correct time if the format was specified as false.

Technically, the $format argument should always be a string, but passing false used to work before [47808], so this restores backward compatibility.

The list of affected functions:

  • get_the_date()
  • get_the_time()
  • get_comment_date()
  • get_comment_time()

Props wittich, Rarst, akabarikalpesh, SergeyBiryukov.
Fixes #51184.

#19 @SergeyBiryukov
4 years ago

  • Keywords commit dev-feedback fixed-major added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to the 5.5 branch after a second committer's review.

#20 follow-up: @Rarst
4 years ago

@SergeyBiryukov note that your implementation changes return filter behavior (PR as worked out doesn't). Now filters would receive a default format instead of original format if it's empty. Feels close to BC break for me.

#21 in reply to: ↑ 20 @SergeyBiryukov
4 years ago

Replying to Rarst:

note that your implementation changes return filter behavior (PR as worked out doesn't). Now filters would receive a default format instead of original format if it's empty. Feels close to BC break for me.

Ah, good point.

I went back and forth for a bit, as the parameter is described as string in the filter docs, so passing false there didn't seem like a good idea. But in trying to fix that, I missed that now an empty string is not passed either.

I think the current code is more readable though, let's just rename the sanitized format variable to something like $_format, so that the original value is left untouched. That would be consistent with some other areas of core.

#22 @SergeyBiryukov
4 years ago

I also missed that get_the_modified_date()/get_the_modified_time() already have an empty( $format ) check for the same use case, so it seems better to stick with that in the other functions too for consistency, and as a closer equivalent of the older code.

See 51184.2.diff.

#23 @Rarst
4 years ago

I have discussed empty() as a possibility above, but felt better about being explicit with a string check. This part probably isn't critical in practice, up to you if there is prior logic to follow. :)

On a syntax side if condition is down to single expression, might read better as ternary.

#24 @SergeyBiryukov
4 years ago

In 48918:

Date/Time: In get_the_date() and related functions, pass the original, unmodified $format value to the filters.

Additionally, simplify the $format argument checks for consistency with similar checks in get_the_modified_date() and get_the_modified_time().

Follow-up to [48912].

Props Rarst.
See #51184.

#25 @SergeyBiryukov
4 years ago

Just noting there are now three commits to backport:

  • [48911] includes some prerequisite test updates.
  • [48912] is the main change of this ticket.
  • [48918] is a follow-up change.
Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#26 @desrosj
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

The changes look good to backport.

#27 @desrosj
4 years ago

In 48920:

Tests: Bring some consistency to Date/Time tests:

  • Move some tests from post.php to a more appropriate location in the date directory.
  • Rename date/postTime.php to date/getPostTime.php to match the function name.

Props Rarst.
Merges [48911] to the 5.5 branch.
See #51184.

#28 @desrosj
4 years ago

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

In 48921:

Date/Time: Make sure get_the_date() and related functions return correct time if the format was specified as false.

Technically, the $format argument should always be a string, but passing false used to work before [47808], so this restores backward compatibility.

The list of affected functions:

  • get_the_date()
  • get_the_time()
  • get_comment_date()
  • get_comment_time()

Props wittich, Rarst, akabarikalpesh, SergeyBiryukov.
Merges [48912] to the 5.5 branch.
Fixes #51184.

#29 @desrosj
4 years ago

In 48922:

Date/Time: In get_the_date() and related functions, pass the original, unmodified $format value to the filters.

Additionally, simplify the $format argument checks for consistency with similar checks in get_the_modified_date() and get_the_modified_time().

Follow-up to [48912].

Props Rarst.
Merges [48918] to the 5.5 branch.
See #51184.

@wittich
4 years ago

Add test for get_the_modified_date and get_the_modified_time

#30 follow-up: @wittich
4 years ago

Okay looks much better cleaner now.

Did I see it correct that there was no testing for get_the_modified_date() and get_the_modified_time() yet?

Here is my suggestion s. 51166.3.diff

Also, I made the test functions public as told by phpcs. Looking through the test, that's really inconsistent used so far. Maybe a follow-up ticket for the future?

Last edited 4 years ago by wittich (previous) (diff)

#31 in reply to: ↑ 30 @SergeyBiryukov
4 years ago

Replying to wittich:

Did I see it correct that there was no testing for get_the_modified_date() and get_the_modified_time() yet?

Here is my suggestion s. 51166.3.diff

That looks great, thank you! It looks like there were some existing tests in general/template.php. I think it would make sense to move them to a new file: date/getTheModifiedDate.php. I'll handle it on commit.

Also, I made the test functions public as told by phpcs. Looking through the test, that's really inconsistent used so far. Maybe a follow-up ticket for the future?

A follow-up ticket for bringing some consistency to the public modifiers also sounds good :)

#32 @SergeyBiryukov
4 years ago

In 48924:

Tests: Move the tests for get_the_modified_time() to a more appropriate place.

Add some new tests to better cover the functionality, for consistency with get_the_date() and get_the_time().

Follow-up to [48911], [48912], [48918].

Props wittich.
Fixes #51184.

#33 @desrosj
4 years ago

[48924] looks good to backport.

#34 @desrosj
4 years ago

In 48926:

Tests: Move the tests for get_the_modified_time() to a more appropriate place.

Add some new tests to better cover the functionality, for consistency with get_the_date() and get_the_time().

Follow-up to [48911], [48912], [48918].

Props wittich.
Merges [48924] to the 5.5 branch.
Fixes #51184.

Note: See TracTickets for help on using tickets.