Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PHP 8.1] Fix passing null to non-nullable internal function params #2586

Merged
merged 37 commits into from
Jan 10, 2023

Conversation

elidrissidev
Copy link
Member

@elidrissidev elidrissidev commented Sep 10, 2022

Description (*)

This PR fixes lots of "Passing null to non-nullable internal function parameters" deprecation notices introduced in PHP 8.1, which I came across when navigating or performing different actions on the frontend and admin.

The commits are grouped by function for easier review, and the fixes favor casting when appropriate, and PHPdocs to determine where to do the change. For example: Mage_Core_Helper_Data::escapeHtml PHPdoc says it accepts a non-nullable string, so instead of casting the parameter to a string inside the method, I cast the arguments in the usages instead.

Deprecation errors in lib/Zend were skipped in the hope we'll switch to zf1-future soon (?).

Related Pull Requests

Manual testing scenarios (*)

  1. Make sure the error_reporting directive in your php.ini includes deprecation errors, then comment out the lines below:
    // Suppress deprecation warnings on PHP 7.x
    if ($errno == E_DEPRECATED && version_compare(PHP_VERSION, '7.0.0', '>=')) {
    return true;
    }

Questions or comments

Please make sure to review thoroughly before approving.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@elidrissidev elidrissidev added the PHP 8 Related to PHP8 label Sep 10, 2022
@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Component: CatalogSearch Relates to Mage_CatalogSearch Component: Checkout Relates to Mage_Checkout Component: Cms Relates to Mage_Cms Component: Core Relates to Mage_Core Component: Cron Relates to Mage_Cron Component: Customer Relates to Mage_Customer Component: Downloadable Relates to Mage_Downloadable Component: Eav Relates to Mage_Eav Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Persistant Relates to Mage_Persistant Component: Rule Relates to Mage_Rule Component: Sales Relates to Mage_Sales Component: SalesRule Relates to Mage_SalesRule Component: Widget Relates to Mage_Widget Mage.php Relates to app/Mage.php Template : admin Relates to admin template Template : base Relates to base template Template : default Relates to base template Template : rwd Relates to rwd template labels Sep 10, 2022
@fballiano
Copy link
Contributor

LGTM

@luigifab
Copy link
Contributor

Today is a great day!

@fballiano
Copy link
Contributor

@sreichel why hold?

colinmollenhour
colinmollenhour previously approved these changes Jan 9, 2023
Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I inspected each change by reading code and believe that the new code is functionally equivalent to the old code and will fix possible PHP 8.1 errors.

One file appears to have triggered PHPStan so I made a suggestion although the submitted code is just as correct.

@sreichel
Copy link
Contributor

sreichel commented Jan 9, 2023

@colinmollenhour stop stop stop :)

This PR was already closed b/c it was split into smaller PRs.

There are some lines I'd like to check .... eg (string)$this->getPath() could be covered by adding getterMethod that returns string,

@colinmollenhour
Copy link
Member

@colinmollenhour stop stop stop :)

Oops, my bad.. :) Just trying to get this merged sooner than later since it seems as people run into these issues one by one there will be many more PRs.

@sreichel
Copy link
Contributor

sreichel commented Jan 9, 2023

edit: we dont need no PR for every change. We can merge whats left over at the end.

@sreichel sreichel marked this pull request as ready for review January 10, 2023 18:11
@sreichel sreichel marked this pull request as draft January 10, 2023 18:12
@github-actions github-actions bot removed the Component: SalesRule Relates to Mage_SalesRule label Jan 10, 2023
@sreichel
Copy link
Contributor

could be covered by adding getterMethod that returns string,

Can be done later. Think its ready.

@sreichel sreichel marked this pull request as ready for review January 10, 2023 20:09
@fballiano
Copy link
Contributor

it also seems fine to me

@elidrissidev
Copy link
Member Author

Finally! this PR has went through so much 😆

@fballiano
Copy link
Contributor

I'll just wait for the workflows to finish then merge to both branches, thanks guys

@fballiano fballiano merged commit bd0cb12 into OpenMage:1.9.4.x Jan 10, 2023
@fballiano
Copy link
Contributor

cherry picked to v20 cause no conflicts

@elidrissidev elidrissidev deleted the fix/php8.1-compat branch January 10, 2023 20:27
kiatng pushed a commit to kiatng/magento-lts that referenced this pull request May 14, 2023
* Updated calls to strlen(), ref OpenMage#2586

* Update/fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Component: Core Relates to Mage_Core Component: Customer Relates to Mage_Customer Component: Eav Relates to Mage_Eav Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Widget Relates to Mage_Widget hold PHP 8.1 Related to PHP 8.1 phpstan
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants