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

Some changes for compatibility with PHP 8.1 #1812

Closed
wants to merge 26 commits into from

Conversation

luigifab
Copy link
Collaborator

@luigifab luigifab commented Sep 7, 2021

Description

Not sure if all changes are the best or not.
PHP 8.0.11 and 8.1.0-rc2 (from deb.sury.org).

Manual testing scenario

First, disable Suppress deprecation warnings in app/code/core/Mage/Core/functions.php:

diff --git a/app/code/core/Mage/Core/functions.php b/app/code/core/Mage/Core/functions.php
index 29e308d1b6..88b17f9c83 100644
--- a/app/code/core/Mage/Core/functions.php
+++ b/app/code/core/Mage/Core/functions.php
@@ -136,9 +136,9 @@ function mageCoreErrorHandler($errno, $errstr, $errfile, $errline)
     }

     // Suppress deprecation warnings on PHP 7.x
-    if ($errno == E_DEPRECATED && version_compare(PHP_VERSION, '7.0.0', '>=')) {
-        return true;
-    }
+    //if ($errno == E_DEPRECATED && version_compare(PHP_VERSION, '7.0.0', '>=')) {
+    //    return true;
+    //}

     // PEAR specific message handling
     if (stripos($errfile.$errstr, 'pear') !== false) {

Go to your backend login page, login, go to dashboard, then enable PHP 8.1, clear your cache, and press F5.

Commit 1 fix:

Deprecated functionality: trim(): Passing null to parameter #1 ($string) of type string is deprecated
  in app/code/core/Mage/Core/Model/Config.php on line 1292

Apply commit 1, then press F5.

Commit 2 fix:

Deprecated functionality: PDOStatement::fetch(): Passing null to parameter #2 ($cursorOrientation)
  of type int is deprecated in lib/Zend/Db/Statement/Pdo.php on line 254

Solution found here: matomo-org/matomo#17689
Apply commit 2, then press F5.

Commit 3 fix:

Deprecated functionality: str_replace(): Passing null to parameter #3 ($subject) of type array|string
  is deprecated in app/code/core/Mage/Core/Model/Resource/Config.php on line 104

Apply commit 3, then press F5.

Dashboard is displayed.
Clear your cache and your log directory, then press F5.

Commit 4 fix (exception.log):

Exception: Deprecated functionality: strlen(): Passing null to parameter #1 ($string) of type string
  is deprecated  in app/code/core/Mage/Core/Helper/Abstract.php on line 218

Apply commit 4, then press F5.

Clear your cache and your log directory, then press F5.
Go to Sales / Order.

Commit 5 fix:

Deprecated functionality: strpos(): Passing null to parameter #1 ($haystack) of type string is
  deprecated in app/code/core/Mage/Core/Model/Design/Package.php on line 540

Apply commit 5, then press F5.

Commit 6 fix:

Deprecated functionality: strtolower(): Passing null to parameter #1 ($string) of type string
  is deprecated in app/Mage.php on line 424

Apply commit 6, then press F5.

Commit 7 fix:

Deprecated functionality: addcslashes(): Passing null to parameter #1 ($string) of type string
  is deprecated  in lib/Zend/Db/Adapter/Pdo/Abstract.php on line 296

TODO Make a PR for zf1-future.
Apply commit 7, then press F5.

Commit 8 fix:

Deprecated functionality: strtolower(): Passing null to parameter #1 ($string) of type string
  is deprecated in app/code/core/Mage/Adminhtml/Block/Widget/Grid/Column.php on line 304

Apply commit 8, then press F5.

Commit 9 fix:

Deprecated functionality: strtolower(): Passing null to parameter #1 ($string) of type string
  is deprecated in app/code/core/Mage/Adminhtml/Block/Widget/Grid/Column.php on line 209

Apply commit 9, then press F5.

Commit 10 fix:

Deprecated functionality: strtolower(): Passing null to parameter #1 ($string) of type string
  is deprecated in app/code/core/Mage/Adminhtml/Block/Widget/Grid/Column/Renderer/Abstract.php
  on line 110

Apply commit 10, then press F5.

Commit 11 fix:

Deprecated functionality: htmlspecialchars(): Passing null to parameter #1 ($string) of type string
  is deprecated in app/code/core/Mage/Adminhtml/Block/Widget/Grid/Column/Filter/Abstract.php
  on line 97

Apply commit 11, then press F5.

Orders grid is displayed.
Click on Export to CSV button.

Commit 12 fix:

Deprecated functionality: auto_detect_line_endings is deprecated in lib/Varien/Io/File.php on line 134

Apply commit 12, then press F5.

Back to Sales / Order.
Filter grid and reset filter.
Sort grid.
Go to any pending order.

Commit 13 fix:

Deprecated functionality: trim(): Passing null to parameter #1 ($string) of type string is deprecated
  in app/code/core/Mage/Eav/Model/Entity/Attribute/Abstract.php on line 641

Apply commit 13, then press F5.

Commit 14 fix:

Deprecated functionality: trim(): Passing null to parameter #1 ($string) of type string is deprecated
  in app/code/core/Mage/Eav/Model/Entity/Type.php on line 267

Apply commit 14, then press F5.

Order is displayed.
Click on Invoice button.

Commit 15 fix:

Deprecated functionality: urlencode(): Passing null to parameter #1 ($string) of type string is
  deprecated in lib/Zend/Locale/Data.php on line 1001

TODO Make a PR for zf1-future.
Apply commit 15, then press F5.

Commit 16 fix:

Deprecated functionality: preg_split(): Passing null to parameter #3 ($limit) of type int is
  deprecated in app/code/core/Mage/Core/Helper/String.php on line 190

Apply commit 16, then press F5.

New Invoice for Order is displayed.
Set quantity to invoice to 0 and press Update Qty button.
TODO There is an hidden error.

Click on Submit invoice button.
Click on Ship button.
Click on Submit shipment button.
Click on Credit memo button.
Set quantity to refund to 0 and press Update Qty button.
Click on Refund offline button.
Click on Credit memo button.
Click on Refund offline button.

Go to Sales / Invoice.
Click on export to CSV button.
Filter grid and reset filter.
Sort grid.
Go to any invoice.

Go to Sales / Shipment.
Click on export to CSV button.
Filter grid and reset filter.
Sort grid.
Go to any shipment.

Go to Sales / Credit memos.
Click on export to CSV button.
Filter grid and reset filter.
Sort grid.
Go to any credit memo.

Go to Sales / Order, click on Create New Order.

Commit 17 fix:

Deprecated functionality: htmlspecialchars(): Passing null to parameter #1 ($string) of type string is
  deprecated in lib/Varien/Data/Form/Element/Abstract.php on line 217

Apply commit 17, then press F5.

Commit 18 fix:

Deprecated functionality: preg_match(): Passing null to parameter #2 ($subject) of type string is
  deprecated in app/code/core/Mage/Eav/Model/Entity/Attribute/Backend/Time/Created.php on line 45

Apply commit 18, then press F5.

Commit 19 fix:

Deprecated functionality: explode(): Passing null to parameter #2 ($string) of type string is
  deprecated in app/code/core/Mage/Eav/Model/Attribute/Data/Multiline.php on line 138

Apply commit 19, then press F5.

Create new order is displayed.
Filter grid and reset filter.
Sort grid.
Select a customer and select a store view.
Add a product and select a shipping method.
Click on Submit order button.
Order is displayed.
Click on Cancel button.
Click on Reorder button.
Click on Submit order button.

Go to Sales / Terms and Conditions.
Filter grid and reset filter.
Sort grid.
Click on Add new button.

Commit 20 fix:

Deprecated functionality: explode(): Passing null to parameter #2 ($string) of type string is
  deprecated in lib/Varien/Data/Form/Element/Multiselect.php on line 80

Apply commit 20, then press F5.

New Terms and Conditions is displayed.
Fill the form and click on Save condition button.
Reboot your computer.

Go to Sales / Order.

Commit 21 fix:

Deprecated functionality: strpos(): Passing null to parameter #1 ($haystack) of type string is
  deprecated in lib/Varien/Object.php on line 343

Apply commit 21, then press F5.
Open a complete order.

Commit 22 fix:

Deprecated functionality: preg_match(): Passing null to parameter #2 ($subject) of type string
  is deprecated in app/design/adminhtml/default/default/template/widget/tabs.phtml on line 35

Apply commit 22, then press F5.

Go to Sales / Tax / Import Export.
Click on Export.

Commit 23 fix:

Deprecated functionality: str_replace(): Passing null to parameter #2 ($replace) of type
  array|string is deprecated in lib/Varien/Object.php on line 612

Apply commit 23, then press F5.
Go to Customers / Customers.
Go to any customer.

Commit 24 fix:

Deprecated functionality: strtotime(): Passing null to parameter #1 ($datetime) of type string
  is deprecated  in app/code/core/Mage/Adminhtml/Block/Customer/Edit/Tab/View.php on line 144

Apply commit 24, then press F5.

Commit 25 fix:

Deprecated functionality: trim(): Passing null to parameter #1 ($string) of type string
  is deprecated in app/code/core/Mage/Core/Model/Config.php on line 1368

Apply commit 25, then press F5.

Customer edit is displayed.
Go to dashboard.

Commit 26 fix:

Deprecated functionality: preg_match(): Passing null to parameter #2 ($subject) of type
  string is deprecated in app/design/adminhtml/default/default/template/widget/tabshoriz.phtml on line 34

Apply commit 26, then press F5.

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

@github-actions github-actions bot added the Component: lib/* Relates to lib/* label Sep 7, 2021
@kiatng kiatng added the PHP 8 Related to PHP8 label Sep 7, 2021
@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Core Relates to Mage_Core Component: Eav Relates to Mage_Eav Component: lib/Zend Component: lib/Varien Relates to lib/Varien Component: Widget Relates to Mage_Widget Mage.php Relates to app/Mage.php Template : admin Relates to admin template labels Oct 9, 2021
@luigifab luigifab changed the title Update mcrypt for PHP 8.1 Some changes for compatibility with PHP 8.1 Oct 9, 2021
@luigifab luigifab force-pushed the mcryptphp81 branch 2 times, most recently from c1a5664 to 1e8f87f Compare October 14, 2021 17:26
@Flyingmana
Copy link
Member

maybe split the PR up a bit type of changes, the size looks already like it needs an hour to properly Review for me.

@luigifab
Copy link
Collaborator Author

Not sure how to proceed.
However, I just saw that one of the changes prevents multiselect from working (for example in System / Configuration / General / Countries options)... 😸.

@Sdfendor
Copy link
Contributor

Sdfendor commented Dec 3, 2021

@luigifab First of all sorry that I overlooked this PR before creating #1891. But I have a question regarding your solution for the deprecation notices in Commit 1 fix.
Do you really think it is necessary to differentiate the PHP versions and duplicate the code like that? My reasoning is that return types exist in PHP since 7.0 and OpenMage requires 7.0. There should be no implications regarding backwards compatibility. The types of the two methods haven't changed (implicitly in phpdoc) over the various PHP versions and are only explicit now in 8.1 (tentatively).
If there are good reasons for the duplication and differentiation I will naturally abandon my PR as a duplication.

@luigifab
Copy link
Collaborator Author

luigifab commented Dec 3, 2021

Oh yes, you are right, I don't remember why I didn't do that. Your solution is better.

@@ -101,7 +101,7 @@ public function loadToXml(Mage_Core_Model_Config $xmlConfig, $condition = null)
if ($r['scope'] !== 'default') {
continue;
}
$value = str_replace($substFrom, $substTo, $r['value']);
$value = empty($r['value']) ? $r['value'] : str_replace($substFrom, $substTo, $r['value']);
Copy link
Contributor

@Sdfendor Sdfendor Dec 13, 2021

Choose a reason for hiding this comment

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

This added empty-check must also be applied to line 121 and 150 for website and store scope, respectively.

@@ -214,7 +214,7 @@ public function removeClass($class)
*/
protected function _escape($string)
{
return htmlspecialchars($string, ENT_COMPAT);
return is_string($string) ? htmlspecialchars($string, ENT_COMPAT) : $string;
Copy link
Contributor

Choose a reason for hiding this comment

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

In general I'm happy with your PR mainly relying on added typechecks to avoid problems with PHP 8.1. But in this case I would like to argue that an explicit cast to string htmlspecialchars((string)$string, ENT_COMPAT) would be more appropriate to use (because we don't want to introduce parameter types here to avoid breaking things).
The method clearly escapes a string, the parameter name and the phpdoc type point in that direction. While a is_string is not taxing the check will be performed a lot of times judging by its location in Varien Lib.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest

return htmlspecialchars((string)$string, ENT_COMPAT);

@Sdfendor
Copy link
Contributor

Sdfendor commented Jan 6, 2022

While working on OpenMage compatibility with PHP 8.1 myself, I'm using this PR as a reference, but have encountered additional errors that are similar in nature and not part of any custom code of the OM instance. Would you say we extend this PR further (it's still a draft after all but on the other hand already quite heavy) or should I provide complementary PRs?

@luigifab
Copy link
Collaborator Author

luigifab commented Jan 6, 2022

You can create a new PR to replace this PR, or multiple PR, as you want.
For now, I not working on it since some weeks.

@@ -140,8 +140,8 @@ public function getStoreLastLoginDateTimezone()
public function getCurrentStatus()
{
$log = $this->getCustomerLog();
if ($log->getLogoutAt() ||
strtotime(now())-strtotime($log->getLastVisitAt())>Mage_Log_Model_Visitor::getOnlineMinutesInterval()*60) {
if ($log->getLogoutAt() || (!empty($log->getLastVisitAt()) &&
Copy link
Contributor

@Sdfendor Sdfendor Jan 19, 2022

Choose a reason for hiding this comment

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

This fix removes the deprecation error but introduces a bug. Up to the fix the behavior has been that strtotime(null) evaluates to false so nothing is subtracted (false = 0) from strtotime(now()) and the 2nd condition evaluates to true so 'offline' is returned. With your fix in those cases the method returns 'online'
.
Your solution displays:
image
before:
image

I fixed it by combining your check with correctly executing the subtraction:

if ($log->getLogoutAt() 
    || strtotime(now()) - (!empty($log->getLastVisitAt()) ? strtotime($log->getLastVisitAt()) : 0) > Mage_Log_Model_Visitor::getOnlineMinutesInterval() * 60) {
    return Mage::helper('customer')->__('Offline');
}

@luigifab
Copy link
Collaborator Author

Today I see that: Function strftime() is deprecated
There are some occurrences.

@kiatng
Copy link
Collaborator

kiatng commented Jan 26, 2022

Today I see that: Function strftime() is deprecated There are some occurrences.

Also gmstrftime .

@sreichel
Copy link
Contributor

https://php.watch/versions/8.1/strftime-gmstrftime-deprecated

@tmotyl
Copy link
Contributor

tmotyl commented Apr 11, 2022

@luigifab thanks for your great work.
I have one suggestion. Insteaf of checking "is_string()", please go with casting to string.
I've added suggestions in some places with this change.
This will make code less cluttered.

Copy link
Contributor

@tmotyl tmotyl left a comment

Choose a reason for hiding this comment

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

please cast value when passing to strtolower, in php7 strtolower was always converting value to string. see https://3v4l.org/J7S66

@@ -421,7 +421,8 @@ public static function getStoreConfig($path, $store = null)
*/
public static function getStoreConfigFlag($path, $store = null)
{
$flag = strtolower(self::getStoreConfig($path, $store));
$flag = self::getStoreConfig($path, $store);
$flag = is_string($flag) ? strtolower($flag) : $flag;
Copy link
Contributor

Choose a reason for hiding this comment

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

$flag = strtolower((string)self::getStoreConfig($path, $store));

@luigifab
Copy link
Collaborator Author

luigifab commented Jun 6, 2022

There is an error in this PR, and I didn't work on it...
So this is the end.

@sreichel
Copy link
Contributor

@luigifab great stuff! ❤️

(for the next time ... please split it into smaller junks to make reviews easier. I know it from my own (large) PRs ... smaller PRs are reviewed/merged faster)

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: Core Relates to Mage_Core Component: Eav Relates to Mage_Eav Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Widget Relates to Mage_Widget Don't forget this PR Mage.php Relates to app/Mage.php PHP 8 Related to PHP8 Template : admin Relates to admin template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants