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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz’s picture

I'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).

pounard’s picture

+1

Status: Needs review » Needs work

The last submitted patch, drupal8.httpkernel.0.patch, failed testing.

Tor Arne Thune’s picture

Yeah, good idea, the size of that one 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.

bojanz’s picture

I 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 ;)

Damien Tournoud’s picture

It seems that it is time to use Git submodules here.

Tor Arne Thune’s picture

Or place the Symfony code at the bottom of the patch.

sun’s picture

Status: Needs work » Needs review

drupal8.httpkernel.0.patch queued for re-testing.

sun’s picture

@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.

bojanz’s picture

I did say "After HttpKernel", meaning after this patch. I have no problem with this issue proceeding.

Status: Needs review » Needs work

The last submitted patch, drupal8.httpkernel.0.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

Testbot hiccup.

Crell’s picture

sun: 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!"

sun’s picture

The 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.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

If 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.

chx’s picture

There's no reason not to commit this. git rm, should we decide on it later is cheap :)

Crell’s picture

sun, 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.

sun’s picture

The patch contains the same files as contained in #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel

Crell’s picture

OK, that should work fine then. (That's 2.1 snapshot as of about 2 weeks ago.)

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK 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!

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

Status: Closed (fixed) » Reviewed & tested by the community
FileSize
298.41 KB

Here 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 .

chx’s picture

Let's commit this under the assumption ... we can back it out later if that turns out to be incorrect.

So 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.

RobLoach’s picture

Having a dependency injection container is absolutely the right solution for us, and one that we desperately need to help:

  1. Remove all our global variables
  2. Fix all our static instances so that they have correct scope
  3. Intelligently lazy load objects in memory, like the Kernel, and maybe even the Plugin Manager

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.

chx’s picture

Yes, 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?

pdrake’s picture

#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.

chx’s picture

Sure it gets accepted, I have not even moved it out of RTBC.

marvil07’s picture

Maybe 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.

effulgentsia’s picture

Issue tags: +revisit before beta

ask users to use pear

We 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.

We havent been great removing stuff from core.

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.

And no, I do not agree with all this "let's throw it in core and let's hope it sticks"

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.

I have not even moved it out of RTBC

Thanks. I hope that if you really thought this component wasn't appropriate for Drupal, you would change the status.

webchick’s picture

I'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.

RobLoach’s picture

Status: Reviewed & tested by the community » Postponed

Regarding #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.

sun’s picture

Status: Postponed » Closed (fixed)
sun’s picture

Issue summary: View changes

Less aggro.

catch’s picture

Title: Commit external library dependencies to Drupal core » Review external library dependencies in core
Status: Closed (fixed) » Postponed
Issue tags: -revisit before beta +revisit before release candidate

Assetic 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.

catch’s picture

Status: Postponed » Closed (fixed)
webchick’s picture

Issue tags: -revisit before release candidate