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

Support laminas code ^4.5.0 #530

Closed
wants to merge 1 commit into from

Conversation

lstrojny
Copy link
Contributor

@lstrojny lstrojny commented Jul 3, 2024

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 lstrojny force-pushed the dev/widen-laminas-code-support branch 2 times, most recently from f10e13c to fa15434 Compare July 3, 2024 14:54
@lstrojny lstrojny force-pushed the dev/widen-laminas-code-support branch from fa15434 to 4a64771 Compare July 3, 2024 15:08
@veewee
Copy link
Contributor

veewee commented Jul 3, 2024

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

@lstrojny
Copy link
Contributor Author

lstrojny commented Jul 3, 2024

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

@veewee
Copy link
Contributor

veewee commented Jul 3, 2024

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

@lstrojny
Copy link
Contributor Author

lstrojny commented Jul 9, 2024

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?

@veewee
Copy link
Contributor

veewee commented Jul 9, 2024

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

@lstrojny
Copy link
Contributor Author

lstrojny commented Jul 9, 2024

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

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

@veewee
Copy link
Contributor

veewee commented Oct 17, 2024

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

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

Successfully merging this pull request may close these issues.

2 participants