Add support for Stringable as DataSubjectId#113
Add support for Stringable as DataSubjectId#113DanielBadura merged 2 commits intopatchlevel:1.12.xfrom
Conversation
8d2455f to
c9d738f
Compare
| } | ||
|
|
||
| if ($subjectId instanceof Stringable) { | ||
| $subjectId = $subjectId->__toString(); |
There was a problem hiding this comment.
I'd usually return early here, but followed the style from above with the casted int.
There was a problem hiding this comment.
Based on my other comment, maybe the cast from int here is also wrong at this place. This should be handled in the normalizing layer and not here. wdyt @DavidBadura ?
There was a problem hiding this comment.
@DanielBadura Nothing is normalized here. The data is already normalized when it enters this function. SubjectId must be a string, that's why the cast is done. And integer remains an integer when normlicized.
DanielBadura
left a comment
There was a problem hiding this comment.
What is the exact issue you have? Do you have a 3rd party object which is implementing this Interface or do you have an own value object?
|
|
||
| $subjectId = new StringableSubjectId('user-123'); | ||
|
|
||
| $result = $cryptographer->encrypt( |
There was a problem hiding this comment.
We would also need a decrypt test case. This would also uncover a problem with this approach as I think: The Stringable is a one way ticket. It does not define a way back, this means we cant transform the decrypted scalar value back to the object it was before. Thats also the reason why we dont ship a StringableNormalizer with the library.
I think transforming the object to a scalar string should be done via normalizer so that the cryptographer can ignore the transforming part and just handle the de-/encryption.
| } | ||
|
|
||
| if ($subjectId instanceof Stringable) { | ||
| $subjectId = $subjectId->__toString(); |
There was a problem hiding this comment.
Based on my other comment, maybe the cast from int here is also wrong at this place. This should be handled in the normalizing layer and not here. wdyt @DavidBadura ?
|
So, @DanielBadura , I think the change still makes sense, because from the perspective of |
Feel free to close, if you decide the edge case of denormalizing the subjectId in a different way is not worth adding. |
|
@wikando-ck we decided to merge this and release this soon. Thanks for the contribution! 🚀 |
Adds a failing test to demonstrate desired behaviour.
Adds the necessary feature to the PersonalDataPayloadCryptographer.
Let me know if this should rather target 2.x or needs to be ported there.