IBX-11959: Added getters to Translation Message and Plural value objects - #795
IBX-11959: Added getters to Translation Message and Plural value objects#795tbialcz wants to merge 2 commits into
Conversation
92a2f41 to
a07c105
Compare
alongosz
left a comment
alongosz
left a comment
There was a problem hiding this comment.
For QA
Internal API only
src/contracts/FieldType/ValidationError.php is definitely not an internal API. Though it doesn't mean that there's something to QA. It's rather something for Doc release notes of 6.0.0
| * @return \Ibexa\Contracts\Core\Repository\Values\Translation\Message|\Ibexa\Contracts\Core\Repository\Values\Translation\Plural | ||
| */ | ||
| public function getTranslatableMessage(): Translation; |
There was a problem hiding this comment.
This looks like a code smell to me. We're declaring it as strict Translation but then say that we return some other types and in each impl. we define Translation again and say again that it's something else.
Can't covariance/contravariance or templating be used here?
There was a problem hiding this comment.
switched to a native Message|Plural return type on the contract, implementations covariantly narrow to Message. Docblocks removed
There was a problem hiding this comment.
Is it me or this is much worse, why other Translation interfaces implementation cannont be returned now from this method?
There was a problem hiding this comment.
you couldn't really return anything else anyway, Translation has only __toString() so everyone checks for Message or Plural to do their job. Subclassing Message/Plural still works
| ]; | ||
| } | ||
|
|
||
| public function testGetters(): void |
There was a problem hiding this comment.
Would it make sense to add to test cases where the current behavior is asserted?
There was a problem hiding this comment.
reworked both tests to share the data providers, so getters are now asserted across all existing cases. Not sure it buys us much given the getters just return constructor args, but the coverage doesn't hurt. 🙂
|
5 participants
Description:
Added getters to
MessageandPluraltranslation value objects, so consumers don't have to rely on magicValueObject::__getproperty access (which PHPStan flags).ValidationError::getTranslatableMessage()now declares the native return typeMessage|Plural(all implementations return exactly these two types); implementations covariantly narrow toMessagewhere applicable.Needed by the follow-up
ibexa/userPR for IBX-11959 (ibexa/user#132). No behavior change.For QA:
No user-facing change, covered by unit tests. End-to-end QA on the
ibexa/userPR.Documentation:
For 6.0.0 release notes:
Ibexa\Contracts\Core\FieldType\ValidationError::getTranslatableMessage()return type narrowed fromTranslationtoMessage|Plural— third-party implementations declaringTranslationmust update their signatures. New getters onMessage(getMessage(),getValues()) andPlural(getSingular(),getPlural(),getValues()).