Skip to content

Fix strtotime() warnings on PHP 8#30921

Merged
HLeithner merged 7 commits intojoomla:stagingfrom
SharkyKZ:j3/php8/strtotime
Dec 7, 2020
Merged

Fix strtotime() warnings on PHP 8#30921
HLeithner merged 7 commits intojoomla:stagingfrom
SharkyKZ:j3/php8/strtotime

Conversation

@SharkyKZ
Copy link
Contributor

@SharkyKZ SharkyKZ commented Oct 5, 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.

@richard67
Copy link
Member

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
Copy link
Member

@SharkyKZ
Can you also fix it here

<?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
Copy link
Contributor Author

@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
Copy link
Member

infograf768 commented Oct 11, 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
Copy link
Contributor Author

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

@richard67
Copy link
Member

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
Copy link
Contributor Author

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

@richard67
Copy link
Member

richard67 commented Oct 13, 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
Copy link
Contributor Author

SharkyKZ commented Oct 13, 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
Copy link
Member

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
Copy link
Member

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

@SharkyKZ
Copy link
Contributor Author

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.

@infograf768
Copy link
Member

#30921 (comment)

@SharkyKZ
Copy link
Contributor Author

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

@HLeithner HLeithner added this to the Joomla! 3.9.24 milestone Nov 18, 2020
@roland-d
Copy link
Contributor

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

@HLeithner
Copy link
Member

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

@bayareajenn
Copy link

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

@bayareajenn
Copy link

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.

@ghost
Copy link

ghost commented Dec 5, 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
Copy link
Member

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

@ghost
Copy link

ghost commented Dec 5, 2020

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

@HLeithner
Copy link
Member

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)

@ghost
Copy link

ghost commented Dec 5, 2020

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

@HLeithner
Copy link
Member

ok @Gostn but still no answer for the version

@richard67
Copy link
Member

Exactly this behaviour was what I've meant.

@ghost
Copy link

ghost commented Dec 6, 2020

ok @Gostn but still no answer for the version

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

@joomla-cms-bot joomla-cms-bot removed this from the Joomla! 3.9.24 milestone Dec 7, 2020
@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 7, 2020
@richard67 richard67 added this to the Joomla! 3.9.24 milestone Dec 7, 2020
@HLeithner HLeithner merged commit 49e6e6a into joomla:staging Dec 7, 2020
@HLeithner
Copy link
Member

Thanks

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 7, 2020
flutterderp added a commit to flutterderp/com_blog that referenced this pull request Jan 12, 2021
• fix batch/copy on php 8 ([#31536](joomla/joomla-cms#31536))
• Fix strtotime() warnings on PHP 8 ([#30921](joomla/joomla-cms#30921))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PHP 8.x PHP 8.x deprecated issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants