#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: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (40)
#1
@
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
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
#3
@
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:
↓ 5
@
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
@
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
@
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
@
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
}
#8
@
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.
#9
@
4 years ago
- Focuses template coding-standards added
- Keywords has-patch needs-testing added; needs-patch removed
#10
@
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).
#13
@
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...
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:
↓ 16
@
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.
#16
in reply to:
↑ 15
@
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?
#19
@
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:
↓ 21
@
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
@
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
@
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
@
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.
#26
@
4 years ago
- Keywords dev-reviewed added; dev-feedback removed
The changes look good to backport.
#30
follow-up:
↓ 31
@
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?
#31
in reply to:
↑ 30
@
4 years ago
Replying to wittich:
Did I see it correct that there was no testing for
get_the_modified_date()
andget_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 :)
dream-encode commented on PR #507:
4 years ago
#35
Merged into WP Core in https://core.trac.wordpress.org/changeset/48912
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()