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

Fixes core and lib issues for PHP 8.0 compatibility #1391

Merged
merged 4 commits into from Apr 17, 2021

Conversation

lamskoy
Copy link
Contributor

@lamskoy lamskoy commented Jan 12, 2021

Description (*)

  1. Adds check for PHP 8 and phpseclib: if it's PHP 8.0+ then disallow inline crypt mode
  2. Fixes each() function calls - not available in PHP 8.0 anymore
  3. Fixes create_function() function calls - not available in PHP 8.0 anymore
  4. Fixes lib/Zend/Xml/Security.php: version_compare doesn't support 'lt' 'gte' 'gt' compare operators anymore in PHP 8.0
  5. Inside app/code/core/Mage/Catalog/Model/Product/Type/Configurable.php fixes wrong calls getProduct($product) for objects:

a) Mage_Sales_Model_Quote_Item_Option has getProduct() method which is not taking any parameters!
b) Mage_Catalog_Model_Product_Configuration_Item_Option is Varien_Object derivative, and "product" is just field containing one product data, not a collection inside. (It's causing issue under PHP 8.0 inside Varien_Object::getData() method passing instance of Catalog_Model_Product as $key argument)

Manual testing scenarios (*)

  1. Add to cart some configurable product on frontend under PHP 8.0

Questions or comments

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)

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Component: lib/Zend Component: lib/Mage Relates to lib/Mage Component: lib/* Relates to lib/* labels Jan 12, 2021
@lamskoy
Copy link
Contributor Author

lamskoy commented Jan 12, 2021

Unit tests seems to be broken:
github.GithubException.GithubException: 403 {"message": "Resource not accessible by integration", "documentation_url": "https://docs.github.com/rest/reference/checks#create-a-check-run"}

@tmotyl tmotyl added the PHP 8 Related to PHP8 label Jan 12, 2021
@Flyingmana
Copy link
Member

sorry about the unittest, the action has a permission issue for sending the results as a comment to the PR. Have modified to allow this step to fail in the future without marking the run as failed.

@jfksk
Copy link

jfksk commented Mar 4, 2021

It seems phpseclib removed create_function altogether. Maybe we could simplify that a little and just follow phpseclib here until we eventually update it?

@lamskoy
Copy link
Contributor Author

lamskoy commented Apr 8, 2021

Have reverted phpseclib changes. Please review. All auto tests are passed

@lamskoy lamskoy requested a review from kiatng April 9, 2021 21:35
Copy link
Collaborator

@kiatng kiatng left a comment

Choose a reason for hiding this comment

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

Thanks.

@@ -289,7 +289,7 @@ protected static function _decodeYaml($currentIndent, &$lines)
{
$config = array();
$inIndent = false;
while (list($n, $line) = each($lines)) {
foreach($lines as $n => $line) {
Copy link

Choose a reason for hiding this comment

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

Add space after 'foreach' to meet PSR-4 standards:
foreach ($lines as $n => $line) {

@@ -88,7 +88,7 @@ public static function getAllCapabilities(TeraWurfl $wurflObj)
if (!is_array($group)) {
continue;
}
while (list ($key, $value) = each($group)) {
foreach($group as $key => $value) {
Copy link

Choose a reason for hiding this comment

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

Add space after 'foreach' to meet PSR-4 standards:
foreach ($group as $key => $value) {

@@ -486,13 +486,15 @@ protected static function _createSimpleXMLElement(&$xml)
*/
protected static function _extractTypeAndValue(SimpleXMLElement $xml, &$type, &$value)
{
list($type, $value) = each($xml);
$value = reset($xml);
Copy link
Member

Choose a reason for hiding this comment

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

technically this is a logic change, as each() returns the current, and reset() the first.
Also each() moves the internal coursor.
But from the usage here it seems to not matter, and also makes the result more predictable, so Iam ok with leaving it as it is.

@Flyingmana Flyingmana merged commit feebf55 into OpenMage:1.9.4.x Apr 17, 2021
@github-actions
Copy link

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
2 runs  ±0  2 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit feebf55. ± Comparison against base commit 2fe9658.

@fballiano
Copy link
Contributor

should we maintain ZF1 by ourselves? I've found this project https://github.com/Shardj/zf1-future/ which I think it's quite interesting, it would have its own challanges but...

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: lib/Mage Relates to lib/Mage Component: lib/* Relates to lib/* PHP 8 Related to PHP8
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants