soap-client icon indicating copy to clipboard operation
soap-client copied to clipboard

Support laminas code ^4.5.0

Open lstrojny opened this issue 1 year ago • 3 comments

It’s not a great change but … hear me out.

Magento requires these versions of laminas-code:

Magento Version laminas-code version
2.4.4 ~4.5.0
2.4.6 ^4.5.0
2.4.7 ^4.13

This means: phpro/soap-client cannot be installed with Magento as it will create version conflicts. The simplest way to fix that would be to tolerate lower versions of laminas and do feature detection. This is what this patch does. If PropertyGenerator from laminas-code does not support setting type information, it will just be skipped.

lstrojny avatar Jul 03 '24 14:07 lstrojny

I'm a bit confused.. 4.14 is in those dependency ranges right?

veewee avatar Jul 03 '24 15:07 veewee

Not in the one for 2.4.4, which was released a bit more than a year ago

lstrojny avatar Jul 03 '24 15:07 lstrojny

Since 4.x is a new major release, it might make more sense to keep it as is. Those on older Magento installations van keep on using an older soap-client version until they have the chance to upgrade their Magento installations. I'm not too keen on adding hacks like this to the project from a maintainability point of view. It already is a year old to be fair...

Maybe a small question from me: were you able to play around with the V4 alpha version? Would love to get some early feedback rather than launching it directly and having to fix bugs under pressure :-)

veewee avatar Jul 03 '24 16:07 veewee

I thought a bit more about the issue and I could see another path: what if code generation and runtime would be separate, so that a library can require the code generation functionality through require-dev and the runtime through require. The dependency tree for require could look like this:

  • phpro/soap-client
    • depends on:
      • phpro/soap-symfony (new package, name TBD) ‼️
        • includes the PhpPro\SoapClient\Event\Subscriber namespace
        • depends on:
          • symfony/event-dispatcher
          • symfony/validator
          • phpro/soap-runtime (new package, name TBD) ‼️
            • includes these namespace: Phpro\SoapClient\Caller, Phpro\SoapClient\Event (but not subscribers), Phpro\SoapClient\Soap\Metadata (but not detectors or manipulators)
            • depends on:
              • php-soap/cached-engine
              • php-soap/engine
              • php-soap/encoding
              • php-soap/psr18-transport
              • php-soap/wsdl-reader
              • psr/event-dispatcher
      • phpro/soap-code-generation (new package, name TBD) ‼️
        • includes the rest of the code from phpro/soap-client
        • depends on:
          • php-soap/encoding
          • php-soap/wsdl-reader
          • laminas/code-generator
          • symfony/console
          • symfony/filesystem

I am sure I got some dependency slightly wrong, but the result would be that libraries using phpro/soap-client can only depend on phpro/soap-runtime and get minimal dependencies (no Symfony, no Laminas) and therefore reducing the likelihood of conflict. The result would also be that existing implementation would continue to depend on phpro/soap-client and that would continue to work transparently.

What do you think?

lstrojny avatar Jul 09 '24 12:07 lstrojny

@lstrojny I've thought about that as well and it could make sense to do so. Yet it's again more package(s) to the chain.

The only thing this package really does is code generation and providing a small 'caller' system for runtime. These packages here have evolved a lot over the past few years. It might make sense to take a step back and see if they are still grouped logically. For example, the caller system could become part of the php-soap/engine or php-soap/client so that it can be reused. Not sure if the current event subscribers are actually being used. They are very simple so can always be implemented downstream or provided as documentation.

I'm also not sure how splitting it up would solve your initial problem? You'dd still need to require the code generator package into your dev dependencies, making it still collapse with the laminas code package.

veewee avatar Jul 09 '24 12:07 veewee

These packages here have evolved a lot over the past few years. It might make sense to take a step back and see if they are still grouped logically. For example, the caller system could become part of the php-soap/engine or php-soap/client so that it can be reused. Not sure if the current event subscribers are actually being used. They are very simple so can always be implemented downstream or provided as documentation.

That would totally work as well, yes.

I'm also not sure how splitting it up would solve your initial problem? You'dd still need to require the code generator package into your dev dependencies, making it still collapse with the laminas code package.

Because it’s only in require-dev, the code generation dependencies are irrelevant for the resolval of the overall tree. So the structure would practically be something like Magento -> My Library -> SOAP runtime (require) and SOAP code generation (require-dev). Therefore when Magento builds it’s dependency tree, dev dependencies from My Library are ignored.

lstrojny avatar Jul 09 '24 12:07 lstrojny

To come back to the original pull request: I understand you correctly that there is no interest from your side to get it merged?

lstrojny avatar Oct 16 '24 10:10 lstrojny

@lstrojny Indeed : there currently is no interest in merging this PR. I've kept it open to keep track of the feedback provided to make this package a dev-only package.

As mentioned above : Since v4 will be a new major release, I don't see much value in lowering the dependencies atm: You can either upgrade magento (given support for 2.4.4 will be dropped in a half year) or use an older version of this package to overcome the issue. Nevertheless thank you for your insight and work on this.

veewee avatar Oct 17 '24 11:10 veewee