Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[FORM] fix post_max_size_message translation #18543

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

Closed
wants to merge 2 commits into from
Closed

[FORM] fix post_max_size_message translation #18543

wants to merge 2 commits into from

Conversation

DavidBadura
Copy link
Contributor

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #15479
License MIT
Doc PR -

@DavidBadura DavidBadura changed the title fix post_max_size_message translation [FORM] fix post_max_size_message translation Apr 14, 2016
{
$this->serverParams = $params ?: new ServerParams();
$this->translator = $translator;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should create a translator if null is passed imo, since this can be used with Form as standalone component.

Alright forget it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a kind of feature, shouldn't be documented in the changelog that you can pass a translator to request handlers ?
Maybe this should go on master ?

@nicolas-grekas
Copy link
Member

It's a new feature to me also, should go on master

@Nek-
Copy link
Contributor

Nek- commented Apr 15, 2016

@nicolas-grekas what feature does it add ? :-) IMO translation is not feature. Error translation is a feature of Symfony.

Also there is no problem merging it in 2.7, so why not ?

@DavidBadura
Copy link
Contributor Author

doesn't the bug exist because of #11924?

Btw. the translation already exists: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Resources/translations/validators.de.xlf#L10 but apparently it doesn't get used.

i don't care whether it gets translated in 2.7 or master but it should be done at one point.
Should i create a PR for master?

@HeahDude
Copy link
Contributor

So as a bug fix it should go on 2.3, right ?
The fact that it might be considered as a new feature is that request handlers now get passed a translator. Someone using its own implementation still needs to do the translation himself in its implementation.

I agree with you and would vote to do this as a bug fix, but I doubt it gets accepted.

However, if it goes in master maybe there is a better way to handle "extra" translations (not handled by the violation builder) more globally (maybe use an abstract request handler to hold some common logic), the csrf type extension also gets the translator only to pass it to its listener (even if that allows a lazy translation of the message, I wonder what consumes more memory: passing the translator in many instances or just a translated message?).

$this->requestHandler = $this->getRequestHandler($translator);

$options = array('post_max_size_message' => 'old max {{ max }}!');
$form = $this->factory->createNamed('name', 'text', null, $options);
Copy link
Contributor

Choose a reason for hiding this comment

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

text --> Symfony\Component\Form\Extension\Core\Type\TextType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this works only in >=2.8

@aitboudad
Copy link
Contributor

aitboudad commented Apr 16, 2016

Instead of the current implementation what do you think about adding a new form type extension that translate post_max_size_message see https://gist.github.com/aitboudad/02db0ef89300991503fd287fb6ce3eec

@HeahDude
Copy link
Contributor

@aitboudad 👍 Awesome! Remains the question: 2.3 or master?

@xabbuh
Copy link
Member

xabbuh commented Apr 21, 2016

To me this is a bugfix (IIRC this used to work before #11924).

@DavidBadura
Copy link
Contributor Author

so, what are the next steps? is it a bug (merge in 2.3/2.7) or a feature (merge in master)? should I change it as proposed by @aitboudad?

@aitboudad
Copy link
Contributor

well I would consider it as a bugfix

@fabpot
Copy link
Member

fabpot commented Apr 28, 2016

This is a bugfix.

@fabpot
Copy link
Member

fabpot commented Jun 15, 2016

2.7 branch is the good one as this is now the oldest and still maintained branch.

@DavidBadura
Copy link
Contributor Author

#19061 <- an alternative implementation as proposed by @aitboudad

@fabpot
Copy link
Member

fabpot commented Jun 22, 2016

closing in favor of #19061

@fabpot fabpot closed this Jun 22, 2016
fabpot added a commit that referenced this pull request Jun 22, 2016
…id Badura)

This PR was merged into the 2.7 branch.

Discussion
----------

[FORM] fix post_max_size_message translation (alt. 2)

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #15479, #18543
| License       | MIT
| Doc PR        | -

Commits
-------

9d8a5e5 fix post_max_size_message translation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.