Skip to content

[3.10] [PHP 8.1] Fixes Table User for Date/Date.php warning on login#36799

Merged
zero-24 merged 1 commit intojoomla:3.10-devfrom
beat:patch-23
Jan 23, 2022
Merged

[3.10] [PHP 8.1] Fixes Table User for Date/Date.php warning on login#36799
zero-24 merged 1 commit intojoomla:3.10-devfrom
beat:patch-23

Conversation

@beat
Copy link
Copy Markdown
Contributor

@beat beat commented Jan 23, 2022

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.

@beat
[3.10] [PHP 8.1] Fixes Table User for Date/Date.php warning on login
5b18831 
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
@alikon alikon added the PHP 8.x PHP 8.x deprecated issues label Jan 23, 2022
@alikon
Copy link
Copy Markdown
Contributor

alikon commented Jan 23, 2022

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.

@richard67
Copy link
Copy Markdown
Member

richard67 commented Jan 23, 2022

I have tested this item ✅ successfully on 5b18831

Code review.

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

@richard67
Copy link
Copy Markdown
Member

richard67 commented Jan 23, 2022

RTC

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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 23, 2022
@zero-24
Copy link
Copy Markdown
Contributor

zero-24 commented Jan 23, 2022

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

public function setLastVisit($timeStamp = null, $userId = null)

@richard67
Copy link
Copy Markdown
Member

richard67 commented Jan 23, 2022

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

public function setLastVisit($timeStamp = null, $userId = null)

That would be a change in the signature. Wouldn't that be some kind of a b/c break?

@zero-24
Copy link
Copy Markdown
Contributor

zero-24 commented Jan 23, 2022

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?

@richard67
Copy link
Copy Markdown
Member

richard67 commented Jan 23, 2022

I don't know. Was just asking.

@beat
Copy link
Copy Markdown
Contributor Author

beat commented Jan 23, 2022 •
edited
Loading

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.

@zero-24 zero-24 added this to the Joomla 3.10.6 milestone Jan 23, 2022
@zero-24
Copy link
Copy Markdown
Contributor

zero-24 commented Jan 23, 2022

Thanks.

@zero-24 zero-24 merged commit d19bbe0 into joomla:3.10-dev Jan 23, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

PHP 8.x PHP 8.x deprecated issues

Projects

None yet

Milestone

Joomla 3.10.6

Development

Successfully merging this pull request may close these issues.

5 participants

@beat @alikon @richard67 @zero-24 @joomla-cms-bot