Related to #1849564: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3. and #1845546: Implement validation for the TypedData API

Turns out, we'll need the symfony TranslatorInterface for being able to use the symfony validator - see http://drupal.org/node/1845546#comment-6781950. Question is how we deal with that?

a) Add the whole component just because of that? It's rather small (~300kB), but still it's quite some unused code just because of the interface.
b) Copy over the interface class only, e.g. below core\lib\Symfony - or somewhere else.

I must say, that personally I'd tend to prefer solution b). Adding the whole component and basically leaving it unused doesn't sound right. As it is not immediately clear the added implementation is not used it might be a bit confusing for newcomers looking at the codebase.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Or ideally, c) it would be solved upstream - see https://github.com/symfony/symfony/issues/6129

fago’s picture

also see the related symfony validator PR: https://github.com/symfony/symfony/pull/6137
This potentially adds a composer dependency on the symfony translation system. We could work around that by adding the interface only, but that's obviously hacky.

fago’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

I'd love to not have parallel solutions for the same thing, and especially not change course on the Drupal translation system 2 days before feature freeze :/

fago’s picture

Talked to Gabor on how well the interface matches our system. He found one problem with multiple formatting though: We generally take both (single, plural) messages as argument to format_plural().

public function transChoice($id, $number, array $parameters = array(), $domain = null, $locale = null);

Gábor Hojtsy’s picture

[5:48pm] GaborHojtsy: fago: this might have been an interesting discussion numerous months back 
[5:48pm] GaborHojtsy: fago: also ClemensTolboom had extensive experience with the Symfony translation backend and found it is not a matching fit for us
[5:48pm] GaborHojtsy: fago: and he did not manage to get stuff in that we'd have needed
[5:48pm] fago: GaborHojtsy, yeah, I read that.
[5:48pm] GaborHojtsy: fago: so I'd prefer not to have parallel systems for the same thing and changing at this point seems like a total no-go
[5:49pm] fago: GaborHojtsy, I know. I don't really want to add it, but symfony valiator might add a dependency on it for translating strings - via that interface
[5:49pm] fago: GaborHojtsy, the code there wouldn't be used, just the interface.
[5:49pm] GaborHojtsy: fago: yeah, but we don't have message domains in Drupal
[5:49pm] fago: ?
[5:50pm] GaborHojtsy: fago: that interface does not make sense
[5:50pm] GaborHojtsy: fago: I know its a string and contexts are strings
[5:50pm] GaborHojtsy: fago: so it looks like its interchangeable, but its not implementing *that* interface if we don't use domains
[5:50pm] fago: GaborHojtsy, according to bschussek, that matches to string contexts - it's a string.
[5:50pm] GaborHojtsy: fago: people cannot just go in and use symfony code with that interface
[5:50pm] GaborHojtsy: fago: because it understands domain differently
[5:51pm] fago: GaborHojtsy, you mean the passed strings would be differently?
[5:51pm] GaborHojtsy: fago: well, domains and contexts in gettext mean two different things, they are independent concepts
[5:52pm] fago: GaborHojtsy, oh, I've not been aware of that domain is a gettext thing.
[5:52pm] GaborHojtsy: fago: yeah both domain and contexts are 
[5:52pm] fago: GaborHojtsy, so it does not match to our context?
[5:52pm] GaborHojtsy: fago: eg. http://docs.python.org/2/library/gettext.html
[5:53pm] GaborHojtsy: fago: so in gettext naturally you pick a domain which is a huge set of strings and then just look up strings without specifying the domain
[5:53pm] GaborHojtsy: fago: its like a DB of strings
[5:53pm] fago: GaborHojtsy, I see.
[5:53pm] GaborHojtsy: fago: context is per string extra metainfo and is not that commonly/widely used
[5:53pm] ClemensTolboom: GaborHojtsy: did I? Symfony had not enough features we need for D8 … not sure what got into their new release
[5:54pm] GaborHojtsy: fago: ^^^^
[5:54pm] fago: GaborHojtsy, got it. Could you do me a favour and post that to the issue? So I can point symfony people to it for why the interface doesn't work for us.
[5:55pm] fago: GaborHojtsy, yeah, I'm not proposing using it 
[5:56pm] ClemensTolboom: GaborHojtsy: this was one of my PR with XRef to others https://github.com/symfony/symfony/pull/4399
[5:56pm] • ClemensTolboom not sure that helps … gtg
effulgentsia’s picture

We need to somehow resolve #5, because it would be a real shame for Drupal to not be able to use Symfony's Validation component due to that incompatibility. Somehow, we need to bridge Symfony validation with Drupal translation.

If the resolution results in a changed or new interface in the Symfony\Component\Translation namespace that works for us, then it would make sense to me for us to add something like a core/vendor-interfaces directory into which we can copy interfaces from other projects without having to copy their implementations.

clemens.tolboom’s picture

webmozart’s picture

There is a chance that the TranslatorInterface, along with other core interfaces, will be split into a separate composer package "symfony/api". Whether this will actually happen is still up for discussion (you are invited to join ;) ).

catch’s picture

Priority: Major » Normal

The PR isn't in yet, so I don't see how this can be major.

fago’s picture

Issue for detailed discussion on how we can solve translating violation messages: #1853096: Integrate symfony validation violations with Drupal translation

effulgentsia’s picture

Status: Active » Needs review
FileSize
3.43 KB

To be honest, I haven't followed the details of #1853096: Integrate symfony validation violations with Drupal translation and related discussions too closely, but it seems to me like that issue reached some resolution, so posting a patch here for what I suggested in #6. #8 would be nice, but I don't think we should hold up Drupal development on it. We can update to using Composer for this component whenever it's ready.

Once this is in, my assumption is that we can unpostpone #1849564: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3.. If that's not the case, please explain, and set this back to needs work.

Status: Needs review » Needs work

The last submitted patch, symfony_translator_interface.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

#11: symfony_translator_interface.patch queued for re-testing.

fago’s picture

Problem with a manual fix like that is that symfony/validator now has the composer-dependency on translator, thus it will still get the whole packages when we do a composer update.

Maybe, we could just go with the package added by composer and modify .gitignore such that we only commit the interface? If someone else wants to add the full symfony translator I suppose he'd have a composer file out of Drupal anyway and just pull the full component out-side of Drupal - thus there should be no harm in the .gitignore approach?

fago’s picture

On IRC effulgentsia mentioned that the .gitignore approach works or him, so going for that in #1849564: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3..

fago’s picture

Status: Needs review » Closed (duplicate)

Let's handle this in the other issue as well now.

fago’s picture

Issue summary: View changes

Updated issue summary.