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

[hubipe]

Hi, soo any resolution to this? This still keeping me from upgrading to PHP 8.4:

[baywet]

@hubipe thanks for the nudge here.
Sorry if I wasn't clear, no-one at Microsoft is currently working on this. I was operating under the assumption that you were.
Here are our options I think:

1. add generic promises to the upstream dependency, but as a derived type instead of modifying the existing type.
2. create a generic promise type in abstractions here, that derives from the upstream dependency promise to avoid breaking changes.
3. use stub files in the abstractions library, only if using those stubs DOES NOT require gaph sdk consumers to make any configuration changes.
4. the "do not upgrade the dependency" is not an acceptable solution since it's the root of the problem we're trying to solve, dropping the dependency would most likely mean a breaking change, and changing the API surface from async to sync is also not viable as it represents a breaking change

With that context in mind, do you have enough information to continue the work?

[julianschelker]

Any possibility to get this done even with breaking changes? Long term not updating and letting the deprecated php-http/promise dependency stay also starts causing interruption to consumers as it starts breaking dependency chains and users not being able to update. See #45.

E.g. several affected libraries which can't be updated used anymore with this deprecation:
microsoft/microsoft-graph
microsoft/microsoft-graph-core
microsoft/kiota-authentication-phpleague
microsoft/kiota-http-guzzle

[baywet]

@julianschelker I think this would need somebody from the community to step up and fix at this point. Either the OP or yourself. Have a look at the conversation and let us know if you have any questions.

[inserve-paul]

Any updates on this issue?

[hubipe]

I have created a pull request to the php-http/promise library last week – php-http/promise#36 which would make promise library compatible with PHP 8.4 in the 1.2 branch.

[inserve-paul]

@hubipe Great work, hopefully it will be released soon

[hubipe]

The php-http/promise#36 has been merged and the new version 1.2.2 released. So I think, this MR could be closed and the rest of the Microsoft libraries waiting for this MR to be resolved could be finally merged?
It seems, that support for PHP 8.4 for the Microsoft graph could be coming. Yayyy!

[baywet]

We should remove the author here

[baywet]

Same comment about authors

[baywet]

Same comment for authors

[baywet]

@hubipe thank you for contributing there.
I'm a little confused by your suggestion to close this MR? Don't we need the generic promises definitions it adds so everything lines up nicely from a static type checking perspective?

[hubipe]

@hubipe thank you for contributing there. I'm a little confused by your suggestion to close this MR? Don't we need the generic promises definitions it adds so everything lines up nicely from a static type checking perspective?

The php-http/promise package in the 1.2.* branch contains the generics. See, the promise library had version 1.1. Then @Ndiritu (I think it was him) added the generics, and the 1.2 version has been released. It caused some problems, so they reverted it in the 1.3 version. They added support for PHP 8.4 in the 1.3 branch, but not to the 1.2 branch. The MR mentioned (and now merged as version 1.2.2) added support for PHP 8.4 on a branch with generics in it.
So this MR is obsolete – it is not needed anymore, nothing has to be changed and this library depending on the 1.2.* version is still valid. Therefore I suggest to close this MR without a merge.

[baywet]

Thank you for the additional information.

So effectively kiota & microsoft graph clients will not be on the latest version of this dependency, but will have the compatibility with PHP 8.4, correct?

[hubipe]

@baywet Correct. Kiota dependency on php-http/promise will stay set to ~1.2.0, Whoever updates php-http/promise dependency to 1.2.2 will not have emmited deprecation warnings in PHP 8.4.
Updating dependency to 1.3 of the php-http/promise library is a no-go, as the Microsoft Graph client requires generics presented in the 1.2 version of the promise library. 1.3 release reverted this addition (why exactly I'm not sure, I believe your comment here microsoft/kiota-abstractions-php#159 (comment) contains more info)

[baywet]

Thank you for the additional information.

The two main downsides I see here being:

• If/when PHP9 is added, the authors will need to go back to that older version to mark it as supporting that version of PHP (nothing guarantees they will)
• If the consumers project already depends on a newer version of php promises than the one we lock, they'll run into issues.

The long term fix (assuming php promises doesn't make generic promises available in their mainline) is probably to unpin the version, and come up with our own generic promises definitions, which this PR was doing. But we can address that whenever people run into issues.

Does that make sense?

(I want to make sure I document the conclusion correctly, this way whenever those issues arise, we'll have context to come back to)

[hubipe]

@baywet

...and come up with our own generic promises definitions, which this PR was doing.

I beg to differ. I opened this MR solely to allow installing PHP 8.4 on a project using microsoft/microsoft-graph. I sent a MR and then went the rabbit hole adding multiple new MR across many microsoft libraries doing the same thing – fixing compatibility with PHP 8.4. I specifically opened this MR to allow installing php-http/promise 1.3.0, because this library depends on it and at the time, the 1.3 version was compatible with PHP 8.4.

After submitting a patch, the PHPStan tests started to failing. It was then, when I realized, why this library is locked to php-http/promise 1.2, not allowing installing 1.3. Making own promises started with your comment from 7th April. I tried to implement the very own Promise classes, but run into errors with PHPStan, I couldn't solve.

I agree, that locking this library to the 1.2 is a bad situation and agree, that making own Promises would solve it properly. I just can not agree with the claim, that this PR was about adding own Promises to the library.

---

If you still think, we should continue trying adding own Promises to the library, so we could unlock the version constraint, I could try to revisit the PHPStan errors and try to figure out, if the problem still persists (after all, it is more than a half year later - meaning I have another half a year more experience with PHPStan and there has been multiple new versions of PHPStan since then).
I mean, lot of work has been done on this patch... But at the same time, this is (I think) a blocker for release of another Microsoft libraries to be compatible with PHP 8.4, which I'd like to see in production, since the underlying problem with compatibility has been fixed upstream.

[baywet]

Thank you for the additional information.

I just can not agree with the claim, that this PR was about adding own Promises to the library.

Sorry, I jumped from the conclusion of "we need to unpin the version, and to do so implement our own promises definitions" to "this is what this PR meant to do". When in fact adding promises in this PR was just a mean to get 8.4 support, which was better achieved by patching the promise dependency.

If we focus on the original issue (PHP 8.4 getting a compat warning during install)

• microsoft graph 2.49.0 (latest): advertises PHP ^7.4 || ^8.0, depends on graph core ^2.2.1
• graph core 2.3.1 (latest): advertises PHP ^7.4 || ^8.0, depends on kiota libs ^1.5.0
• kiota auth phpleague 1.5.1 (latest): advertises PHP ^7.4 || ^8.0, depends on kiota abstractions ^1.5.1
• kiota http guzzle 1.5.1 (latest): advertises PHP ^7.4 || ^8.0, depends on kiota abstractions ^1.5.1
• kiota serialization form 1.5.1 (latest): advertises PHP ^7.4 || ^8.0, depends on kiota abstractions ^1.5.1
• kiota serialization json 1.5.1 (latest): advertises PHP ^7.4 || ^8.0, depends on kiota abstractions ^1.5.1
• kiota serialization multipart 1.5.1 (latest): advertises PHP ^7.4 || ^8.0, depends on kiota abstractions ^1.5.1
• kiota serialization text 1.5.1 (latest): advertises PHP ^7.4 || ^8.0, depends on kiota abstractions ^1.5.1
• kiota abstractions 1.5.1 (latest) : advertises PHP ^7.4 || ^8.0, depends on promises ~1.2.0

So unless another version is being pulled by the consuming project, or a third party dependency of the packages listed above, the end user should not receive a warning about PHP 8.4 compatibility anymore. Could you please confirm that?

As for the risk posed by having a version pinned, which would lead to conflicts in consumer projects, or version 1.2.X of promises getting stale/posing a security risk, this is something that can be addressed at a later stage. Of course if you want to get ahead of the curve here and finalize this PR, which contains most of what's needed to address this risk, we'll be more than happy to take the contribution in.

Does that make sense?