Skip to content

[Translation] Improve handling of non-string messages in MessageCatalogue #50252

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Translation] Improve handling of non-string messages in MessageCatalogue #50252

wants to merge 1 commit into from

Conversation

rob006
Copy link
Contributor

@rob006 rob006 commented May 6, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #49894
License MIT
Doc PR -

@carsonbot carsonbot added this to the 6.3 milestone May 6, 2023
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.3 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.3" but it seems your PR description refers to branch "5.4".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@rob006 rob006 changed the base branch from 6.3 to 5.4 May 6, 2023 19:26
@rob006 rob006 force-pushed the 5.4-MessageCatalogue_add-non-string-messages branch from 2f27024 to c3143c2 Compare May 6, 2023 19:28
javiereguiluz
javiereguiluz reviewed May 9, 2023
View reviewed changes
src/Symfony/Component/Translation/MessageCatalogue.php
unset($this->messages[$domain][$id]);
} else {
unset($this->messages[$altDomain][$id]);
$this->messages[$domain][$id] = (string) $message;
Copy link
Member

Choose a reason for hiding this comment

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

But the (string) $message cast will throw an exception if for objects/resources that can't be converted into strings. Maybe it's what we want (to fail loudly so you can catch these errors) but I just wanted to mention it. Thanks.

@stof
Copy link
Member

stof commented May 10, 2023

Adding the validation in the MessageCatalogue constructor will have a performance impact in prod for 2 reasons:

  • this logic will have to run as part of instantiating the cached catalogue (so time spent)
  • the array mutations being performed mean that the array will have to be copied out of the OPCache shared memory (so increased memory usage)

To me, it would be much better to leave MessageCatalogue as is (except adding more precise phpdoc describing the accepted array as being array<string, array<string, string>>) and adding such validation of invalid values in loaders instead.

@rob006
Copy link
Contributor Author

rob006 commented May 10, 2023

How about removing this validation from constructor and leaving it only in add()?

I could move validation to loaders, but it will be duplicated at least 3 times, and add() will be less bulletproof.

@stof
Copy link
Member

stof commented May 10, 2023

Having the validation in loaders might allow providing better error messages (as they would be able to tell you which resources is being loaded that contains an error). And not all loaders will have the same validation (for instance, Xliff will not allow setting null values as all values are strings in XML)

Side note, I don't think the low-level MessageCatalogue object should silently skip values.

@rob006 rob006 mentioned this pull request May 13, 2023
Merged
@rob006
Copy link
Contributor Author

rob006 commented May 13, 2023

Alternative PR: #50315

@nicolas-grekas
Copy link
Member

Closing in favor of #50315

@rob006 rob006 deleted the 5.4-MessageCatalogue_add-non-string-messages branch May 19, 2023 08:24
nicolas-grekas added a commit that referenced this pull request May 19, 2023
@nicolas-grekas
bug #50315 [Translation] Fix handling of null messages in `ArrayLoade…
d189869 
…r` (rob006)

This PR was merged into the 5.4 branch.

Discussion
----------

[Translation] Fix handling of null messages in `ArrayLoader`

| Q             | A
| ------------- | ---
| Branch?       |  5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #49894
| License       | MIT
| Doc PR        | -

Replaces #50252

Commits
-------

4be2036 [Translation] Fix handling of null messages in `ArrayLoader`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz left review comments

Assignees
No one assigned
Labels
Bug Status: Needs Review Translation
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.

[Translation] MessageCatalogue::add() accepts non-string messages
5 participants
@rob006 @carsonbot @stof @nicolas-grekas @javiereguiluz