Improve deprecation for aud claims

[SvenRtbg]

Addresses some concerns in #629.

[SvenRtbg]

Seems like I have to enable signatures for my commits... will do ASAP, but it will take a while. Please review and give feedback.

[Ocramius]

Note: I don't think you actually need to add GPG signatures to commits.

Doubt cleared

[SvenRtbg]

Do we still have an issue here because of the commit signature requirement?

[SvenRtbg]

I force-pushed again with a GPG key attached.

[lcobucci]

@SvenRtbg thanks for sending your suggestions.

I'm hesitant to add the deprecation messages because v3 wasn't designed to use arrays on the audience claim. This worked by "accident" for your application (because of several issues on the lib).

Apart from that argument, adding deprecation messages in a patch release is a no-go for me and v3 series is in bug-fix mode only.

[SvenRtbg]

I can relate, however I think the 3.4 version is aimed at supporting the transition to 4.0.

Before 3.4, I could easily use withClaim('aud',[...]) without complaints. Deprecation messages from that call lead to using permittedFor(), and this doesn't work when using an array as the parameter, which has been fine before.

And I cannot really skip migrating this call in 3.4 because in 4.0 the previous method is deleted. And I also cannot really avoid calling permittedFor() because the withClaim() in 3.4 calls forwardCallToCorrectClaimMethod() internally, which calls permittedFor() for 'aud' claims. So it will always cast the parameter to a string, changing any (previously valid) array to the string "Array", and messing with the application - or in my case, the test cases.

So I would consider this change a backwards incompatible behaviour regarding withClaim(), and it should be explained to the developer somehow. The least intrusive way is the deprecation message.

[lcobucci]

@SvenRtbg I completely understand your point and frustration. My main concern is that you were relying on something that wasn't officially supported so, I struggle to label it as a BC-break.

However, let's summon other maintainers here. @Ocramius @Slamdunk what are your thoughts on the matter?

[Slamdunk]

3.3 adhered to RFC better than 3.4, so I agree with @SvenRtbg

What have been done in 3.4 should be considered to me only an oversight, affecting a minority of users.
I like the message of trigger_error written in the getClaim method, and I'd adopt a similar one into permittedFor as well:

-Using arrays for audience claims is not really supported in 3.4 and will behave different in 4.0.
+Support to arrays for audience was mistakenly removed from 3.3 in 3.4. In 4.0 this will change again with more flexibility, please consult the documentation.

And then I'd tag a new patch release for 3.4.

[lcobucci]

The discussion isn't about adherence but the fact that it has never been officially supported.

It used to work by mistake, which was made clearer on v3.4. The official support only came in v4.

So how can we label the fact that something used to work by mistake and now doesn't work anymore as a BC-break?

[Slamdunk]

So how can we label the fact that something used to work by mistake and now doesn't work anymore as a BC-break?

I see you are referring to the string doc-block already mentioned here.

There is no right answer: it was a mistake to not dynamically type-check the variables in the first place, as well as it is a user mistake to not use static analysis.

3.3 is the past, 4.0 is the future, 3.4 is a bridge between the two: as is, I don't value the correct label for the issue, but rather the clear message to the users on what happened and how to remedy. This PR, maybe with better messages in the trigger_error, is good at aiming this goal, imho.

[SvenRtbg]

I do like @Slamdunk 's suggestion for the deprecation message better. Will change that. The comments regarding the upgrade file will also be considered.

As has been said before, arrays used to work in 3.3, they stopped working in 3.4, and work again in 4.0. I had to figure out why my test cases failed after updating to the compatible 3.4 version, and I'd very much like to have something reassuring message that it isn't my code that is still broken during upgrades, but a use case that will be supported again.

Thanks for your feedback.

[lcobucci]

@SvenRtbg @Slamdunk thank you very much for your patience and making me think.

3.3 is the past, 4.0 is the future, 3.4 is a bridge between the two

This is a simple and yet very interesting sentence - and it made me reconsider the whole thing.

Considering v3.4 as the bridge between the two versions, I've created a bridge with a missing plank.

I took the deliberate decision of not backporting the support for multiple audiences and that was a wrong decision. It was wrong simply because the interface/behaviour isn't the same as v4.

Making the old API behave as the new one requires a minor BC-break (claim replication, broken functionality which wasn't removed when introducing the new builder methods in v3.3 or so).

I've created a patch to fix this issue (#656) and would like for us to move to that direction instead - even if we have to do a minor BC-break.

Would you please review it and share your thoughts there?

If we all agree with that solution, we can repurpose this PR to keep some of the improvements @SvenRtbg done on the docs and release it together with v3.4.4.

Thank you again for your patience throughout the discussion and sorry for being attached to such petty details - sometimes I get stuck in useless peculiarities.

[SvenRtbg]

I like deprecation messages, so I added them to the Token class for every deprecated method. I'm not sure if this is going too far, however one has to deal with deprecated methods anyway, and the easier they are to spot the easier they are to remove.

Regarding "too far": I have the feeling that I'd probably have to deal with internal calls to methods triggering deprecation notices in order to get rid of them, otherwise one would not be able to upgrade a code base to be free of them.

I intentionally left the upgrade document in a separate commit, to be able to cherrypick it. Let me know what you think.

[SvenRtbg]

The library still exposes an asymmetric behavior when dealing with audience arrays and using the deprecated methods, as described in my PR now. So without an update to the new Token API, there will be errors appearing in edge cases. But I think the world can live with that regardless.

I'm not sure I added this fact into the ParserTest sufficiently, I just duplicated the test cases that seem to accidentally use the audience claim as example data.

[lcobucci]

I like deprecation messages, so I added them to the Token class for every deprecated method. I'm not sure if this is going too far, however one has to deal with deprecated methods anyway, and the easier they are to spot the easier they are to remove.

Regarding "too far": I have the feeling that I'd probably have to deal with internal calls to methods triggering deprecation notices in order to get rid of them, otherwise one would not be able to upgrade a code base to be free of them.

My (implicit) policy is to only add the deprecation messages for things that can't be converted with @deprecated (like deprecating arguments or deprecation of logic blocks). IDEs and tools like phpstan/phpstan-deprecation-rules help people to move away from components annotated with @deprecated, which reduces the noise generated by the library.

With that said, let's please only add the messages when using arrays on aud claim via the old API 👍

I intentionally left the upgrade document in a separate commit, to be able to cherrypick it. Let me know what you think.

I reaaaaaaaaally appreciate your commit organisation ❤️

[lcobucci]

Just some feedback on the docs :)

[SvenRtbg]

I think we are good now: Improved upgrade guide, and the offending issue has a dedicated deprecation message with test case.

[lcobucci]

That's perfect. Thanks a lot for your help @SvenRtbg!

[lcobucci]

@SvenRtbg all released! I'll do some manual merge-ups since the release automation failed due to branch naming and all.

Thanks again!