Updated Bank ID version #5
Conversation
It's also possible up fallback to an older version. Some dependencies got updated to meet the requirements of the updates packages.
|
Hi and thanks for the PR! Can't we just change from 5 to 5.1? What is the use case for having the version as an argument? |
|
The reason I see is that Bank ID wrote somewhere that there might be some breaking changes. So my reason is if we let the user decide if he/she want to use 5, 4 or 5.1 (and later down the road) they can. If I understande their documentation right, what will change is mostly the response. For me personally, to simply change to 5.1 is "enough" / good. :-) |
|
Just to clarify. There's no breakning changes between 5 and 5.1. So it was more a way to make it future proof. |
|
I see, thanks for the clarification! I don't think the library will be able to support different major versions at the same time (e.g the now discontinued v4, and v5), but probably minor releases. This is the first minor release during the life span of this package so I think we'll take a wait and see approach. But if we have a hard time keeping up it might be an option to provide a setting for this. I tried to find information regarding v5.1 on https://www.bankid.com/utvecklare/rp-info but couldn't find any. Do you have any references? |
|
This is the biggest change between 5.1 and 5.0, "Support for animated QR-codes. It's not a big deal, but perhaps those that use autoStartTokenRequired qould preferer to be able to do so. You can read some more on page 19 (26) here: https://www.bankid.com/assets/bankid/rp/bankid-relying-party-guidelines-v3.5.pdf But I agree, it isn't very important to be able to set the version. The main reason for me to even start to update your package (which works great) was to update the dependencies as the previous version of GuzzleHttp conflicted with some WP-plugins. Do you wan't me to clean up the PR (without the option to set version) and send a new commit/PR? |
|
Oh, I missed that section. Thanks! autoStartTokenRequired seems to still be supported (but deprecated), so should be fine to bump to 5.1. A PR that just changes the url to v5.1 is what I would prefer. The documentation changes is not needed I think, except maybe mention that we use 5.1. Regarding guzzle that will need to be discussed in an issue or separate PR - I'm not sure that would be a non breaking change or be the preferred packages for e.g Laravel users. Not sure how to best solve that at the moment. |
|
I will give you more information tomorrow. But a short description of the problem I stumbled upon today regarding guzzle. Between 7.01 and 7.2 they changed some method signatures. This led to an exception that made the Bank ID fail. I haven't taken a look at exactly what row in what method that happened. But I was able to pinpoint that as soon any other plugin, package, code that had a dependency on guzzle Bank ID failed as the version contained methods that has been removed (if my tired eyes read their changelog correctly). One of the plugin that caused this was SEO Press, which is top 5 most popular SEO plugins for WP. That's why it's a bit of a bummer for me. From my understanding, even if you change dependency to 7.2 instead of 7.0.1 it won't break anything. Except if "you" user a small set of methods. Those methods would need to be changed (more or less just a rename). Guzzle will still be able to run v 6. So personally I think the gain far overweigh the cons by updating. At the same time I do respect your opinion to not rush to a conclusion and just update right away. I will try to collect more information tomorrow so both you and me have all the facts straight. I'll wait to submit a PR with only 5.1 until then. Hope that's ok. :-) |
|
Sounds good!
I’m pretty sure the version constraint in this package doesn’t limit you to use guzzle 7.2 in your own project.
… 21 jan. 2021 kl. 00:28 skrev Daniel Nordmark ***@***.***>:
I will give you more information tomorrow. But a short description of the problem I stumbled upon today regarding guzzle. Between 7.01 and 7.2 they changed some method signatures. This led to an exception that made the Bank ID fail.
I haven't taken a look at exactly what row in what method that happened.
But I was able to pinpoint that as soon any other plugin, package, code that had a dependency on guzzle Bank ID failed as the version contained methods that has been removed (if my tired eyes read their changelog correctly).
One of the plugin that caused this was SEO Press, which is top 5 most popular SEO plugins for WP.
That's why it's a bit of a bummer for me.
From my understanding, even if you change dependency to 7.2 instead of 7.0.1 it won't break anything. Except if "you" user a small set of methods. Those methods would need to be changed (more or less just a rename). Guzzle will still be able to run v 6.
So personally I think the gain far overweigh the cons by updating.
At the same time I do respect your opinion to not rush to a conclusion and just update right away.
I will try to collect more information tomorrow so both you and me have all the facts straight. I'll wait to submit a PR with only 5.1 until then. Hope that's ok. :-)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
I'll give that a try in 10 minutes or so. I tried to read up what's changed between 7.0.1 and 7.2. It's mostly bug fixes except for this,
I took a look at the code and it seems they moved these methods to static metods on Utils. I did a quick try and as long as I changed to use As long as the above methods is used in another dependency it's pretty straight forward to just swap to Utils. Personally I would say that's it's a better alternativt to move to 7.2 because it's over 3 months old and more and more packages depend on that version. 7.0.1 is over 6 moths old. Other than the methods signatures above, 7.2 contains some good bug fixes. |
Updated BankID version to 5.1 and I also made it possible to specify another version when creating an instance of
BankID(). Clarified the difference between the different verions. Finally I updated the dependencies as they was a bit out of date.