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

Fix getChildren must be compatible with SimpleXMLElement with PHP 8 #1603

Closed
wants to merge 1 commit into from
Closed

Fix getChildren must be compatible with SimpleXMLElement with PHP 8 #1603

wants to merge 1 commit into from

Conversation

luigifab
Copy link
Contributor

@luigifab luigifab commented May 6, 2021

Description

This PR fix the following error with PHP 8 (#1602):

Fatal error: Declaration of Mage_Api_Model_Wsdl_Config_Element::getChildren($source)
 must be compatible with SimpleXMLElement::getChildren() in
 [...]/app/code/core/Mage/Api/Model/Wsdl/Config/Element.php on line 179

OpenMage 20.0.10 / PHP 8.0.5

Manual testing scenarios

$proxy = new SoapClient('https://.../api/v2_soap/?wsdl', ['user_agent' => 'Local/Test']);
$sessionId = $proxy->login('...', '...');
$products = $proxy->catalogProductList($sessionId, [], 1);
foreach ($products as $product)
	echo $product->product_id,' ',$product->name,"\n";

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 the Component: Api PageRelates to Mage_Api label May 6, 2021
@kiatng kiatng added the PHP 8 Related to PHP8 label May 7, 2021
Copy link
Contributor

@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.

More info on the error in PHP doc. The mandatory parameter needs to be optional to have signature compatibility.

@Flyingmana
Copy link
Contributor

anyone found the documentation for the SimpleXMLElement::getChildren method?
Is this a new method?

@jfksk
Copy link

jfksk commented May 7, 2021

@Flyingmana
The documentation seems to be missing. According to PHP src iterator functionality was added to SimpleXMLElement in 8.0. The signature according to the stub (changes/added methods):

SimpleXMLElement::getChildren() : ?SimpleXMLElement

@Flyingmana
Copy link
Contributor

thank you. The return type could lead to major issues for us here.
Did someone already test if the return type will also cause a signature error, and if returning an array will cause issues?

colinmollenhour added a commit to colinmollenhour/magento-lts that referenced this pull request May 14, 2021
@colinmollenhour
Copy link
Member

I can see how the this PR would fix the issue but making the parameter optional when it really is required is hard to stomach.. :)

I propose an alternative in #1613 which changes the method name so it doesn't conflict with the parent method. It doesn't use "$this" so I made it static as well.

@Flyingmana
Copy link
Contributor

closed in favor of #1613

Still, thank you a lot

@Flyingmana Flyingmana closed this May 23, 2021
Flyingmana pushed a commit that referenced this pull request May 23, 2021
…1613)

* Alternative solution for #1603 (Fix getChildren must be compatible with SimpleXMLElement with PHP 8)

* Add back original method as deprecated.
@luigifab luigifab deleted the path-simplexmlelement branch May 23, 2021 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Api PageRelates to Mage_Api PHP 8 Related to PHP8
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants