Joomla! Issue Tracker | Joomla! CMS #30921 - Fix strtotime() warnings on PHP 8

? ? ?

Fixed in Code Base
7 Dec 2020
Medium
Build: staging
# 30921
Diff
SharkyKZ:j3/php8/strtotime

Success

Success Download Prebuilt packages are available for download. Details
Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
Success continuous-integration/drone/pr Build is passing Details

User tests: Successful: Unsuccessful:

SharkyKZ
5 Oct 2020

Summary of Changes

Removes use of strtotime() in favor of string comparison for dates.

Testing Instructions

Use 32-bit version of PHP 8.0.0 RC1 or newer dev build.
Create an article. Don't set Finish Publishing date.
View article list/blog/featured view containing the article in frontend.
View the article in frontend.

Actual result BEFORE applying this Pull Request

Warning: strtotime(): Epoch doesn't fit in a PHP integer

Expected result AFTER applying this Pull Request

No warnings.

Documentation Changes Required

No.

18bfb4d 4 Oct 2020

Fix strtotime() notices on PHP 8

SharkyKZ - open - 5 Oct 2020

SharkyKZ - change - 5 Oct 2020

Status

New

⇒

Pending

joomla-cms-bot - change - 5 Oct 2020

Category

⇒

Front End com_content Layout Templates (site)

richard67 - comment - 11 Oct 2020

Unfortunately I don't have the required testing environment, 32-bit version of PHP 8.0.0 RC1 or newer dev build.

But from code review it's absolutely clear what this PR does, and it makes sense, first check for the null date and then to a strtotime for the other check, and not vice versa.

@HLeithner What do you think? Would you count a good code review as a good test, too?

infograf768 - comment - 11 Oct 2020

@SharkyKZ
Can you also fix it here

joomla-cms/components/com_contact/tmpl/category/default_items.php

Lines 113 to 126 in a1ecbc2

    
           <?php if (strtotime($item->publish_up) > strtotime(Factory::getDate())) : ?> 
        
           	<div> 
        
           		<span class="list-published badge badge-warning"> 
        
           			<?php echo Text::_('JNOTPUBLISHEDYET'); ?> 
        
           		</span> 
        
           	</div> 
        
           <?php endif; ?> 
        
           <?php if (!is_null($item->publish_down) && strtotime($item->publish_down) < strtotime(Factory::getDate())) : ?> 
        
           	<div> 
        
           		<span class="list-published badge badge-warning"> 
        
           			<?php echo Text::_('JEXPIRED'); ?> 
        
           		</span> 
        
           	</div 
        
           <?php endif; ?>

SharkyKZ - comment - 11 Oct 2020

@infograf768 Will do. But I'm starting to question the validity of this fix. It has a limited range:

The valid range of a timestamp is typically from Fri, 13 Dec 1901 20:45:54 UTC to Tue, 19 Jan 2038 03:14:07 UTC. (These are the dates that correspond to the minimum and maximum values for a 32-bit signed integer.)

I guess we should account for values outside this range?

infograf768 - comment - 11 Oct 2020

I was born after 1901 and 2038 would mean I would be 90. Can't project so far. ;)
But this says that anyway code will have to be modified in 2038.
https://www.unixtimestamp.com/ (unix starts in 1970 btw)

https://en.wikipedia.org/wiki/Year_2038_problem

SharkyKZ - comment - 12 Oct 2020

Do we even need strtotime() here? Looks like we could be comparing date strings instead.

richard67 - comment - 12 Oct 2020

Do we even need strtotime() here? Looks like we could be comparing date strings instead.

@SharkyKZ Where do you mean? For the greater than or lower than comparisons we can only use strings if we can 100% be sure that they are in ISO format.

SharkyKZ - comment - 13 Oct 2020

Dates returned from the databases should always be the same format, right?

richard67 - comment - 13 Oct 2020

@SharkyKZ Yes, but the question is not if it is the same format always but if this format means that alphabetical sorting is equal to sorting by date and time, and this is not given for a format like e.g. "DD.MM.YYYY hh:nn:ss". Alphabetical sorting is only equal to sorting by date and time if the order of the time periods "year, month, day ... seconds" is either ascending or descending with the length of these periods, and if the number of digits used for each period is always the same, i.e. it will not work when using only 1 digit for months 1 to 9.

So the ISO formats (short or long) fulfil this, they go from year down to second: "YYYY-MM-DD hh:mm:ss".

It would theoretically also work with "ss:mm:hh DD-MM-YYYY", but nobody really uses that.

So if you can be 100% sure that the string representation is always the same ISO format, then you can compare strings like times.

But if not, then you have to force this format.

SharkyKZ - comment - 13 Oct 2020

But dates from the database are always going to be in YYYY-MM-DD hh:mm:ss (or Y-m-d H:i:s in PHP terms) format across all supported databases and locales?

richard67 - comment - 13 Oct 2020

But dates from the database are always going to be in YYYY-MM-DD hh:mm:ss (or Y-m-d H:i:s in PHP terms) format across all supported databases and locales?

@SharkyKZ Exactly that's what I'm not 100% sure about. At least on MS SQL Server it might be different.

richard67 - comment - 13 Oct 2020

@SharkyKZ I'd stay with the strtotime to play safe.

8b6d648 27 Oct 2020

Merge remote-tracking branch 'upstream/staging' into j3/php8/strtotime

f063ded 27 Oct 2020

Drop strtotime()

40ec9cf 27 Oct 2020

Restore order

SharkyKZ - comment - 27 Oct 2020

We can't use strtotime() because of the limitations. And we do only support Y-m-d H:i:s format for database dates. Database API can support different formats but this doesn't concern the CMS.

SharkyKZ - change - 27 Oct 2020

The description was changed

SharkyKZ - edited - 27 Oct 2020

d1529c2 27 Oct 2020

Correct method

287f96e 27 Oct 2020

Reduce code duplication

bbcb7bf 27 Oct 2020

Unnecessary brackets

SharkyKZ - change - 27 Oct 2020

The description was changed

SharkyKZ - edited - 27 Oct 2020

infograf768 - comment - 27 Oct 2020

#30921 (comment)

SharkyKZ - comment - 27 Oct 2020

I'll do that later. It's for 4.0 anyways. There's also same issue in calendar field on 4.0.

roland-d - comment - 21 Nov 2020

Like Richard I don have a 32-bit environment either, who does 🤔 However code review looks good.

HLeithner - comment - 21 Nov 2020

@roland-d can you test on a 64bit system?

bayareajenn - comment - 21 Nov 2020

also @roland-d will you see if Patch Tester gives you this PR in PHP8? :)

bayareajenn - test_item - 4 Dec 2020 - Tested successfully

bayareajenn - comment - 4 Dec 2020

I have tested this item ✅ successfully on bbcb7bf

I tested on 64 bit PHP8. Added article. Added cat blog, cat list, single article, featured on the frontend. No errors. Couldn't find it broke anything.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30921.}

gostn - test_item - 5 Dec 2020 - Tested successfully

gostn - comment - 5 Dec 2020

I have tested this item ✅ successfully on bbcb7bf

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30921.}

richard67 - comment - 5 Dec 2020

@gostn Which PHP version did you use? 8? If so: 32 or 64 bit?

gostn - comment - 5 Dec 2020

ups i forgot i'm a troll #30716 (comment)

HLeithner - comment - 5 Dec 2020

ups i forgot i'm a troll #30716 (comment)

not sure what you mean with this but in this case it's important to know which version you tested. (I mean it's always a useful information)

gostn - comment - 5 Dec 2020

if you read the comment you wll understand that asking by one who call you a troll is …

HLeithner - comment - 5 Dec 2020

ok @gostn but still no answer for the version

richard67 - comment - 5 Dec 2020

Exactly this behaviour was what I've meant.

gostn - comment - 6 Dec 2020

ok @gostn but still no answer for the version

as long as i´m called a troll and no apologize for that …

richard67 - change - 7 Dec 2020

Status

Pending

⇒

Ready to Commit

richard67 - comment - 7 Dec 2020

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30921.}

HLeithner - close - 7 Dec 2020

HLeithner - merge - 7 Dec 2020

HLeithner - change - 7 Dec 2020

Status	Ready to Commit	⇒	Fixed in Code Base
Closed_Date	0000-00-00 00:00:00	⇒	2020-12-07 16:06:32
Closed_By		⇒	HLeithner
Labels	Added: ?

HLeithner - comment - 7 Dec 2020

Thanks

Add a Comment

Older
Newer

Joomla! Issue Tracker - CMS

[#30921] - Fix strtotime() warnings on PHP 8

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

Add a Comment

	<?php if (strtotime($item->publish_up) > strtotime(Factory::getDate())) : ?>
	<div>
	<span class="list-published badge badge-warning">
	<?php echo Text::_('JNOTPUBLISHEDYET'); ?>
	</span>
	</div>
	<?php endif; ?>
	<?php if (!is_null($item->publish_down) && strtotime($item->publish_down) < strtotime(Factory::getDate())) : ?>
	<div>
	<span class="list-published badge badge-warning">
	<?php echo Text::_('JEXPIRED'); ?>
	</span>
	</div
	<?php endif; ?>