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

Generated classes/files enhancements #233

Open
10 tasks
mikaelcom opened this issue Feb 6, 2021 · 9 comments
Open
10 tasks

Generated classes/files enhancements #233

mikaelcom opened this issue Feb 6, 2021 · 9 comments
Assignees

Comments

@mikaelcom
Copy link
Member

mikaelcom commented Feb 6, 2021

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 WsdlToPhp/PackageEws365@f7cb291 and WsdlToPhp/PackageEws365@49631e9#diff-302f453a999740b20b4456f8a14ee56d561ae75ea578a341e5bb5b73fabb65ebR195-R197)
  • Improve perfs according to while generating the classes: use all cpu cores/threads #253
@mikaelcom mikaelcom self-assigned this Feb 6, 2021
@mikaelcom mikaelcom changed the title Generated classes enhancements Generated classes/files enhancements Feb 8, 2021
@sprankhub
Copy link

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

@sprankhub
Copy link

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
Copy link

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
Copy link

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 :)

@mikaelcom
Copy link
Member Author

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

@mikaelcom
Copy link
Member Author

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 #234. If I understand well, this is the case when the Soap operation does not require any parameter, isn't?

@mikaelcom
Copy link
Member Author

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
Copy link
Member Author

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

@sprankhub
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants