-
Notifications
You must be signed in to change notification settings - Fork 8
Replace abandoned packages #47
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #47 +/- ##
============================================
+ Coverage 61.75% 64.17% +2.41%
+ Complexity 466 459 -7
============================================
Files 62 62
Lines 1561 1552 -9
============================================
+ Hits 964 996 +32
+ Misses 597 556 -41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi @cooperaj, thanks! |
There are a large number of failing static analysis points in files that have not been touched so I can only assume the upstream library has changed some interfaces etc. |
…erBuilderInterface breaks the return type
Looks like it should all be passing now but the failing 7.4 test matrix (removed in this PR) stops the rest from completing. |
@cooperaj you can fix that by changing the CI under the |
Can't believe I missed that. Thanks. EDIT. |
Closing and reopening to trigger actions, it got disabled. |
Until PHP-CS-Fixer/PHP-CS-Fixer#8300 is fixed it won't have full 8.4 support but if language features are kept at 8.1 levels (as they should be to ensure supported versions) then having this flag shouldn't cause issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a changelog entry. In that regard, is it correct to say that this PR does not bring any breaking change to the end user, apart from indirect dependencies change?
"ext-json": "*", | ||
"facile-it/php-jose-verifier": "^0.3 || ^0.4.3", | ||
"facile-it/php-jose-verifier": "^0.5.0-beta1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: if everything is ok, we should re-tag as stable and change the requirement here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite possibly. Though I've not used any of that library in anger apart from what this library touches so I can't vouch for it all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the 0.5 tag available on that repo yet? I've an issue open there but it is still open?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, I'm thinking that we can use the beta version, and tag this PR in beta too
Apart from the dropping of 7.4 support I'd say that's the case. I don't believe any of the public interfaces were changed but our uses of the library don't touch all the functionality so I'm depending on the unit tests to flag others up. I've been unable to get the conformance suite running. I think the upstream APIs tondo it have changed since the tests were written (~2020) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late additional review.
First of all, thank you! Good job!
We are almost ready to release a beta! Just few changes and clarifications requested.
No CI pipeline seems to be running? As I suspected the additional array_key_exists check was for Psalms benefit. It throws a wobbler when it sees the unset call further down the file about the _claim sources possibly being null. I don't under stand why given the |
CI was disabled due to inactivity, I re-enabled it. I'll close & reopen this PR to trigger a new CI run. |
…o have missed the code check.
Due the web-token libraries reorganisation there are abandoned packages. The upstream facile-it/php-jose-verifier library has a 0.5-beta version that works with the new web-token but needs changes.
These are included. Lots of changes from @p4veI 's branch and some addition work by me.
I've been unable to get the conformance tests to work with a timeout error and there potentially are a large number of other changes that need making.