Problem/Motivation

#1849564: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3. added the interface from Symfony's translation component. This is only required because validator references that interface (and hence has a dependency), we never actually use the translation component anywhere in core. In that patch a .gitignore trick was used to avoid pulling in the entire library.

There was supposed to be a follow-up to sort this out properly - see #1849564-52: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3., but as far as I know that never got filed.

In the meantime, subsequent composer updates have brought in the full component. This is potentially confusing to people because they might think we actually use it (incuding posting to planet about that fact today hence me opening this issue), whereas Drupal's translation system is completely separate and none of the code gets executed ever.

Proposed resolution

Composer has a replace feature which core already uses since #2456009: Add a "replace" section to core/composer.json. So we can add the interface alone to core and tell composer that core replaces symfony/translation.

Remaining tasks

User interface changes

API changes

Data model changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because its not a bug for itself, but we want to get rid of as many external code as possible
Issue priority Critical, because once its out, we cannot remove it anymore. One less library also means less maintenance costs.
CommentFileSizeAuthor
#95 vendor-symfony_translator-2546618-95.patch414.81 KBplach
#95 vendor-symfony_translator-2546618-95.interdiff.txt1.89 KBplach
#90 vendor-symfony_translator-2546618-90.patch413.28 KBplach
#90 vendor-symfony_translator-2546618-90.interdiff.txt1.94 KBplach
#88 vendor-symfony_translator-2546618-88.patch414.49 KBplach
#88 vendor-symfony_translator-2546618-88.interdiff.txt322 bytesplach
#82 vendor-symfony_translator-2546618-82.review.txt8.04 KBplach
#82 vendor-symfony_translator-2546618-82.patch414.51 KBplach
#82 vendor-symfony_translator-2546618-82.interdiff.txt1.06 KBplach
#80 vendor-symfony_translator-2546618-80.review.txt8.06 KBplach
#80 vendor-symfony_translator-2546618-80.patch414.53 KBplach
#76 vendor-symfony_translator-2546618-76.review.txt6.98 KBplach
#76 vendor-symfony_translator-2546618-76.patch412.61 KBplach
#65 vendor-symfony_translator-2546618-65.patch421.09 KBplach
#65 vendor-symfony_translator-2546618-65.review.txt17.95 KBplach
#65 vendor-symfony_translator-2546618-65.interdiff.txt1.5 KBplach
#59 2546618_58-do-not-test.patch16.44 KBchx
#58 vendor-symfony_translator-2546618-47.patch419.58 KBplach
#47 vendor-symfony_translator-2546618-47.review.txt16.44 KBplach
#47 vendor-symfony_translator-2546618-47.patch419.58 KBplach
#47 vendor-symfony_translator-2546618-47.interdiff.txt1.67 KBplach
#44 vendor-symfony_translator-2546618-44.patch419.05 KBplach
#39 vendor-symfony_translator-2546618-39.review.txt15.92 KBplach
#39 vendor-symfony_translator-2546618-39.patch418.21 KBplach
#39 vendor-symfony_translator-2546618-39.interdiff.txt5.08 KBplach
#31 2546618-30.patch419.12 KBdawehner
#31 interdiff.txt5.51 KBdawehner
#31 review.txt13.29 KBdawehner
interdiff.txt5.51 KBdawehner
2546618-30.patch419.12 KBdawehner
review.txt13.29 KBdawehner
#27 2546618-27.patch418.82 KBdawehner
#23 review.txt12.99 KBdawehner
#23 2546618-23.patch417.98 KBdawehner
#20 2546618-20.patch417.98 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

alexpott’s picture

Did we try to file an upstream patch to decouple validator and translation - it seems unnecessary?

catch’s picture

Issue summary: View changes
dawehner’s picture

dawehner’s picture

xjm’s picture

Priority: Major » Critical
Issue tags: -revisit before release candidate

Discussed with @alexpott, @catch, @effulgentsia, and @webchick. Removing unused 3rd party code before RC is part of #2485119: [meta] The Drupal 8.0.0-rc1 Release Checklist, so bumping to critical now for that reason.

A non-critical thing to fix would be to figure out how we don't accidentally re-add this again in the future.

heddn’s picture

Issue tags: +Novice

Is this actionable and simple enough that all we are doing is doing git rm -rf on a folder? If so, let's mark this as novice.

dawehner’s picture

I thought we just committed the interface ...

effulgentsia’s picture

I think the Novice tag makes sense because it doesn't require a lot of Drupal expertise, but it might require some git skills.

We still need to retain Symfony/Component/Translation/TranslatorInterface.php, whether that's achieved manually or via git magic. Prior to #1977570: Update third-party vendors and fix composer.json, we had a /core/vendor/.gitignore file that did this via:

# Symfony Validator depends on Symfony Translation but only requires the
# TranslatorInterface. Thus, we add only the required interface from Symfony
# Translation by ignoring everything except the interface.
symfony/translation/Symfony/Component/Translation/*
!symfony/translation/Symfony/Component/Translation/TranslatorInterface.php

But the patch in that issue removed that .gitignore file, maybe for good reasons. Per #6, reinstating that .gitignore file doesn't need to be part of this issue's scope if there are any downsides to doing so.

xjm’s picture

So I'm confused now; is the idea that the translation component should be listed in core/composer.json still and just not be in the repo, except for the interface?

webchick’s picture

Component: base system » documentation

That's the proposal, yes. Which sounds frightfully confusing to me, not to mention a Drupalism. :\ I lean towards documentation here, and just pulling the entire library in rather than resorting to weird, non-standard hacks.

The ideal place to put such documentation would be in composer.json itself, since that is presumably the "table of contents" that people are reading when they're reaching false conclusions that we're using Symfony's translation stuff instead of our own, which we're not.

However, as we all remember from the great CMI file format debate, you can't put comments in JSON. Womp womp.

Second place could possibly be at the top of the newly-minted https://www.drupal.org/node/2562903.

Another place could be CHANGELOG.txt.

Moving to documentation to get some opinions from Jennifer et al.

webchick’s picture

Heh. 79 comments on the won't fixed "allow comments in composer.json" issue https://github.com/composer/composer/issues/1988 ;)

plach’s picture

What about extending the interface and putting the documentation there?

plach’s picture

This way people inspecting Drupal code will find it uses a Drupal-provided interface and may be less prone to conclude we leverage the Symfony translation component.

catch’s picture

bojanz mentioned in irc that there is https://getcomposer.org/doc/04-schema.md#replace

So we could:

1. Fork the interface to somewhere in /core (with some documentation as to why).

2. Make sure the autoloader finds it properly.

3. Add the replace line to composer.json to point out that drupal-core satisfies the Translation component.

Then the actual Translation component disappears.

dawehner’s picture

So the plan is to a) Use replace b) Use a custom interface c) Profit

xjm’s picture

#15 sounds quite sensible to me.

webchick’s picture

Title: Remove symfony Translation component, except for the interface, or document better that we don't use it » Replace the Symfony Translation component with a custom interface since we don't use it
Component: documentation » base system

Updating the title based on the plan. Also moving out of documentation.

plach’s picture

Issue tags: -Novice

I will work on this tomorrow if none beats me to it

dawehner’s picture

Status: Active » Needs review
Issue tags: +D8 Accelerate
FileSize
417.98 KB

Working in bed.

dawehner’s picture

The interface documentation certainly needs improvements.
Should we also put a README maybe pointing to the documentation in the interface into the new directory?

Status: Needs review » Needs work

The last submitted patch, 20: 2546618-20.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
417.98 KB
12.99 KB

Reroll + a review patch.

Status: Needs review » Needs work

The last submitted patch, 23: 2546618-23.patch, failed testing.

The last submitted patch, 20: 2546618-20.patch, failed testing.

The last submitted patch, 23: 2546618-23.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
418.82 KB

This patch is proudly presented by --binary

Status: Needs review » Needs work

The last submitted patch, 27: 2546618-27.patch, failed testing.

The last submitted patch, 27: 2546618-27.patch, failed testing.

dawehner’s picture

Let's see

dawehner’s picture

FileSize
13.29 KB
5.51 KB
419.12 KB

These are the files I wanted to upload

The last submitted patch, 2546618-30.patch, failed testing.

The last submitted patch, 2546618-30.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 31: 2546618-30.patch, failed testing.

The last submitted patch, 31: 2546618-30.patch, failed testing.

webchick’s picture

#2565337: Upgrade to Symfony 2.7.4 is now in. That's hopefully the last of annoying composer.json conflicts that will result in annoying re-rolls.

plach’s picture

Assigned: Unassigned » plach

Working a bit on this

klausi’s picture

+++ b/core/composer.json
@@ -120,7 +120,8 @@
+    "symfony/translation": "~2.4"

why 2.4? Shouldn't we be on Symfony 2.7?

plach’s picture

This should fix the failures, we just had a few wrong namespaces.

Status: Needs review » Needs work

The last submitted patch, 39: vendor-symfony_translator-2546618-39.patch, failed testing.

dawehner’s picture

why 2.4? Shouldn't we be on Symfony 2.7?

The reason I went with that value was that symfony validator also just needs that particular version.

  1. +++ b/core/lib/Drupal/Core/Validation/TranslationInterface.php
    @@ -0,0 +1,24 @@
    +/**
    + * Defines an interface used in validation.
    + *
    + * This extends the interface used by the Symfony validator in order to indicate
    + * that the Drupal code is actually independent from the Symfony translation
    + * component.
    + *
    + * Note: We use a composer.json "replace" entry in order to bypass the
    + * composer.json dependency onto the Symfony translation component.
    + */
    +interface TranslatorInterface extends SymfonyTranslatorInterface {
    +
    +}
    

    Should we expand the documentation a bit here?

  2. +++ b/core/lib/Drupal/Core/Validation/TranslationInterface.php
    index 0000000..43028bc
    --- /dev/null
    
    --- /dev/null
    +++ b/core/lib/Symfony/Component/Translation/LICENSE
    

    What about adding a README.txt here?

The last submitted patch, 39: vendor-symfony_translator-2546618-39.patch, failed testing.

catch’s picture

#1 we could use a @todo/@link to https://github.com/symfony/symfony/pull/6189 and https://github.com/symfony/symfony/issues/15714, that might be enough though? Especially if we add a README.

plach’s picture

Status: Needs work » Needs review
FileSize
419.05 KB

Rerolled with --binary

Status: Needs review » Needs work

The last submitted patch, 44: vendor-symfony_translator-2546618-44.patch, failed testing.

The last submitted patch, 44: vendor-symfony_translator-2546618-44.patch, failed testing.

plach’s picture

dawehner’s picture

The readme looks fine for me! Now we just need to have a patch which actually passed.

dawehner’s picture

12:49:39 Segmentation fault (core dumped)
12:49:39 FATAL Drupal\taxonomy\Tests\Migrate\d6\MigrateTermNodeRevisionTest: test runner returned a non-zero error code (139).
12:49:39 Drupal\text\Tests\Migrate\MigrateTextConfigsTest 10 passes

Mh

Gábor Hojtsy’s picture

Issue tags: +D8MI, +sprint, +language-ui

Tagging with all the right D8MI tags too.

Status: Needs review » Needs work

The last submitted patch, 47: vendor-symfony_translator-2546618-47.patch, failed testing.

dawehner’s picture

Wow, DrupalCI actually doesn't see segmentation faults :(

plach’s picture

Status: Needs work » Needs review

Test is green here, re-testing.

plach’s picture

Assigned: plach » Unassigned
plach’s picture

Created #2565669: Update Twig to 1.21.2 while working on this.

dawehner’s picture

Yeah I guess it will be green.

@plach
Better reupload a patch in case we introduced random failures.

Status: Needs review » Needs work

The last submitted patch, 47: vendor-symfony_translator-2546618-47.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
419.58 KB

Reuploading

chx’s picture

FileSize
16.44 KB

Here's a review only version of this patch. I created it with git diff HEAD --diff-filter=MA. (Just an aside, funny how the typical patch can be created with --diff-filter=MAD.)

chx’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
dawehner’s picture

Issue summary: View changes

added beta eval.

bojanz’s picture

Looks great!

The last submitted patch, 47: vendor-symfony_translator-2546618-47.patch, failed testing.

plach’s picture

Status: Reviewed & tested by the community » Needs work

It seems we need also the IdentityTranslator...

plach’s picture

This should fix the last failing test, however currently the ValidatorBuilder will fatal if no translator is set because it will try to instantiate IdentityTranslator. However we cannot include that as it has lots of implicit dependencies, so we would end up copying many classes from the translation component which is definitely not ideal.

Not sure how to deal with this honestly: we could either document that the ValidatorBuilder should always have a translator or we need to do some "fancy" stuff like providing our own version of the IdentityTranslator in the Symfony\Component\Translation namespace extending our DrupalTranslator (ew).

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/TypedData/RecursiveContextualValidatorTest.php
@@ -82,7 +80,12 @@ protected function setUp() {
+      ->willReturnCallback(function($id) {
+        return $id;
+      });

Nitpick: Any reason to not use $id but rather $string?

plach’s picture

$id is the name of the parameter in the interface.

chx’s picture

Status: Needs review » Reviewed & tested by the community

> ValidatorBuilder will fatal if no translator is set

So what, you will get a fatal in HEAD if you call ValidatorBuilder::setPropertyAccessor as it is typehinted with an interface from the PropertyAccess namespace/component which we did not include in HEAD. So: no new problems introduced by this patch.

The last submitted patch, 58: vendor-symfony_translator-2546618-47.patch, failed testing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Yes I agree with #68, the unit test update is very minor and I can't see this coming up anywhere else.

bojanz came up with the composer trick, so adding to commit credit.

Committed/pushed to 8.0.x, thanks!

  • catch committed 3d2cf1a on 8.0.x
    Issue #2546618 by plach, dawehner, chx, bojanz: Replace the Symfony...
alexpott’s picture

Just asking because it does not seem to be covered - what happens if a project wants to include the full Symfony Translation component as a dependency?

Gábor Hojtsy’s picture

Issue tags: -sprint

Removing from D8MI sprint.

alexpott’s picture

Status: Fixed » Needs review

After discussing on IRC with @bojanz, @dawehner and @plach, I'm less convinced that this was the correct solution. This is because whilst including the entire symfony translation component for a single interface seems really wasteful, what this change means is that if some project wants to add a dependency using composer that has a dependency on Symfony Translation they have to hack core's composer.json and rebuild their autoloader. This will be a really hard bug to find and once found will result in "what the dickens were they thinking" thoughts.

I think we should revert this change. Setting back to needs review for discussion.

plach’s picture

I'd be fine with adding back to component, but I think we should keep the new interface: that still serves as a good place to provide documentation and above all will allow us to decouple our code from the component if ever needed.

plach’s picture

This is a patch implementing #75 in case we want to go that way.

Status: Needs review » Needs work

The last submitted patch, 76: vendor-symfony_translator-2546618-76.patch, failed testing.

MustangGB’s picture

Could you not have a flag that disables downloading the Symfony component by default, but which could be toggled by contrib if they want to use it, so both options would be possible without hacking core.

plach’s picture

Status: Needs work » Needs review

Ignore #76

plach’s picture

This implements the solution discussed with @catch, @dawehner and @alexpott: we do #75 and add some documentation marking the translation component as "internal".

dawehner’s picture

Looks great so far!

+++ b/core/composer.txt
@@ -0,0 +1,34 @@
+fabpot/goutte
...
+phpunit/phpunit

this is a dev dependency, let's remove it already.

plach’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you plach!

You messed up your interdiff, but nevermind

The last submitted patch, 76: vendor-symfony_translator-2546618-76.patch, failed testing.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Isn't +symfony/css-selector also dev-only?

I'd be fine with moving composer.txt to a follow-up by the way.

catch’s picture

Apparently it's also used by http-kernel, but afaik we don't use it directly anywhere.

dawehner’s picture

Good point regarding css-selector, yeah we will never run that on production.

plach’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
322 bytes
414.49 KB

I agree we should defer any other discussion around composer.txt to a follow-up.

Back to RTBC, since the change was trivial and only concerning docs.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/vendor/composer/autoload_psr4.php
@@ -12,7 +12,7 @@
-    'Symfony\\Component\\Translation\\' => array($baseDir . '/lib/Symfony/Component/Translation'),
+    'Symfony\\Component\\Translation\\' => array($baseDir . '/lib/Symfony/Component/Translation', $vendorDir . '/symfony/translation'),

The $baseDir . '/lib/Symfony/Component/Translation' looks wrong - that no longer exists.

plach’s picture

good catch!

plach’s picture

Status: Needs review » Needs work

Mmh, no

dawehner’s picture

Needs work

Mh?

Do you need composer to regenerate those files directly?

The last submitted patch, 90: vendor-symfony_translator-2546618-90.patch, failed testing.

The last submitted patch, 90: vendor-symfony_translator-2546618-90.patch, failed testing.

plach’s picture

plach’s picture

Status: Needs work » Needs review
+++ b/core/vendor/symfony/translation/TranslatorBagInterface.php
@@ -0,0 +1,31 @@
diff --git a/core/lib/Symfony/Component/Translation/TranslatorInterface.php b/core/vendor/symfony/translation/TranslatorInterface.php
similarity index 100%
rename from core/lib/Symfony/Component/Translation/TranslatorInterface.php
rename to core/vendor/symfony/translation/TranslatorInterface.php
diff --git a/core/vendor/symfony/translation/Writer/TranslationWriter.php b/core/vendor/symfony/translation/Writer/TranslationWriter.php

If you are wondering why the TranslatorInterface is not removed, here's why.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The hash is the same as I got locally when I tried it out.

alexpott’s picture

Title: Replace the Symfony Translation component with a custom interface since we don't use it » Document that we don't depend on the Symfony Translation component
Status: Reviewed & tested by the community » Fixed

Committed e3d0b79 and pushed to 8.0.x. Thanks!

  • alexpott committed e3d0b79 on 8.0.x
    Issue #2546618 followup by plach: Document that we don't depend on the...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.