WsdlToPhp / PackageGenerator

Generates a PHP SDK based on a WSDL, simple and powerful, WSDL to PHP
https://providr.io
MIT License
428 stars 73 forks source link

Inherited struct methods should not be overwritten #217

Closed astehlik closed 3 years ago

astehlik commented 4 years ago

We currently have an issue with the following webservice definition:

https://gold.datgroup.com/DATECodeSelection/services/VehicleSelectionService?wsdl

I extracted the problematic parts in a minimal WDSL to show the problem caused by the complexType / simpleContent / extension construct.

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<definitions xmlns:xs="http://www.w3.org/2001/XMLSchema"
             xmlns:tns="http://sphinx.dat.de/services/VehicleSelectionService"
             xmlns:soap="http://schemas.xmlsoap.org/wsdl/soap/"
             xmlns:wsam="http://www.w3.org/2007/05/addressing/metadata"
             xmlns:xsd="http://www.w3.org/2001/XMLSchema"
             xmlns="http://schemas.xmlsoap.org/wsdl/"
             targetNamespace="http://sphinx.dat.de/services/VehicleSelectionService" name="VehicleSelectionService">
    <types>
        <xsd:schema targetNamespace="http://sphinx.dat.de/services/VehicleSelectionService"
                    xmlns="http://www.w3.org/2000/10/XMLSchema">
            <xs:complexType name="fieldString">
                <xs:simpleContent>
                    <xs:extension base="xs:string">
                        <xs:attribute name="nil" type="xs:boolean"/>
                        <xs:attribute name="overwrite" type="xs:boolean"/>
                        <xs:attribute name="origin" type="xs:string"/>
                    </xs:extension>
                </xs:simpleContent>
            </xs:complexType>
            <xs:complexType name="fieldString1000">
                <xs:simpleContent>
                    <xs:extension base="tns:fieldString"/>
                </xs:simpleContent>
            </xs:complexType>
            <xs:complexType name="getVersion">
                <xs:sequence>
                    <xs:element name="request" type="tns:fieldString1000" minOccurs="0"/>
                </xs:sequence>
            </xs:complexType>
            <xs:complexType name="getVersionResponse">
                <xs:sequence>
                    <xs:element name="version" type="xs:string" minOccurs="0"/>
                </xs:sequence>
            </xs:complexType>
            <xs:element name="getVersion" type="tns:getVersion"/>
            <xs:element name="getVersionResponse" type="tns:getVersionResponse"/>
        </xsd:schema>
    </types>
    <message name="getVersion">
        <part name="parameters" element="tns:getVersion"/>
    </message>
    <message name="getVersionResponse">
        <part name="parameters" element="tns:getVersionResponse"/>
    </message>
    <portType name="VehicleSelectionService">
        <operation name="getVersion">
            <input wsam:Action="getVersion" message="tns:getVersion"/>
            <output wsam:Action="http://sphinx.dat.de/services/VehicleSelectionService/VehicleSelectionService/getVersionResponse"
                    message="tns:getVersionResponse"/>
        </operation>
    </portType>
    <binding name="VehicleSelectionServicePortBinding" type="tns:VehicleSelectionService">
        <soap:binding transport="http://schemas.xmlsoap.org/soap/http" style="document"/>
        <operation name="getVersion">
            <soap:operation soapAction="getVersion"/>
            <input>
                <soap:body use="literal"/>
            </input>
            <output>
                <soap:body use="literal"/>
            </output>
        </operation>
    </binding>
    <service name="VehicleSelectionService">
        <port name="VehicleSelectionServicePort" binding="tns:VehicleSelectionServicePortBinding">
            <soap:address location="https://gold.datgroup.com/DATECodeSelection/services/VehicleSelectionService"/>
        </port>
    </service>
</definitions>

This will generate two fieldString struct classes with inheritance (which is correct):

class FieldString
}

class FieldString1000 extends FieldString
...
}

Here is where the problem occurs: The parent class has a setter method for the $_ property which accepts a string:

/** ... @param string $_ ... */
public function set_($_ = null)
{
    ...
}

The child class also has the $_ property which for some reason has the type of the parent struct:

public function set_(FieldString $_ = null)
{
    ...
}

This method will cause PHP warnings:

PHP Warning: Declaration of FieldString1000::set(FieldString $ = null) should be compatible with FieldString::set($ = null)

I created a functioning workaround by removing the inherited properties from the child class (which makes sense IMO) but I'm unsure if this is correct:

https://github.com/Intera/PackageGenerator/commit/e912af7d26e1ef8169fe2487422742dc84b83820

I'm happy to help out with the solution if you can point me to the right direction :)

Cheers, Alex

mikaelcom commented 4 years ago

What I could find out is that the native SoapClient class returns these types:

[183] => struct fieldString {
 string _;
 boolean nil;
 boolean overwrite;
 string origin;
}
    [184] => struct fieldString1000 {
 fieldString _;
}
    [185] => struct fieldString2 {
 fieldString _;
}
    [186] => struct fieldString30 {
 fieldString _;
}
    [187] => struct fieldString5 {
 fieldString _;
}
    [188] => struct fieldString10 {
 fieldString _;
}
    [189] => struct fieldString4 {
 fieldString _;
}
    [190] => struct fieldString8 {
 fieldString _;
}
    [191] => struct fieldCharacter {
 fieldString _;
}
    [192] => struct fieldString9 {
 fieldString _;
}
    [193] => struct fieldString6 {
 fieldString _;
}
    [194] => struct fieldString3 {
 fieldString _;
}
    [195] => struct fieldString17 {
 fieldString _;
}
    [196] => struct fieldString15 {
 fieldString _;
}
    [197] => struct fieldString4000 {
 fieldString _;
}
    [198] => struct fieldString80 {
 fieldString _;
}
    [199] => struct fieldString40 {
 fieldString _;
}
    [200] => struct fieldString50 {
 fieldString _;
}
    [201] => struct fieldString12 {
 fieldString _;
}
    [202] => struct fieldString20 {
 fieldString _;
}
    [203] => struct fieldString100 {
 fieldString _;
}
    [204] => struct fieldString60 {
 fieldString _;
}

As you can see, it is declared that each fieldString* struct contains the _ property of type fieldString, the very first one struct.

This is why you end up with such classes.

IMHO, the current usage of the fieldString* structs is not very useful as they don't define any additional information from the base fieldString.

I currently don't see any improvement that would work around this type of issue.

Let me know what you think.

astehlik commented 4 years ago

Thank you for the quick feedback.

I agree that these fieldString* structs do not make much sense. Unfortunately the service is a third party product and we have no influence on the provided WSDL / data structure.

I tested the creation of the webservice client with wsdl2java of Apache CXF and this is generated:

public class FieldString {
    @XmlValue
    protected String value;
    ...
}

public class FieldString9 extends FieldString
{
}

As far as I understand it the value property is the equivalent of the underscore _ property in PHP. And it is only present in the parent class.

So I guess removing the property in the child classes is the way to go.

There might be an edge case when there are additional constraints for the string provided in the child types. Then you would need a setter set_() in the child class that validates those constraints. But then the setter should still be for the type string and not for the parent class as it is at the moment.

I hope this makes any sense :)

jsakars commented 3 years ago

We are experiencing the same problem

mikaelcom commented 3 years ago

I presume the duplicated property and the duplicated setter/getter in the child classes could be removed meaning the parent classes would force the parameter type for every child class which could be restrictive if wrongly defined in the WSDL.

I must check amongst the existing WSDL in the unit tests in order to see if it would change anything or if it would finally be a rare case (such as yours) and thus not so impactful.

Let me know if you have any additional thought :wink: