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
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. |
Comment | File | Size | Author |
---|---|---|---|
#95 | vendor-symfony_translator-2546618-95.patch | 414.81 KB | plach |
Comments
Comment #2
alexpottDid we try to file an upstream patch to decouple validator and translation - it seems unnecessary?
Comment #3
catchComment #4
dawehnerLet's see what they answer: https://github.com/symfony/symfony/issues/15714`
Comment #5
dawehnerLet's see what they answer: https://github.com/symfony/symfony/issues/15714`
Comment #6
xjmDiscussed 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.
Comment #7
heddnIs 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.
Comment #8
dawehnerI thought we just committed the interface ...
Comment #9
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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: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.
Comment #10
xjmSo 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?Comment #11
webchickThat'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.
Comment #12
webchickHeh. 79 comments on the won't fixed "allow comments in composer.json" issue https://github.com/composer/composer/issues/1988 ;)
Comment #13
plachWhat about extending the interface and putting the documentation there?
Comment #14
plachThis 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.
Comment #15
catchbojanz 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.
Comment #16
dawehnerSo the plan is to a) Use replace b) Use a custom interface c) Profit
Comment #17
xjm#15 sounds quite sensible to me.
Comment #18
webchickUpdating the title based on the plan. Also moving out of documentation.
Comment #19
plachI will work on this tomorrow if none beats me to it
Comment #20
dawehnerWorking in bed.
Comment #21
dawehnerThe interface documentation certainly needs improvements.
Should we also put a README maybe pointing to the documentation in the interface into the new directory?
Comment #23
dawehnerReroll + a review patch.
Comment #27
dawehnerThis patch is proudly presented by --binary
Comment #30
dawehnerLet's see
Comment #31
dawehnerThese are the files I wanted to upload
Comment #36
webchick#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.
Comment #37
plachWorking a bit on this
Comment #38
klausiwhy 2.4? Shouldn't we be on Symfony 2.7?
Comment #39
plachThis should fix the failures, we just had a few wrong namespaces.
Comment #41
dawehnerThe reason I went with that value was that symfony validator also just needs that particular version.
Should we expand the documentation a bit here?
What about adding a README.txt here?
Comment #43
catch#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.
Comment #44
plachRerolled with
--binary
Comment #47
plachAlso wrong file name :(
I tried to address #41 - #43, let me know :)
Comment #48
dawehnerThe readme looks fine for me! Now we just need to have a patch which actually passed.
Comment #49
dawehnerMh
Comment #50
Gábor HojtsyTagging with all the right D8MI tags too.
Comment #52
dawehnerWow, DrupalCI actually doesn't see segmentation faults :(
Comment #53
plachTest is green here, re-testing.
Comment #54
plachComment #55
plachCreated #2565669: Update Twig to 1.21.2 while working on this.
Comment #56
dawehnerYeah I guess it will be green.
@plach
Better reupload a patch in case we introduced random failures.
Comment #58
plachReuploading
Comment #59
chx CreditAttribution: chx commentedHere'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
.)Comment #60
chx CreditAttribution: chx commentedComment #61
dawehneradded beta eval.
Comment #62
bojanz CreditAttribution: bojanz at Centarro commentedLooks great!
Comment #64
plachIt seems we need also the
IdentityTranslator
...Comment #65
plachThis should fix the last failing test, however currently the
ValidatorBuilder
will fatal if no translator is set because it will try to instantiateIdentityTranslator
. 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 theIdentityTranslator
in theSymfony\Component\Translation
namespace extending ourDrupalTranslator
(ew).Comment #66
dawehnerNitpick: Any reason to not use $id but rather $string?
Comment #67
plach$id
is the name of the parameter in the interface.Comment #68
chx CreditAttribution: chx commented> 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 thePropertyAccess
namespace/component which we did not include in HEAD. So: no new problems introduced by this patch.Comment #70
catchYes 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!
Comment #72
alexpottJust 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?
Comment #73
Gábor HojtsyRemoving from D8MI sprint.
Comment #74
alexpottAfter 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.
Comment #75
plachI'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.
Comment #76
plachThis is a patch implementing #75 in case we want to go that way.
Comment #78
MustangGB CreditAttribution: MustangGB commentedCould 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.
Comment #79
plachIgnore #76
Comment #80
plachThis implements the solution discussed with @catch, @dawehner and @alexpott: we do #75 and add some documentation marking the translation component as "internal".
Comment #81
dawehnerLooks great so far!
this is a dev dependency, let's remove it already.
Comment #82
plachfixed, thanks
Comment #83
dawehnerThank you plach!
You messed up your interdiff, but nevermind
Comment #85
catchIsn't +symfony/css-selector also dev-only?
I'd be fine with moving composer.txt to a follow-up by the way.
Comment #86
catchApparently it's also used by http-kernel, but afaik we don't use it directly anywhere.
Comment #87
dawehnerGood point regarding css-selector, yeah we will never run that on production.
Comment #88
plachI 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.
Comment #89
alexpottThe $baseDir . '/lib/Symfony/Component/Translation' looks wrong - that no longer exists.
Comment #90
plachgood catch!
Comment #91
plachMmh, no
Comment #92
dawehnerMh?
Do you need composer to regenerate those files directly?
Comment #95
plachThis should be better, interdiff is with #88.
Comment #96
plachIf you are wondering why the
TranslatorInterface
is not removed, here's why.Comment #97
dawehnerThe hash is the same as I got locally when I tried it out.
Comment #98
alexpottCommitted e3d0b79 and pushed to 8.0.x. Thanks!