Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Marked Translation class as implementing Stringable #307

Open
wants to merge 1 commit into
base: 1.3
Choose a base branch
from

Conversation

Steveb-p
Copy link
Contributor

Question Answer
JIRA issue N/A
Type improvement
Target Ibexa version v3.3
BC breaks no

This PR adds Stringable as phpdoc annotation to eZ\Publish\API\Repository\Values\Translation.

Reasoning

When working with content validation it become apparent for me that this class is lacking Stringable interface. Ibexa\Core\FieldType\ValidationError::getTranslatableMessage returns a Translation object that - judging by our implementations: Plural & Message - should always have __toString() method.

Of course, Stringable did not exist at the moment when this code was written.

However, because there is no guarantee that user code that extends it does indeed implement this method, and because Stringable interface is available from PHP 8.0, I'm opting to mark the class as implementing it via phpdoc instead. Otherwise we could break user code or require a polyfill (which is in itself an option, due to it's lightweight nature).

We cannot simply add this method to Translation without hiding the fact that implementation is missing, because as per php docs:

Warning
It was not possible to throw an exception from within a __toString() method prior to PHP 7.4.0. Doing so will result in a fatal error.

I propose that for 3.3:

  • We add a phpdoc to Translation denoting the intent of implementing Stringable

but for 4.x:

  • We add a polyfill requirement for symfony/polyfill-php80
  • Directly implement Stringable in Translation
  • Add an exception throwing implementation __toString() method, so that user code does not break.

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/engineering-team).

@sonarcloud
Copy link

sonarcloud bot commented May 10, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Steveb-p Steveb-p requested a review from a team May 11, 2022 11:14
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Steveb-p I'm all for it if it doesn't break static analysis. Or is the issue outdated since it was forgotten? :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants