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.

Sign up for GitHub

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

Jump to bottom

Optional parameters before required ones in constructor #280

Closed
mfickers opened this issue Feb 13, 2023 · 4 comments · Fixed by #282 · May be fixed by #294
Closed

Optional parameters before required ones in constructor #280

mfickers opened this issue Feb 13, 2023 · 4 comments · Fixed by #282 · May be fixed by #294
Assignees
@mikaelcom
Labels
bug

Comments

@mfickers
Copy link

Describe the bug
After upgrading our project from PHP 7.4 to PHP 8.1 this deprecation warning appeared:
Deprecated Functionality: Optional parameter $baz declared before required parameter $quux is implicitly treated as a required parameter.

To Reproduce
This type:

<xs:complexType
    name="Foo">
    <xs:sequence>
        <xs:element name="baz" maxOccurs="1" minOccurs="0" nillable="true"
                    type="xs:string"/>
        <xs:element name="qux" maxOccurs="1" minOccurs="1" nillable="false"
                    type="xs:string"/>
        <xs:element name="quux" maxOccurs="1" minOccurs="1" nillable="true"
                    type="xs:date"/>
        <xs:element name="corge" maxOccurs="1" minOccurs="1" nillable="false"
                    type="xs:string"/>
        <xs:element name="grault" maxOccurs="unbounded" minOccurs="1" nillable="true"
                    type="tns:Bar"/>
    </xs:sequence>
</xs:complexType>

will result in this generated code:

public function __construct(string $qux, string $corge, ?string $baz = null, ?string $quux, ?array $grault)
{
    $this
        ->setQux($qux)
        ->setCorge($corge)
        ->setBaz($baz)
        ->setQuux($quux)
        ->setGrault($grault);
}

It seems that nillable parameters get sorted after non-nillable parameters, but other than that, the order of the XML is kept. Parameters with default values are not ordered last and thus can not be omitted when creating an object of this class.

Expected behavior
Constructor parameters of generated classes should appear in this order:

  • Required parameters that are not nillable
  • Required parameters that are nillable
  • Optional parameters

This is the expected generated code for the given example:

public function __construct(string $qux, string $corge, ?string $quux, ?array $grault, ?string $baz = null)
{
    $this
        ->setQux($qux)
        ->setCorge($corge)
        ->setQuux($quux)
        ->setGrault($grault)
        ->setBaz($baz);
}

Additional context
(https://www.php.net/manual/en/migration80.deprecated.php)
@mfickers mfickers added the bug label Feb 13, 2023
mikaelcom added a commit that referenced this issue Feb 18, 2023
@mikaelcom
issue #280 - put nullable constructor args at the end
51e2040
@mikaelcom mikaelcom linked a pull request Feb 18, 2023 that will close this issue
issue #280 - put nullable constructor args at the end #282
Merged
@mikaelcom
Copy link
Member

Hi @mfickers, I'm trying to improve the way the project is managed.

I created a team for reviewers "only" members so if you accept the invitation, you should be able to review and validate, if it's ok for you, the PR #282.

You can test the PR using one of these phars:

https://phar.wsdltophp.com/wsdltophp-feature-issue-280-php7.phar
https://phar.wsdltophp.com/wsdltophp-feature-issue-280-php81.phar
https://phar.wsdltophp.com/wsdltophp-feature-issue-280-php82.phar

You let me know

@mfickers
Copy link
Author

First of all, thank you for the quick work on that fix.

I've tested the fix with the PHP 8.1 phar from your comment above, but there was no difference in the parameter order.

It seems like the phar does not contain the changes, though:

$ cat wsdltophp-feature-issue-280-php81.phar | grep "function putRequiredAttributesFirst" -A 10
    protected function putRequiredAttributesFirst(StructAttributeContainer $allAttributes): StructAttributeContainer
    {
        $attributes = new StructAttributeContainer($this->getGenerator());
        $requiredAttributes = new StructAttributeContainer($this->getGenerator());
        $notRequiredAttributes = new StructAttributeContainer($this->getGenerator());

        /** @var StructAttribute $attribute */
        foreach ($allAttributes as $attribute) {
            if ($attribute->isRequired() && !$attribute->isNullable()) {
                $requiredAttributes->add($attribute);
            } else {

@mikaelcom
Copy link
Member

mikaelcom commented Feb 20, 2023 •
edited

It seems like the phar does not contain the changes, though:

You're right, I had modiffied my script that generates the phar, which did not check out the branch 🤦‍♂️

Please, download it again now and let me know,

mikaelcom added a commit that referenced this issue Mar 25, 2023
@mikaelcom
issue #280 - put nullable constructor args at the end (#282)
876d3c4 
no news is good news 😄
@lurkker
Copy link

lurkker commented May 8, 2023

This issue remains on version 4.1.8, to address this I reordered these lines in wsdltophp/packagegenerator/src/Model/Struct.php:412
within the function putRequiredAttributesFirst

from:

array_walk($requiredAttributes, [$attributes, 'add']);
array_walk($notRequiredAttributes, [$attributes, 'add']);
array_walk($nullableNotRequiredAttributes, [$attributes, 'add']);

to:

array_walk($requiredAttributes, [$attributes, 'add']);
array_walk($nullableNotRequiredAttributes, [$attributes, 'add']);
array_walk($notRequiredAttributes, [$attributes, 'add']);

There is probably a better way to solve this, but doing this doesn't break functionality and removes the deprecation warnings.

Thank you for the library.

superbiche added a commit to superbiche/WsdlToPhpPackageGenerator that referenced this issue Jun 20, 2023
@superbiche
WsdlToPhp#280: workaround wrong order of optional/nullable parameters
7cfa2da
@superbiche superbiche mentioned this issue Jun 20, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mikaelcom mikaelcom

Labels
bug
Projects
None yet
Milestone
No milestone
3 participants
@mikaelcom @mfickers @lurkker