Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
- Patches involving Symfony components are difficult to review and the sheer size of the patches hide essential changes to Drupal core from prying eyes.
- There's no community consensus on whether we ultimately want to rely on and use Symfony components yet, so it's not guaranteed that the change proposal depending on it will succeed for D8.
Goal
- Commit the library, and then show us how you want to use it.
- If you fail to use it in an accepted way, we remove it.
Details
- Upcoming patches for the Web services and Layout initiatives require Symfony components to make their envisioned design work.
- Most of that envisioned architectural design was discussed at a sprint in Boston recently. — This means nothing in terms of general community acceptance for the actual proposed changes. But it is a clear indicator for what's needed in terms of external libraries, and how the involved developers are going to implement that vision.
- #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel and potentially other issues are going to propose architectural changes for Drupal core based on those external libraries.
- Each of those patches is 500+ KB in size, since all of the dependencies are missing and need to be contained in each patch, too.
- The actual changes to Drupal core are much smaller.
- By committing the external libraries first, everyone can properly review all changes in each patch in detail.
- We have ~8 months left to remove unused code from Drupal core.
- The mere fact that the external library code exists is not a guarantee that we will use it, nor that it will stay.
- We apparently did the identical thing with jQuery UI. We merely failed to remove it before we released D7.
- We are not going to touch or change the code of the external library within Drupal core.
- No one is going to review the external library code as part of those core patches either way.
If you want to read and learn about that code, you go to the library's repository or API documentation pages instead. Simply looking at your D8 checkout would be even better. - Hence:
Commit that symfony component code to core. It's not used by the mere commit.
No sandbox involved. No patch involved.
It reduces the patches in those issues to the actual changes to Drupal core.
Thus, patches are reviewable and manageable, by everyone.
We can remove unused code at any time. - This very issue will come up again for every single new patch that's posted to any issue for the affected initiatives. Let's solve it once for all. For the time being.
Comment | File | Size | Author |
---|---|---|---|
#22 | symfony-dependency-injection.patch | 298.41 KB | effulgentsia |
drupal8.httpkernel.0.patch | 425.03 KB | sun | |
Comments
Comment #1
bojanz CreditAttribution: bojanz commentedI'm hoping that after HttpKernel, in time for the next dependency to be added we'll have done #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo), which would mean not bundling Symfony at all (and not having stupid issues like #1343160: Update Symfony2 components to latest release).
Comment #2
pounard+1
Comment #4
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedYeah, good idea, the size of
thatone of the patches implementing Symfony made me close it. I opened it just to explore the actual changes to Drupal, but wading through all the Symfony code made it feel like work, and less like a pass-time.Comment #5
bojanz CreditAttribution: bojanz commentedI actually find it nice to have the parent classes in the patch as well, I can jump and see what the logic is there as well, instead of just accepting anything in the Symfony namespace as a black box. But I'm clearly in the minority ;)
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt seems that it is time to use Git submodules here.
Comment #7
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedOr place the Symfony code at the bottom of the patch.
Comment #8
sundrupal8.httpkernel.0.patch queued for re-testing.
Comment #9
sun@bojanz + @Damien Tournoud: Can we focus on the actual scope of this issue? Both composer and submodules are kinda vaporware to me at this time, as long as there's no definite answer to #1451056: [policy] How to handle unforeseen diversion of Symfony code in stable/API-locked Drupal core?. Would be good to keep further comments on those topics on that issue or alternatively #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo), as this one will hopefully bite the dust very soon.
Comment #10
bojanz CreditAttribution: bojanz commentedI did say "After HttpKernel", meaning after this patch. I have no problem with this issue proceeding.
Comment #12
sunTestbot hiccup.
Comment #13
Crell CreditAttribution: Crell commentedsun: It really seems like this is splitting the discussion into too many issues. No one is going to keep up with them.
For me, Composer is a go. Let's just do it and then this problem goes away. The sooner we do so, the sooner this problem goes away. We can role two-part patches for the time being until then, as long as it's a small period of time. Meaning, "stop talking and go code Composer support!"
Comment #14
sunThe point of this issue is to commit almost half a megabyte of PHP code to Drupal core that isn't used - for the sake of easing and focusing on the actual changes to Drupal core in reviews.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedIf the Composer patch lands first, then reroll or close this as appropriate. Meanwhile, RTBC. If a committer can get to this issue before the Composer one is RTBC, then I see no reason not to commit this one first, and then reroll the Composer patch accordingly. This just adds 3 Symfony components: HttpKernel, EventDispatcher, and Routing. Getting this in will help with reviews and getting to a successful bot pass with #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel, and I see no down side.
Comment #16
chx CreditAttribution: chx commentedThere's no reason not to commit this. git rm, should we decide on it later is cheap :)
Comment #17
Crell CreditAttribution: Crell commentedsun, are these the 2.0 or 2.1-snapshot versions of the code? The 2.0 won't work, as the Drupal kernel patch depends on the 2.1 snapshots.
Comment #18
sunThe patch contains the same files as contained in #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel
Comment #19
Crell CreditAttribution: Crell commentedOK, that should work fine then. (That's 2.1 snapshot as of about 2 weeks ago.)
Comment #20
catchOK then, committed/pushed to 8.x. We can switch to composer if/when that issue is ready, we can take things back out if/when it turns out they're not being used. Thanks!
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedHere is just Symfony's Dependency Injection component from #1497230-18: Use Dependency Injection to handle object definitions . I ran "diff -r" on this to compare with a download of https://github.com/symfony/symfony/tree/master/src/Symfony/Component/Dep..., and found them to be identical. For the same reason as with the kernel, I think the dependency injection issue will be helped by being able to post updated patches that contain Drupal code only, and not 300K of Symfony code. Let's commit this under the assumption that Symfony's dependency injection component will work for us; we can back it out later if that turns out to be incorrect.
I'm RTBCing this on the grounds that I didn't write any of this code, and this patch only contains what is already in #1497230-18: Use Dependency Injection to handle object definitions .
Comment #23
chx CreditAttribution: chx commentedSo then why not #1492916: Do not stop at adding Poormanscron under the same logic? Remember, we have ~8 months left to remove unused code from Drupal core. Yes. I am a jackass. And no, I do not agree with all this "let's throw it in core and let's hope it sticks". We havent been great removing stuff from core.
Comment #24
RobLoachHaving a dependency injection container is absolutely the right solution for us, and one that we desperately need to help:
Don't get me wrong though, I am all for removing code from Drupal, but there are better solutions out there than having global and static variables everywhere.
Copying and pasting all these components directly in our git repository is probably the worst way to handle this, but it will have to do for now. We need a Drupal build process to bring in these dependencies and possibly do other stuff like .min-ify our CSS/JS, and #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo) can let us do that kind of thing.
Comment #25
chx CreditAttribution: chx commentedYes, and a 300KB patch that we are not even sure we will use or how we will use is the best we can come up with for this purpose?
Comment #26
pdrake CreditAttribution: pdrake commented#22 is related to an active issue with patches that show how we will use functionality provided by Symfony\Component\DependencyInjection\Container. If we will use it, I suppose, depends on whether that patch ends up getting accepted, though I hope it does for the reasons listed above.
Comment #27
chx CreditAttribution: chx commentedSure it gets accepted, I have not even moved it out of RTBC.
Comment #28
marvil07 CreditAttribution: marvil07 commentedMaybe it is a good time to ask users to use pear for this instead of adding it to drupal git repository.
Please notice that Symfony Components are available through a pear channel.
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedWe need something that integrates correctly with testbot. As per #24, Composer (#1424924: Use Composer for updating Symfony components (without removing Symfony code from repo)) appears to be the most favored solution at the moment, but until that work is complete, or someone can argue for why some other solution, like pear, is better, committing to the git repo is the only thing that works.
This isn't about Drupal code that someone once thought useful but maybe can be refactored or moved to contrib. This is about a vendor library, from a project we've already decided to use some components from. What's an example of this kind of thing that we failed to clean up? However, using another project's PHP components is somewhat new territory for us, so adding the "revisit before release" tag to ensure we remove any unused components prior to release.
Have you changed your mind about process in general since #16, or do you feel that the dependency injection component is less justified for some reason than the kernel component? Again, using another project's PHP components is new territory for us, so it's ok for us to still be flailing about process. My opinion is that if someone is contributing serious patches towards improving Drupal, and those patches rely on a Symfony component, that we give that person the benefit of the doubt, and commit the Symfony component (or in the future, add it to composer.json or whatever), and allow that issue's review/tweak/review/tweak process to proceed as efficiently as possible.
Thanks. I hope that if you really thought this component wasn't appropriate for Drupal, you would change the status.
Comment #30
webchickI'm also comfortable with a "revisit before release" approach here. I really don't think there's a huge risk in forgetting to use this code.
Comment #31
RobLoachRegarding #22, we should probably just bring Dependency Injection in with #1497230: Use Dependency Injection to handle object definitions itself. Setting this to postponed for later discussion.
Comment #32
sunComment #32.0
sunLess aggro.
Comment #33
catchAssetic is not used in core, and only will be if #1762204: Introduce Assetic compatibility layer for core's internal handling of assets gets committed. That's currently critical, however if it doesn't make it, then we should remove Assetic from core.
Opening this back up, and marking postponed.
Comment #34
catchNope #1784774: Remove Assetic component from core handles that.
Comment #35
webchickWe covered this in #2485109: Remove any un-used external dependencies.