Review external library dependencies in core

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

+1

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.

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

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

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

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

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

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.

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

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.

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

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!

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 .

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.

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.

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?

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

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

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.

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.

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.

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.

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.

Nope #1784774: Remove Assetic component from core handles that.

We covered this in #2485109: Remove any un-used external dependencies.