v5.5.0 JWT::decode() signature breaks backward compatibility

[AymDev]

The Firebase\JWT::decode() method signature changed for its second argument from string|array|resource to Key|array<Key>, which broke some of our CI builds at their static analysis stage. This has been introduced in v5.5.0 with bc0df64. We're going to lock our version dependency at >=5.0 <5.5 but would like to know if this will be reverted or not ?

[tuupola]

Changing method signature is a BC break. Maybe this should be released as 6.0?

[wokenlex]

Yes, got it today in composer update, the main package what used it even didn't know about it. That's was really frustrating

[bshaffer]

IMPORTANT: 5.5 does not break backwards compatibility, only the docblock was changed

The resource type still accepts string|array|resource and remains fully backwards compatible. The docblock was updated because using the library in this way exposes a potential security hole (see #351).

Do you have an example of a previous implementation actually breaking, or are you only concerned regarding the documentation of the function? The intention of v5.5.0 is that the library remains fully backwards compatible to anyone who updates to this version.

[bshaffer]

We can change the docblock to include string|array|resource in a v5.5.1 release, if this is an issue. I went with the current approach because I wanted an indication that implementations should be changed if possible (but won't result in any breaks in production if left unupdated).

[wokenlex]

With this package - https://github.com/patrickbussmann/oauth2-apple - and 5.5.0 it makes "Uncaught PHP Exception UnexpectedValueException: "$keyOrKeyArray must be a string key, an array of string keys, an instance of Firebase\JWT\Key key or an array of Firebase\JWT\Key keys"

[bshaffer]

Ok you're right... The exception here will be thrown when keys are a resource, which was previously accepted. I'll remove it and add a test and tag 5.5.1

[AymDev]

Okay so it is backward compatible, good news. The only issues we met are the errors reported by static analysis (PHPStan) which reads the typehints and the PHPDoc.
I guess that bringing the previous types back in a v5.5.1 would be a good idea. Maybe with a note that they won't be supported in the next major release ?

Thank you for your response.

[rpkamp]

It's not a runtime BC, but it does break pipelines that have tools like PHPStan running, as they will complain that passing a string is not allowed.

Also, since PHP now has named parameters, that makes parameters names part of the public API as well. So changing the name of a parameter is also a BC break.

[InvisibleSmiley]

While you're at it, you might also want to fix this PHPDoc:

@return an array containing the keyMaterial and algorithm

(JWT::getKeyMaterialAndAlgorithm; parsed type is "an")

[bshaffer]

@AymDev @tuupola @wokenlex @rpkamp @InvisibleSmiley

Can you take a look at #371 and verify this will fix the issues you're seeing?

[AymDev]

@bshaffer Yes this would work for me !

[bshaffer]

Fixed in https://github.com/firebase/php-jwt/releases/tag/v5.5.1

[rpkamp]

The variable is still renamed, so technically it's still a BC.

Personally I don't mind. Just saying.