[#36799] - [3.10] [PHP 8.1] Fixes Table User for Date/Date.php warning on login
- Fixed in Code Base
- 23 Jan 2022
- Medium
- Build: 3.10-dev
- # 36799
- Diff
- beat:patch-23
User tests: Successful: Unsuccessful:
Pull Request for Issue # none
Summary of Changes
Fixes Deprecated: DateTime::__construct(): Passing null to parameter #1 ($datetime) of type string is deprecated in /home/beat/www/j/libraries/src/Date/Date.php on line 112 error on login
A new \Joomla\Date\Date($date, $tz); requires a string as $date, and defaults to 'now'. It does not treat the special case of null, which in PHP 8.1 warns deprecation, so I think that the correct place to fix the null parameter is just above where the default for "now" is NULL, in the call of the below where the default is 'now' and doesn't treat null.
Testing Instructions
Code-review or test;
PHP 8.1 with all errors on, and joomla debug on. Then login in front-end, and before redirect it has that warning (at least when CB is installed as CB pauses redirects when there is an error).
Actual result BEFORE applying this Pull Request
Deprecated: DateTime::__construct(): Passing null to parameter #1 ($datetime) of type string is deprecated in /home/beat/www/j/libraries/src/Date/Date.php on line 112 error on login
Expected result AFTER applying this Pull Request
That warning disappears.
Documentation Changes Required
None.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
I have tested this item
Code review.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36799.
| Status | Pending | ⇒ | Ready to Commit |
| Labels |
Added:
?
?
|
||
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36799.
Should't it be better and easier to understand to set the "now" case in the signature:
joomla-cms/libraries/src/Table/User.php
Line 465 in 5b18831
Should't it be better and easier to understand to set the "now" case in the signature:
joomla-cms/libraries/src/Table/User.php
Line 465 in 5b18831
That would be a change in the signature. Wouldn't that be some kind of a b/c break?
Is changing the default value in the signature a b/c breaking change? I mean other than the same change done later in the code which result into the same behavior?
Changing the order or the name is but i would be suprised when changing the default value would be an issue too? I'm missing somehting?
I don't know. Was just asking.
Is changing the default value in the signature a b/c breaking change? I mean other than the same change done later in the code which result into the same behavior?
Changing the order or the name is but i would be suprised when changing the default value would be an issue too? I'm missing somehting?
Changing that function's default parameter from $timestamp = null to $timestamp = 'now' does not fix the bug, because it's called from Joomla\CMS\User\User->setLastVisit($timestamp = null) which calls the function I fixed with return $table->setLastVisit($timestamp); like this https://github.com/joomla/joomla-cms/blob/4.1-dev/libraries/src/User/User.php#L501-L509 .
Moreover, changing the default of this @param integer $timestamp to string "now" is a change of ths signature of the function, which would become ?integer|string-typed and still have to honor a "null" parameter. As it would become a type-mixing, my argument is that my fix is the right middle-ground. Imho, it's the function below this one (\JFactory::getDate($timeStamp)that is unclean in its parameter and its implementation. But it's Joomla 3.10, so we are not going to break B/C.
Due to the above, I maintain my suggested fix proposal.
Thanks.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-01-23 17:07:44 |
| Closed_By | ⇒ | zero-24 | |
| Labels |
Added:
?
|
||
I have tested this item✅ successfully on 5b18831
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36799.