Skip to content

[3.10] [PHP 8.1] Fix UriHelper Automatic conversion of false to array#36774

Closed
beat wants to merge 3 commits intojoomla:3.10-devfrom
beat:patch-13
Closed

[3.10] [PHP 8.1] Fix UriHelper Automatic conversion of false to array#36774
beat wants to merge 3 commits intojoomla:3.10-devfrom
beat:patch-13

Conversation

@beat
Copy link
Contributor

@beat beat commented Jan 21, 2022

Fixes Deprecated: Automatic conversion of false to array is deprecated in libraries/vendor/joomla/uri/src/UriHelper.php on line 50

Pull Request for Issue # none, directly fix proposal.

Summary of Changes

Another PHP 8.1 warning fix.

Testing Instructions

Just review code would be enough as it's obvious PHP 8.1 issue and fix.

Open admin aera in PHP 8.1, find this warning.

Actual result BEFORE applying this Pull Request

Deprecated: Automatic conversion of false to array is deprecated in libraries/vendor/joomla/uri/src/UriHelper.php on line 50

Expected result AFTER applying this Pull Request

Not that warning anymore.

Documentation Changes Required

none.

Fixes `Deprecated: Automatic conversion of false to array is deprecated in libraries/vendor/joomla/uri/src/UriHelper.php on line 50`
@richard67
Copy link
Member

I have tested this item ✅ successfully on 7f591bc

Code review. Maybe not the most elegant way to fix it, having that check inside the loop, but it works and is safe and so ok for me.


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

@richard67
Copy link
Member

I have tested this item ✅ successfully on 8be1c23

Code review.


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

@beat
Copy link
Contributor Author

beat commented Jan 22, 2022

I have tested this item white_check_mark successfully on 7f591bcCode review. Maybe not the most elegant way to fix it, having that check inside the loop, but it works and is safe and so ok for me.

Thank you @richard67 for the reviews and feedback. After 2nd thought, found and committed a way more elegant fix proposal.

Actually the function definition itself isn't elegant, as it's returning array|bool. It could have returned an empty array instead of bool. But that was before PHP was moving to strict types.

The really elegant fix would be to change the API and return an empty array instead of bool false, and as as (bool) [] === false it would be staying compatible, but would represent strictly an API change looking at the phpdoc bloc.

@richard67
Copy link
Member

Actually the function definition itself isn't elegant, as it's returning array|bool. It could have returned an empty array instead of bool. But that was before PHP was moving to strict types.

The really elegant fix would be to change the API and return an empty array instead of bool false, and as as (bool) [] === false it would be staying compatible, but would represent strictly an API change looking at the phpdoc bloc.

Yeah, we have to live with that mess, but as far as I know it shall be cleaned up with 5.0, and that will be next year. So there is light at the end of the tunnel (I hope it's not a train) :-)

@Quy
Copy link
Contributor

Quy commented Jan 22, 2022

I have tested this item ✅ successfully on 8be1c23


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

@Quy
Copy link
Contributor

Quy commented Jan 22, 2022

RTC


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

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

zero-24 commented Jan 23, 2022

This here is a change in a composer dependency so this PR should be send against the framework's 1.x branch from where we would take it back down to the CMS.

@beat
Copy link
Contributor Author

beat commented Jan 23, 2022

This here is a change in a composer dependency so this PR should be send against the framework's 1.x branch from where we would take it back down to the CMS.

Done here: joomla-framework/uri#30

Letting you merge and/or close this PR once the framework-PR is merged.

@zero-24
Copy link
Contributor

zero-24 commented Jan 23, 2022

Let us close here and once @nibra did the upstream merge and version release we can pull the changes in with an composer update.

@Quy Quy closed this 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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants