Require php-http/promise 1.3.0 to be compatible with PHP 8.4

[hubipe]

php-http/promise version 1.3.1 fixed compatibility issues with PHP 8.4. This pull request updates composer.json to allow installing this version of the php-http/promise library

[baywet]

Thanks for the contribution!

[baywet]

@hubipe as you can see we're getting quite a few failures in the CI. Here is a bit of history: we (the kiota team) originally contributed to the promise library to make it generic. But they decided to revert that change because it was breaking in some scenarios. So we've kept back the version of the library over time. @Ndiritu probably has more context here.

I do have a few questions for you:

• is the promise upgrade required for PHP 8.4 support? why?
• Is there a library that builds on this one and makes it generic?
• Do the library owners have any plans to bring genericity back?

Thanks!

[hubipe]

@baywet
The problem is, that the php-http/promise 1.2.0 version is not compatible with the PHP 8.4. The compatibility change was made in the 1.3.1 release. The same change could possibly be backported to the 1.2 so even 1.2 version would be compatible with PHP 8.4.
But until either of these would be done, the PHP 8.4 version will raise deprecation notices about implicit nullable parameters (more info about it here at php.watch).

As for your questions:

• is the promise upgrade required for PHP 8.4 support? why? – Yes, because of the implicit nullable parameters in some of the methods
• Is there a library that builds on this one and makes it generic? – I honestly don't know. I don't understand much why the promise library added generics in 1.2 version and then removed it in the 1.3 version.
• Do the library owners have any plans to bring genericity back? – Also don't know, I'm neither contributor nor owner of the promise library.

[baywet]

Thank you for the additional information.
I was able to find the reverting PR here php-http/promise#33
Do you want to engage there? Maybe there a way to bring back the genericity without breaking existing users.

[hubipe]

@baywet To be honest I don't. I don't use php-http/promise nor have any experience with the generics for PHPStan.
If you're dealing with generics annotations for PHPstan only, maybe the better approach since the owner of the library can't solve any particular problems in it's library would be to use PHPStan stub files, where you could overwrite (wrong) annotations by correct annotations only for the PHPStan analysis?

[baywet]

Thank you for the additional information.

The solution should not require anybody who has a kiota client to change their project configuration. So if the stubs can ship with this library as well, we can go ahead.

As an alternative, what about creating a specialized type in the promise library, like generic promise, which would derive from the existing type?

[baywet]

gentle reminder on this @hubipe to wrap things up.

Alternatively, what's the biggest issue with NOT upgrading the dependency? People simply get a warning? But they can still install all kiota and microsoft graph packages?

[hubipe]

@baywet Thanks for the reminder, but I don't know how to wrap it up.

1. If we leave php-http/promise version in the 1.2 branch, the PHPstan will be satisfied, but the inherited dependency will cause, that anyone who is using library microsoft/microsoft-graph on PHP 8.4 will get deprecation warnings (and possibly in PHP 8.5 it won't work at all). There is a possibility to try to make a new pull request to address incompatibility in the php-http/promise library and make a new 1.2.2 release.
2. Or we can remove generics from this library and keep dependency on the 1.3 branch. That would make PHPStan happy and no functionallity would change. But I have no idea, if changing the PHPDocs wouldn't break anything further down the path.
3. Another option would be to ignore PHPStan errors either by muting them in the code or in the phpstan.config file.
4. Or lastly this library could incorporate PHPStan stub files.

Which option would be best to wrap this PR up?

[baywet]

Thank you for the additional information.

Any solution that'd require consumers to update their PHP stan configuration wouldn't be acceptable. So would any solution that leads to sources breaking changes (even at linting/doc since it'd be disruptive). I believe this rules out 2, 3 and 4 from your last response.

What about trying this:

1. create a "async promise" type here, which derives from the promise in the promise lib
2. update the dependency
3. update all references here from promise to async promise.
4. (hold the merge)

We can then revert to the promise library to see whether they have any appetite to get this async promise as a contribution, or simply roll with that here if they push back. This should not be source breaking for anybody. Thoughts?

[hubipe]

@baywet Do I understand correctly, that I should create new Promise interface in this library, which should be extending Promise interface from the php-http/promise library and replace the return types to our Promise interface?

[baywet]

@baywet Do I understand correctly, that I should create new Promise interface in this library, which should be extending Promise interface from the php-http/promise library and replace the return types to our Promise interface?

that's what I'm suggesting yes.

[hubipe]

@baywet Will do on Thursday.

[hubipe]

@baywet Sooo, I tried to implement the new Microsoft\Kiota\Http\Promise interface in the library, which extends the Http\Promise\Promise interface. But I run into the problem with the classess FulfilledPromise and RejectedPromise, as these classes are marked as final, so I can't extend them by custom implementation. That means, that we have to replace FulfilledPromise and RejectedPromise also with our implementation.

Although I copied the FulfilledPromise and RejectedPromise classes from the Http\Promise library as they were in the version 1.2.1, I couldn't make PHPStan stop complaining about the generics:

 ------ -------------------------------------------------------------------------
  Line   http/promise/src/FulfilledPromise.php
 ------ -------------------------------------------------------------------------
  52     Method Microsoft\Kiota\Http\FulfilledPromise::then() should return
         Microsoft\Kiota\Http\Promise<Exception>|Microsoft\Kiota\Http\Promise<V>
         but returns Microsoft\Kiota\Http\RejectedPromise<mixed>.
         🪪  return.type
 ------ -------------------------------------------------------------------------

 ------ -------------------------------------------------------------------
  Line   http/promise/src/RejectedPromise.php
 ------ -------------------------------------------------------------------
  39     Method Microsoft\Kiota\Http\RejectedPromise::then() should return
         Microsoft\Kiota\Http\Promise<V> but returns
         Microsoft\Kiota\Http\RejectedPromise<mixed>.
         🪪  return.type
 ------ -------------------------------------------------------------------

Maybe would anyone know, how to handle this error?

[baywet]

Thanks for the additional work here. Is there any specific reason why they are final?

[hubipe]

Thanks for the additional work here. Is there any specific reason why they are final?

I don't think so, they are final in the php-http/promise library, so I copied them as is.

[hubipe]

I removed the final keywords from the Fulfilled and Rejected promises

[Ndiritu]

@baywet @hubipe

I think re-implementing the promise types would effectively mean we no longer need a dependency on php-http/promise which leaves us always chasing their implementation. Since we exposing the Promise type in Kiota-generated API clients like the Graph SDK - example, this could potentially be a breaking change.

The goal of the generics effort was to improve the auto-completion experience with API clients so that we have IDEs suggest relevant methods/properties on the value a returned promise resolves to. Without this, the developer would need to check API docs/method docs to know the return type & add PHPDocs on their side for better auto-completion e.g.

/** @var UserCollectionResponse|null **/
$users = $graphServiceClient->users()->get()->wait();
$users->... // resolves to relevant auto-completion

I'd suggest exploring:

• A patch for php-http/promise 1.2.x as a quicker fix before a long-term solution to generic support is added. I've requested this
• PHPStan Stub files could be something to explore
• Worst case, evaluating work-arounds/impact of "breaking" the auto-completion experience by bumping to 1.3.x until there's a stable annotations implementation
• Longer term, we should probably not have gone for an async-first pattern and perhaps in a major rev we should return the actual objects.

[hubipe]

@Ndiritu

I think re-implementing the promise types would effectively mean we no longer need a dependency on php-http/promise

Agree. The interface Microsoft\Kiota\Abstraction\Promise\Promise extends Http\Promise\Promise only to be backwards compatible. If the interface wouldn't extend Http\Promise\Promise interface, then you could remove the php-http/promise dependency, but that definitely would be a breaking change.

PHPStan Stub files could be something to explore

Stub files works only for PHPStan check in the library. It effectively replace the method signatures and annotation with those in the stub files, but only for the PHPStan check. That means, that these stub files would need every user's implementation, if they're using PHPStan for their libraries. That's because this library disclose Promise as a return type in the Promise::then() interface.

Anyhow, what next?

[Ndiritu]

@hubipe
Sorry, I missed that it extends php-http's Promise type. Thanks for the clarity on the stubs.
If we can't patch the php-http lib then perhaps this is the best we can do for now until we do a major rev.

[Ndiritu]

@baywet this approach seems like the best compromise if we can't patch php-http. We'd need to change the generator namespace as well. A major rev, probably should drop the async approach. Thoughts?

[baywet]

Thanks for joining here. To recap for my understanding:

• The best approach would be to patch the upstream lib, but we've had mitigated success. Do we know what's blocking them from accepting the generic changes for good?
• Short of that, we could upgrade to latest (non generic) + ship PHP stan stub files. Can you confirm this would break CI/linting for consumers? and NOT require configuration changes for consumers? And NOT impact the autocompletion experience?
• If stubs don't work as expected, we could explore implementing our own promises. Wouldn't this change be source breaking since the return type of request adapter would technically change? (unless we derive from the existing promises)
• An alternative would be to NOT upgrade the package, let people get warnings on PHP 8.4, potentially errors in later versions of PHP.
• Another alternative would be to remove async all together, which would be a major breaking changes.

Given our current funding situation, any breaking change solution is too much of a cost for us to bear as it requires coordination between the generator, packages for multiple languages, etc...)