PackageGenerator icon indicating copy to clipboard operation
PackageGenerator copied to clipboard

Generated classes/files enhancements

Open mikaelcom opened this issue 5 years ago • 9 comments

Tasks:

  • [ ] Group use statements on top of the class for every used class not in the same namespace
  • [ ] Use short name for classes in the same namespace
  • [ ] Add use InvalidArgumentException statement only if it's used
  • [ ] Add ext-dom:* to composer.json only if xml validation rule is used or if the DOMDocument class is used within the generated class
  • [ ] Remove validation rules relative to native PHP types as method parameter is typed
  • [ ] Rethink getResult and setResult usages within ServiceType classses by returning null instead of bool in order to type-hint method and throw an exception if necessary
  • [ ] Refactor files/classes code generation
  • [ ] In the generated ClassMap, it would be much nicer to use MyType::class instead of '\MyType'. To be tested
  • [ ] Improve validation rule generation by UnionRule in order to match StyleCI (see https://github.com/WsdlToPhp/PackageEws365/commit/f7cb291b646e019c3015a641a1f4f5387cfcc91d and https://github.com/WsdlToPhp/PackageEws365/commit/49631e95b3408b323b3786d3a8f1593e38d6aff7#diff-302f453a999740b20b4456f8a14ee56d561ae75ea578a341e5bb5b73fabb65ebR195-R197)
  • [ ] Improve perfs according to https://github.com/WsdlToPhp/PackageGenerator/discussions/253

mikaelcom avatar Feb 06 '21 15:02 mikaelcom

In the generated ClassMap, it would be much nicer to use MyType::class instead of '\\MyType'.

sprankhub avatar Feb 26 '21 13:02 sprankhub

WSDL:

<wsdl:operation name="SomePing">
    <soap:operation soapAction="urn:WebService-Shop#SomePing" style="rpc"/>
    <wsdl:input>
        <soap:body use="literal" encodingStyle="http://schemas.xmlsoap.org/soap/encoding/" namespace="urn:WebService-Shop"/>
    </wsdl:input>
    <wsdl:output>
        <soap:body use="literal" encodingStyle="http://schemas.xmlsoap.org/soap/encoding/" namespace="urn:WebService-Shop"/>
    </wsdl:output>
</wsdl:operation>

This leads to broken code:

$this->setResult($resultSomePing = $this->getSoapClient()->__soapCall('SomePing', ], [], [], $this->outputHeaders));

Not sure if this only happens in the new version or also in the old one.

sprankhub avatar Feb 26 '21 14:02 sprankhub

Constructors of child classes miss parent constructor calls. Given the following hierarchy:

class ShopRequest extends AbstractStructBase

class SomeRequestType extends ShopRequest

The generated constructor of SomeRequestType does not call the constructor of ShopRequest and also does not include the parent properties.

Not sure if this only happens in the new version or also in the old one.

sprankhub avatar Feb 26 '21 15:02 sprankhub

Currently, service classes return either a type or a bool, which does not let us use a return type. I suggest throwing an exception in case of any errors or at least return null, so that we can use return type hints.

Sorry for just posting this here, but I am currently unsure if these issues belong to the old or the new or both versions and I do not know how you would like to structure your work. Tell me if I should better create new issues :)

sprankhub avatar Feb 26 '21 15:02 sprankhub

No worries, this issue is perfectly suited to gather future enhancements, wether they are old issues or not.

mikaelcom avatar Feb 26 '21 19:02 mikaelcom

WSDL:

<wsdl:operation name="SomePing">
    <soap:operation soapAction="urn:WebService-Shop#SomePing" style="rpc"/>
    <wsdl:input>
        <soap:body use="literal" encodingStyle="http://schemas.xmlsoap.org/soap/encoding/" namespace="urn:WebService-Shop"/>
    </wsdl:input>
    <wsdl:output>
        <soap:body use="literal" encodingStyle="http://schemas.xmlsoap.org/soap/encoding/" namespace="urn:WebService-Shop"/>
    </wsdl:output>
</wsdl:operation>

This leads to broken code:

$this->setResult($resultSomePing = $this->getSoapClient()->__soapCall('SomePing', ], [], [], $this->outputHeaders));

Not sure if this only happens in the new version or also in the old one.

It has to be fixed within https://github.com/WsdlToPhp/PackageGenerator/pull/234. If I understand well, this is the case when the Soap operation does not require any parameter, isn't?

mikaelcom avatar Mar 01 '21 16:03 mikaelcom

Constructors of child classes miss parent constructor calls. Given the following hierarchy:

class ShopRequest extends AbstractStructBase

class SomeRequestType extends ShopRequest

The generated constructor of SomeRequestType does not call the constructor of ShopRequest and also does not include the parent properties.

Not sure if this only happens in the new version or also in the old one.

This is the default behaviour.

In my point of view, adding the inherited constructor arguments to the child classes constructor is not necessarily useful because:

  • If the child class has its own properties, the constructor contains its properties as arguments. The parent class(es) constructor arguments would be added at the end. Instantiating such child class would be fat to write
  • In addition the previous point, if the parent class has more than 10 arguments in the constructor, if you need to set the last argument value, you'll have to set each previous arguments.

Most of the time, arguments are optional, so I usually use the fluent way:

$struct = (new Struct())
    ->setValue($value)
    ->setAny($any)
;

Or even the __set_state method:

$struct = Struct::__set_state([
    'value' => $value,
    'any'   => $any,
]);

Nevertheless, calling the parent constructor without any argument could be added in order to handle removable properties. This is feasible only if there is no required arguments unless they are also added to the child constructor arguments at first position with its own required arguments.

Let me know your thoughts about this.

mikaelcom avatar Mar 01 '21 16:03 mikaelcom

In the generated ClassMap, it would be much nicer to use MyType::class instead of '\\MyType'.

To be tested as there is a time classmap required fully qualified class name with the first \.

mikaelcom avatar Mar 01 '21 16:03 mikaelcom

Thanks @mikaelcom!

It has to be fixed within #234. If I understand well, this is the case when the Soap operation does not require any parameter, isn't?

Most probably yes :)

Constructors of child classes miss parent constructor calls.

If most of the properties are optional, I agree. However, in my case, nearly all are required. And if this is the case, it is easy to miss required parameters if the child constructor does not also include the parent properties. However, this of course depends on the use case and is completely opinionated, so feel free to ignore that suggestion :)

To be tested as there is a time classmap required fully qualified class name with the first .

Sounds strange. I can only imagine this happened in really old PHP versions. However, since we now target PHP >= 7.4, I do not think that this leads to issues.

sprankhub avatar Mar 01 '21 19:03 sprankhub