Problem/Motivation

Upgrading the components of Symfony within Drupal Core can become cumbersome as it means checking out the latest version and manually replacing the existing components.

Proposed resolution

Use Composer to manage downloading and updating the Symfony components within Drupal Core when we need to perform an upgrade of Symfony's components. To update the Symfony Components, edit composer.json with the new version numbers, then do a Composer install:

cd core
curl -s http://getcomposer.org/installer | php
php composer.phar install

All components are updated cleanly, and any new dependencies are resolved. The proposed patch here does not remove the Symfony components, but allows us to manage them via composer.json. End users and developers don't have to worry about running Composer.

Remaining tasks

The patch needs to be reviewed and committed. The following issues are related:

Drupal Core
#1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead
Drupal.org
#1503234: Process Composer components when building Drupal core
Drush
#1503240: Process composer.json during core's make()
#1316322: Add PSR-0 autoloader to drush

User interface changes

No user interfaces are changed.

API changes

No API changes.

CommentFileSizeAuthor
#128 1424924-formatpatch.patch1.35 MBRobLoach
#126 1424924-formatpatch_4.patch1.34 MBRobLoach
#124 1424924-diff.patch1.31 MBRobLoach
#124 1424924-formatpatch.patch1.34 MBRobLoach
#120 composer.update.120.patch512.62 KBsun
#114 1424924.patch522.53 KBRobLoach
#110 1424924-110.patch535.68 KBRobLoach
#108 1424924-108.patch536.24 KBRobLoach
#103 1424924-yaml.patch598 KBRobLoach
#102 1424924.patch573.51 KBRobLoach
#100 1424924.patch572.64 KBRobLoach
#99 1424924.patch353.02 KBRobLoach
#75 1424924.patch1.88 MBRobLoach
#73 1424924.patch1.88 MBRobLoach
#71 1424924.patch1.88 MBRobLoach
#69 1542710.patch1.88 MBRobLoach
#67 1542710.patch1.05 MBRobLoach
#62 1424924-62.patch291.7 KBRobLoach
#60 1424924-60.patch291.76 KBRobLoach
#59 1451524.patch291.19 KBRobLoach
#58 1451524.patch647.86 KBRobLoach
#54 1424924-diff.patch214.55 KBRobLoach
#51 1424924-diff.patch214.55 KBRobLoach
#42 1424924.patch50.91 KBRobLoach
#40 1424924.patch51.48 KBRobLoach
#38 1424924-diff.patch51.86 KBRobLoach
#36 1424924-diff.patch50.54 KBRobLoach
#34 1424924-diff.patch64.54 KBRobLoach
#23 1424924-diff.patch24.92 KBRobLoach
#23 1424924-formatpatch.patch32.73 KBRobLoach
#17 1424924-diff.patch69.58 KBRobLoach
#17 1424924-formatpatch.patch77.18 KBRobLoach
#8 1424924-diff.patch69.66 KBRobLoach
#8 1424924-formatpatch.patch77.26 KBRobLoach
#6 1424924-diff.patch199.29 KBRobLoach
#6 1424924-formatpatch.patch218.24 KBRobLoach
#4 1424924-diff.patch99.08 KBRobLoach
#4 1424924-formatpatch.patch107.55 KBRobLoach
composer-formatpatch.patch452.04 KBRobLoach
composer-diff.patch438.13 KBRobLoach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

I'm fully OK with using composer to pull on the Symfony components, but why are there so many additions in this patch? Wouldn't we then want to leave the components out of our repo entirely and compose them as needed? (We'd also have to update testbot and the packager to run composer.phar first before zipping up, which I'm also fully OK with.)

Also, please add

[diff]
renames = copies

to your .gitconfig file. (Hat tip to sun for pointing that out to me. Not sure if it will help here, but it may.)

RobLoach’s picture

Status: Needs review » Needs work

Thanks for the renames = copies note :-).

  1. Move composer.json into /core/
  2. Remove the vendor-dir config change
  3. Add Drupal's auto-load /core/lib/ stuff into composer.json
  4. Update drupal_classloader() to be more similar to the Composer's auto-generated autoload.php to simplify the upgrade process
Crell’s picture

I assume there's a patch coming here... :-)

Actually I'm thinking that we remove everything from vendor, add composer.json to the top-level, and then just include the composer-generated class loader. (We may want to still put composer.json inside vendor; I'm not sure. Experimentation required.)

I don't know if it makes sense to use composer for our own stuff, though. Isn't it more intended for 3rd party code? Seems weird to use it that way.

RobLoach’s picture

FileSize
107.55 KB
99.08 KB

This patch has composer.json in /core/ without any of the Drupal project meta-data in it. You might be right about having it in the top-level. The Composer project is now up and running, so you can run:

/core/ $ drush dl composer
/core/ $ drush composer install

I do like your idea of using the composer-generated class loader directly. Having composer.json inside vendor wouldn't work though, as we need to reference /core/lib for the Drupal/Core namespace.

RobLoach’s picture

Status: Needs work » Needs review
RobLoach’s picture

FileSize
218.24 KB
199.29 KB
  • Has composer.json in /composer.json rather than /core/composer.json
  • Uses Composer's generated autoload.php itself
  • Includes Symfony components from Symfony 2.1.0

In future patches, to upgrade components, all you have to do is change the version numbers in composer.json accordingly, and then run:

drush dl composer
drush composer install
sun’s picture

Status: Needs review » Needs work
copy from core/vendor/Symfony/Component/HttpFoundation/Cookie.php
copy to core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Cookie.php

I really don't like how this hi-jacks the entire PSR-0 directory structure.

I semi-understand that this is how Composer works, and that said, I actually like that it is treating each component as a self-contained package/library, but in the end, the way it puts the individual packages into the filesystem totally contradicts the entire painful PSR-0 discussions we had until now.

Lastly, I don't really see how one would be able to use the APC-based class loader with the auto-generated file of Composer.

Least to say that the stripped PHP file is unreadable.

Also, I don't understand how we'd register our Drupal namespace fallbacks with this.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
77.26 KB
69.66 KB

This patch...

  1. Provides additional code documentation on how to register additional namespaces with the class loader
  2. Since we now have Composer's ClassLoader, we don't need Symfony's. Provides the same feature-set, and the documentation outlines that.
  3. Updates to the stable Symfony 2.0.10 rather than the development 2.1.* branch
RobLoach’s picture

I don't understand how we'd register our Drupal namespace fallbacks with this.

$loader = drupal_classloader();
$loader->add("Drupal\\SomeFallbackNamespace", array("some/fallback/directory", "other/fallback/directory"));
$loader->register();

What fallback directories would we be requiring? I can see us registering module /lib directories later on in the bootstrap.

I semi-understand that this is how Composer works, and that said, I actually like that it is treating each component as a self-contained package/library, but in the end, the way it puts the individual packages into the filesystem totally contradicts the entire painful PSR-0 discussions we had until now.

It does look quite confusing at first, but when you think about it it makes sense. This is vendor-based PSR-0 auto-loading. Since components define their own "autoload" parameters, it's actually better for us to let them tell us what namespaces the components register. Take a look at HttpFoundation's composer.json. Having the component itself define its own autoloading requirements means it takes the work off our backs. It means there won't be namespace and vendor overlap conflicts when lots of vendor components are loaded.

sun’s picture

Can't you make it so that it puts the files where they should be according to our PSR-0 directory structure?

Crell’s picture

PSR-0 doesn't mandate that all code start from the same root; just that once you get TO a root, you have the full class name map to the file system. You can have as many roots as you want, especially for different namespaces. This is something that is totally not obvious about PSR-0 at all, and confused me for a long time, too.

For each top-level namespace, we tell the autoloader (whether it's Symfony's, Composers, or anything else PSR-0 compliant):

- For classes that start with Foo, look in directory A.
- For classes that start with Bar, look in directory B.
- For classes that start with Foo\Baz, look in directory C.

There is absolutely no requirement that directories A and B be next to each other, nor that directory C be a subdirectory of directory A.

So with composer, it downloads each package in "whatever directory structure it has", and the composer.json file within that package says "For classes starting with X, look in directory Y". Composer then aggregates all of that into a single generated class loader.

(sun: This is what I was talking about in IRC a while back about the directory structure getting weirder with Composer, but me not caring about that. It doesn't have any impact on performance, and no one should be editing code in there anyway.)

None of that, actually, has to interact with our own class loader for Drupal code. There is nothing preventing us from having multiple class loaders registered. I'd actually prefer to continue using the Symfony class loader for our own code, at least for now, and let Composer do its thing for vendor code (Symfony, and anything else like that we pull in like Zend_Feed or Buzz).

I am also not too worried right now about APC, since that just caches filestat lookups, which if we're smart we won't have many of anyway. Plus, if you really want to trick out your performance you'll install APC and use some sort of aggregator mechanism for production anyway so the code is already in memory and the class loader is never hit in the first place. See: #1241190: Possible approaches to bootstrap/front loading of classes

Crell’s picture

Cross posted. Re #10: What we have in core right now is /lib/ for our code, and /vendor/ for 3rd party code, done manually. Using composer to automate /vendor/ changes the exact directory structure within /vendor/, but is still 100% PSR-0 compatible and has no performance impact since the class loader has to be registered for each top level namespace anyway. So "in /vendor/ somewhere" is valid according to "our PSR-0 directory structure".

In short: Let go, let the code do its job, it'll be OK. :-)

(I know this is somewhat confusing. As I said, it took me a while to figure out, too, because PSR-0 was defined and documented so horridly.)

sun’s picture

Still, this is muddying waters.

I don't see why Composer wouldn't be able to copy the files into a location that retains the clean directory structure we want to use.

I'd even put it into an extreme and will say that if Composer is not able to do just that, then it's the wrong tool for the job.

It's a tool to fetch a package (and its dependencies) and put that into a desired target location. That's it.

no one should be editing code in there anyway.

I totally know that this is absolutely the wrong issue to state this, but that absolute statement is problematic and has a very good chance to not hold water. Here's why:

  1. We're pulling in code from an external software project. Both that project as well as Drupal claim to retain API compatibility in stable branches.
  2. We're going to update the external software to its latest minor/point release when we're about to create a new minor/point release ourselves.
  3. As we've learned at the WSCCI sprint, we should be safe to use any component that is tagged with @api (or similar).
  4. If we find any issues, we'll surely file a patch upstream.

So far, so good. But this train of thought has to be continued:

  1. Symfony is not a dumb frontend library like jQuery. We're talking about bare essential code that is going to drive Drupal at its core.
  2. If we find a security issue, then we'll attempt to file a patch upstream.
  3. As you know, security issues require ugly hacks or even API changes at times, so there's a pretty good chance that the fix we're filing upstream won't be accepted for various reasons, or will only land for the next major version.
  4. At the same time, we likely still want to update to newer minor/point releases of the external software, because they are likely to fix minor bugs down the line (which equally may affect our core code or contrib code).
  5. This inherently means that we might have to touch our copies, and actually have to merge in upstream changes instead.
  6. In the end, there's absolutely no guarantee that we will be able to always use those components as-is, for the entirety of the next ~5+ years.

This is a fact. Even more so, since we never did something like this before. Whoever argues against that argues on the base on hypothetical assumptions, which may or may not be true. You can happily point me to this comment in 5 years and tell me that I was wrong. But until that future point in time has elapsed, this boils down to pure risk management. And there is a 99% risk in my book.

Again, this is not about jQuery or jQuery UI libraries. They're not comparable, at all.

Apparently, one of the Symfony developers told me in IRC last night that there actually have been backwards-incompatible changes to components flagged as @api in some minor/point releases. That's a topic which comes actually on top of what I outlined above, because the API change is reversed; it originates from upstream instead of downstream.

--

In the end, I wonder whether we wouldn't run much safer and better by using git-subtree for the Symfony components. The git-subtree approach provides full flexibility in a possibly mixed upstream/downstream scenario like this.

Also, if I understood Symfony devs correctly, then they're actually using git-subtree themselves to split the components into separate repositories.

(that said, they already clarified that those component repos are only one-way downstream; all changes happen in the main upstream Symfony repo, so, in fact, they're not leveraging the full power of git-subtree, and in order to patch a component, you need to patch the main repo. — Speaking of, the root cause for having to patch the main repo instead of the component's repo is that the components are not self-contained; the component repos do not contain the respective tests for the component. The tests are only contained in the main repo. In turn, this means we won't ever have tests for Symfony components, unless we pull them in manually. And yes, that topic of not self-contained components really bugs me. The answers to my questions for why that is were only based on the [simplified] reasoning that Symfony's test framework does not really support component dependencies currently, and thus, tests are simply maintained in the main repo, because the main repo can make the magic/safe assumption that all components exist, and thus, all dependent components exist. And that said, none of the answers actually provided a valid reasoning for why tests are located outside of the component, instead of being part of the component. Assuming that all components exist is one thing, but moving tests out of the respective component is a completely different thing. A dependent project like Drupal can very easily cope with the former, but the latter is almost unmanageable from here.)

Crell’s picture

Symfony uses subtree-splits to spin the components OUT of the main repository. git-subtree is (as I understand it), a way to suck external code INTO a repository. Subtly differen things, I think. I'm actually find with Symfony's code not physically living in our repository as long as it's trivially added, especially if that's something other projects are doing.

I don't see why Composer wouldn't be able to copy the files into a location that retains the clean directory structure we want to use.

That's where I disagree. I really do not care why the precise internal structure of the /vendor/ directory matters so much. There is no performance difference. There is no significant difference in autoloader complexity. I don't even see much of an aesthetic difference. So, who cares? I don't. (And I'm normally the major pedant around here. :-) )

(I also completely disagree that not having Symfony's PHPUnit tests, which we cannot run anyway, in our repository is "almost unmanageable". But either way, that's what Symfony does and we cannot change that, so it's a moot point.)

Seldaek’s picture

@sun, a few comments (from a perhaps biased Composer lead dev:)

Lastly, I don't really see how one would be able to use the APC-based class loader with the auto-generated file of Composer.

The plan for composer is to provide a class-map dumper that can generate an optimized map (array of class=>path couples) class loader. Once you have that, APC class loader shouldn't even be required, but if there is a real need for it, we can obviously provide that as well. My goal/hope is that nobody should have to bother with autoloaders ever again.

Least to say that the stripped PHP file is unreadable.

The source is on github https://github.com/composer/composer/blob/master/src/Composer/Autoload/C... - I agree it's not super nice that it's dumped as garbage, but it keeps the phar size down a bit, and typically people shouldn't have to look into this directory.

Can't you make it so that it puts the files where they should be according to our PSR-0 directory structure?

This is pretty much impossible for a few reasons:

- Handling package updates would be a massive pain since we would have to keep track of which file came from where.
- You could not use git clones anymore (which you can right now with install --prefer-source) since the directories wouldn't be left intact. This is a neat feature for working "in-line" on libs/vendors.
- Some packages might depend on files outside of their PSR-0 autoloadable files, be it for good or bad reasons, this would likely break them.

This inherently means that we might have to touch our copies, and actually have to merge in upstream changes instead.

If you really have to patch a vendor, one option is to add a repository to composer (see http://packagist.org/about-composer) to have your own forked version of the package override the default one. You can ship your patched version to everyone that way.

Finally regarding symfony components and their tests, I don't really see the problem, but in the long term we could envision that the component packages are built using a script instead of automatically extracted from github via packagist as they are now. The script could move appropriate tests etc in the dir before packaging. I have no idea if it will happen, I'm just saying there is a potential solution.

RobLoach’s picture

Status: Needs review » Needs work

We should actually have composer.json in /core, so that users arn't inclined to edit it.

RobLoach’s picture

Status: Needs work » Needs review
Issue tags: +Framework Initiative
FileSize
77.18 KB
69.58 KB
  • Moved composer.json to /core so that users don't mess with it
  • Updated references accordingly

It's looking pretty good, we should get this in sooner than later, so that we can get follow up patches in #1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch]. Adding the Framework Initiative tag as sun mentioned PSR-0 was part of the Framework WSCCI. This definitely makes it easier to manage our Symfony dependencies.

Crell’s picture

I still don't get why this patch is removing the Symfony class loader entirely. Why wouldn't we be using that for non-composer-downloaded code, ie, almost everything that's not Symfony?

RobLoach’s picture

We can use Composer's ClassLoader.php to register namespaces, as well as fallbacks. The generated autoload.php already registers Drupal\Core and Drupal\Components, which are shipped with Drupal Core. It's easy to add additional ones via composer.json.

As for things that arn't shipped with Drupal Core, we can register the namespaces manually, or via fallbacks:

$loader = drupal_classloader();
$loader->add("Drupal\\MyModule", drupal_get_path('module', 'mymodule') . '/lib/');
$loader->add("Drupal\\SomeFallbackNamespace", array("some/fallback/directory", "other/fallback/directory"));
$loader->register();

Is there anything that we're missing here from Symfony's ClassLoader here? The above would have to be in a follow up patch.

RobLoach’s picture

Status: Needs review » Postponed
sun’s picture

Crell’s picture

Status: Postponed » Needs review

The DBTNG conversion is in. Back to active.

RobLoach’s picture

  • Updates to the latest now that the DBTNG conversion and Symfony 2.0.10 update is in.
  • Does not remove Symfony's Class Loader, but still returns an instance of it. The Symfony class loader isn't needed or used (as we could just use Composer's instead), but let's move that discussion to a follow-up issue.

EDIT: Updated the issue summary.

RobLoach’s picture

Issue summary: View changes

Updated summary

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Status: Needs review » Needs work
+++ b/core/vendor/.composer/autoload.phpundefined
@@ -0,0 +1,20 @@
+if (!class_exists('Composer\\Autoload\\ClassLoader', false)) {
+    require __DIR__.'/ClassLoader.php';
+}

A coding style nit... There needs to be spaces on both sides of the . after __DIR__.

There are several other lines in this patch that similarly need to be adjusted.

RobLoach’s picture

Status: Needs work » Needs review

autoload.php conforms to Symfony's coding standard rather than Drupal's. We shouldn't be editing that file though as it is generated for us by Composer. Running a "composer install" will replace any changes we make in there ;-) .

Lars Toomre’s picture

Thanks for the explanation Rob.

webchick’s picture

Can someone advocating for this solution please explain over at #1451056: [policy] How to handle unforeseen diversion of Symfony code in stable/API-locked Drupal core? how this will work in practice if we need to patch the Symfony components locally in order to shield our users from a BC break or similar? I don't even build Drupal websites sucking in blindly from upstream. :)

sun’s picture

Status: Needs review » Needs work

@Lars Toomre: That code is auto-generated and seems to follow Symfony's coding style. I agree it's ugly though, and already wondered myself whether there'd be a chance to discuss coding standards with Symfony guys.

Anyway, for this patch, I see the following blockers:

  1. #1451056: [policy] How to handle unforeseen diversion of Symfony code in stable/API-locked Drupal core?
  2. #1290658-144: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch]
  3. Unstripped Composer ClassLoader.php.
RobLoach’s picture

Status: Needs work » Needs review

how this will work in practice if we need to patch the Symfony components locally in order to shield our users from a BC break or similar

We will not patch Symfony locally to protect from BC breaks, as that means maintaining our own version of Symfony. This would result in a monstrous amount of support problems when things don't work with additional Symfony components. We do not have the capacity to maintain something like that. Instead, we would do something like #1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch], and use the autoloader to depict which classes and namespaces to use. They are different discussions, and work in tandem.

Dependency injection and abiding by object oriented design principles are the correct way to handle this, maintaining our own fork of Symfony is not.

RobLoach’s picture

Status: Needs review » Needs work

Unstripped Composer ClassLoader.php.

I'll open up a Composer issue to allow use of an unstripped verison of the ClassLoader.

EDIT: https://github.com/composer/composer/issues/386

Crell’s picture

Agreed with #29. Lukas Smith also pointed out (in one of these dozen issues) that if we properly use a dependency injection approach then we can simply subclass a Symfony class to change what we need to. There are several classes we'll be doing that to anyway purely for functionality reasons.

There are far better approaches than maintaining forks.

RobLoach’s picture

Status: Needs work » Needs review

Stof responded at http://github.com/composer/composer/issues/386#issuecomment-4314088 , and made a note that all PHP files are actually stripped during the Composer .phar generation to keep the phar size small. The Composer ClassLoader does what we need it to do, and if we need its source, it can be easily found at ClassLoader.php. I'd consider shipping the stripped ClassLoader.php a non-issue as it is generated and we shouldn't be editing it ourselves.

However, using the Composer source directly, it is possible to build with the unstripped ClassLoader.php:

cd core
git clone git://github.com/composer/composer.git
php composer/bin/composer install

I still consider this a non-issue though. Setting back to review. The other fore-mentioned issues can be worked on in tandem to this one. Do you think we should ship the unstripped ClassLoader.php?

RobLoach’s picture

Status: Needs review » Needs work

Composer now doesn't compress ClassLoader.php since it's user-facing, so we should update the patch to get the unstripped ClassLoader.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
64.54 KB
  • This patch updates with the unsquished ClassLoader.php
  • Removes Symfony's ClassLoader since we're using Composer's instead. If you still want to replace a class of Drupal\Core and fallback to core/lib, it is still possible with:
    $loader = drupal_classloader();
    $path = drupal_get_path('module', 'better_core') . '/lib';
    $loader->add("Drupal\\Core", $path);
    $loader->register(TRUE);
    

    Passing TRUE to register() will check the paths in the "Better Core" module before falling back to core/lib. We still should be using dependency injection for this, but I just wanted to point out that the feature-set is still there.

  • Added a bit more documentation

Back to "needs review"!

Status: Needs review » Needs work

The last submitted patch, 1424924-diff.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
50.54 KB

We should actually have the .lock file in there to prevent that from happening. Let's try this again.

The last submitted patch, 1424924-diff.patch, failed testing.

RobLoach’s picture

FileSize
51.86 KB

Found it.

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

Seldaek’s picture

Rob, the advertised method for overriding stuff in Drupal in #34 is not really ideal because you're bloating up the autoloaders space. Basically every time you re-register the same autoloader, so if it's not matching something it'll run through all its namespaces many times.

The best approach would be to just instantiate a new ClassLoader, and register that one with register(true) so it takes precedence. Or you just define your overrides in your composer.json so they're part of the core composer one.

RobLoach’s picture

FileSize
51.48 KB

Thanks for the note, Seldaek. I've updated the patch with that small doc fix.

Status: Needs review » Needs work

The last submitted patch, 1424924.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
50.91 KB
Crell’s picture

Assigned: Unassigned » dww

I'm still unclear why we're leaving the HttpFoundation code in the repo with this change, other than "we haven't updated the packager and testbot yet to run composer." If that's the only reason, we should get some input from the infra folks. Tagging one of 'em. :-)

dww’s picture

Assigned: dww » Unassigned

Sorry, I don't have bandwidth/energy to read this whole thread, contemplate the implications, and say anything useful (at least not this week). I'm now following this, so I'll continue to get pinged about this, but saying this is "assigned" to me is currently a lie. Sorry, but I can't be everywhere at once.

Seldaek’s picture

@dww: Just in case if you have questions or problems whenever you have time to look into this please stop by #composer-dev on irc.freenode.org

RobLoach’s picture

I'm still unclear why we're leaving the HttpFoundation code in the repo with this change, other than "we haven't updated the packager and testbot yet to run composer."

Until the packager and test bot handle Composer installations, HttpFoundation will have to stay in there.

Should we be updating CHANGELOG.txt here?

Crell’s picture

So you're suggesting we just use Composer as an update mechanism until that gets taken care of? That would mean patches that add new Symfony libraries would still be huge in order to satisfy testbot.

I'm OK with doing that as a stopgap as long as we open up the appropriate follow-ups to get packager and testbot working with Composer so that we can not ship that code in core.

sun’s picture

The issue summary only states updating Symfony components in Drupal core.

Why are we discussing to remove Symfony components from Drupal core (and only provide them via packaging or some other scripting tools) now? What's the benefit of doing that? (Compared to the [rather huge] negative impact of no longer being able to clone/checkout Drupal and directly use it?)

RobLoach’s picture

Why are we discussing to remove Symfony components from Drupal core (and only provide them via packaging or some other scripting tools) now? What's the benefit of doing that? (Compared to the [rather huge] negative impact of no longer being able to clone/checkout Drupal and directly use it?)

He's talking about removing HttpFoundation from the git repository, and then having Composer add them when "building" Drupal. I'm kind of against that at this stage, as it would mean require running drush composer install after checking out Drupal's git, but am willing to put it up the issue for it:
#1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead

For this patch though, I think we should just leave it in.

RobLoach’s picture

Status: Needs review » Needs work

We must update with EventDispatcher, HttpKernel, and Routing!

RobLoach’s picture

Status: Needs work » Needs review
FileSize
214.55 KB

This patch includes 2.0.10 of EventDispatcher, HttpKernel and Routing. Also addresses some documentation notes brought up by andremolnar and Stof from #drupal-wscci.

RobLoach’s picture

Status: Needs review » Needs work

Drupal 8 was dead... Re-upping this one.

RobLoach’s picture

Status: Needs work » Needs review
RobLoach’s picture

FileSize
214.55 KB

Let's try a whole new patch...

andremolnar’s picture

On the whole I think there are far far more positives than negatives here. Another bold step towards modern tools for Drupal. So after initially hesitating (mostly from not fully grasping what composer would give us), this has got my support.

I'm not following why the components are a part of the latest patches - aren't they are already in 8.x.

Crell’s picture

The Kernel patch depends on the 2.1/master versions of those libraries, or at least of EventDispatcher. The 2.0.10 version won't work with it.

RobLoach’s picture

Status: Needs review » Needs work
RobLoach’s picture

Status: Needs work » Needs review
FileSize
647.86 KB

Updates to 2.1.x-dev.

RobLoach’s picture

FileSize
291.19 KB

I uploaded the wrong patch there... This one should fix it.

RobLoach’s picture

FileSize
291.76 KB

Updated now that #1467126: PSR-0 namespace auto registration for modules has been committed.

Status: Needs review » Needs work

The last submitted patch, 1424924-60.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
291.7 KB
RobLoach’s picture

#62: 1424924-62.patch queued for re-testing.

katbailey’s picture

I must say I really like this approach, even the extreme version of it, i.e. removing the symfony components out of Drupal's repo and managing them purely via composer.json.

Consider this hypothetical scenario: a site developer makes a well-informed decision to use a patched version of a symfony component (someone has discovered a change that can improve performance by a factor of a gazillion under certain circumstances or whatever... but the patch isn't going to make it in to the project any time soon). Rather than have a symfony patch masquerading as a Drupal core patch that she then needs to manage as such, it would make far more sense for the Drupal patch to just be a change in the composer.json file, i.e. to add the path to the fork repo and specify the branch to use for the component.

{
    "repositories": [
        {
            "type": "vcs",
            "url": "http://github.com/path/to/fork/of/symfony/event-dispatcher"
        }
    ],
    "require": {
        "symfony/event-dispatcher": "dev-performancefix",
        ....
    }
}

Granted, this is possibly very edge-casey (hard to say at this point) and I don't know to what extent we care about allowing for scenarios like this. But in general I just find the approach so much cleaner than including the symfony components in core's repo.

RobLoach’s picture

Composer seems to be a perfect fit for Drush: #1316322: Add PSR-0 autoloader to drush. Maybe that's the first step?

RobLoach’s picture

RobLoach’s picture

FileSize
1.05 MB

Status: Needs review » Needs work

The last submitted patch, 1542710.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.88 MB

Status: Needs review » Needs work

The last submitted patch, 1542710.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.88 MB

--binary.

Status: Needs review » Needs work

The last submitted patch, 1424924.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.88 MB

Removing traits.php for testbot.

Status: Needs review » Needs work

The last submitted patch, 1424924.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.88 MB

Updated without the other trait test.

webchick’s picture

Assigned: Unassigned » Dries

Tossing this to Dries to comment on, since this would add barriers to contributors.

RobLoach’s picture

Assigned: Dries » Unassigned
Status: Needs review » Postponed

I'd rather wait for #1542710: Update to latest Symfony 2.1 code to hit so that the patch is simplified with just the Composer stuff.

effulgentsia’s picture

For when this is unpostponed, some questions:

+++ b/core/composer.json
@@ -0,0 +1,10 @@
+{
+  "require": {
+    "symfony/class-loader": "2.1.*",
+    "symfony/dependency-injection": "2.1.*",
+    "symfony/event-dispatcher": "2.1.*",
+    "symfony/http-foundation": "2.1.*",
+    "symfony/http-kernel": "2.1.*",
+    "symfony/routing": "2.1.*"
+  }
+}

Does this mean that two people can checkout the same commit of Drupal, but depending on when they run composer, receive slightly different versions of Symfony? I wonder if that could lead to confusion and collaboration difficulties. It also means that Drupal HEAD could pass tests one day, and then the next day, with no commits to Drupal, HEAD can be broken (if a Symfony update causes a problem that leads to one of our functional tests breaking) and bot start failing unrelated patches in the queue. I wonder if it would be better to tell composer to get a specific commit (SHA-1) of Symfony instead, and then periodically, we submit Drupal patches to update that SHA-1?

I'd rather wait for #1542710: Update to latest Symfony 2.1 code to hit so that the patch is simplified with just the Composer stuff.

One of the problems identified in that issue is testbot is seeing Symfony's tests and choking on them. Does Composer solve that for us, and if so, how? In that issue, BTMash manually removed Symfony tests from the patch. Will Composer give us a way to do that, or do we need to fix that part of testbot regardless?

Seldaek’s picture

@effulgentsia: No this is not what it means :) Check out the docs on the lock file for more info: http://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file

As for the testing issue, Composer won't help you there, except for the fact that it puts all vendors in a vendor dir that you should be able to easily ignore from the test scanner in testbot. At this point we don't have the ability to ignore test files from packages (yet?).

effulgentsia’s picture

@Seldaek: thanks. That link says:

Commit your application's composer.lock (along with composer.json) into version control.

#75 does not include the composer.lock file. Am I understanding you right that the next patch here should include that file?

Seldaek’s picture

Well it's up to you to decide but basically if you commit it, you make sure everyone runs the same version (down to the exact commit if you use branch versions), but if you don't, you make sure your code is tested against more various versions, and therefore make sure it's using it more robustly. I see benefit in both.

patcon’s picture

@effulgentsia Wouldn't personally say composer adoption is official until we do :)

Some people feel otherwise about including lockfiles, often under the guise that they want everyone on bleeding edge easily for dev. But it's as simple composer update [--dev]? to get to the same bleeding edge that we'd be on without the lockfile using composer install.

EDIT: @Saldaek. Hm. Never thought of it that way. Might actually ge effective in aligning and educating on the expectations of semantic versioning and using libs properly... :)

Crell’s picture

So, there's 2 slightly different possible directions here:

1) Use composer to pull down new copies of Symfony code and check them into the repo. Commits for updates to Symfony are then still big diffs of everything that changed since our last snapshot. It really just becomes a slightly easier way to roll "chasing HEAD" patches.

2) Use composer to pull down new copies of Symfony code on the developer's machine. Vis, do not put Symfony code in the repository, but have everyone just run composer themselves when checking out from Git. (In this case, the tarball packager and testbot would have to be updated to run composer before doing their thing as well. People who download premade tarballs from d.o would not notice a difference either way.)

I personally much prefer option 2. Note that whatever we go with here will affect any other 3rd party libraries we pull in, too, such as Zend_Feed (for Atom parsing, if we put that into core).

patcon’s picture

My money's on #2 being the way forward. rubygems and npm have proven this approach as very effective at getting everyone thinking in terms of shared libraries rather than platform-specific modules -- vers-ctrl what you need to build your app, not all the code you need to run it.

But having said that, there's wisdom is small steps that don't change too much too fast, so I'm not opposed to #1 if that's the rational.

effulgentsia’s picture

I like #83.2 as well. Just checking: that's not incompatible with #80, right? We could still add a composer.lock file to git, so that every checkout (followed by composer install) of a particular commit of Drupal always results in the same set of vendor code. I think that's desirable.

To get #83.2 accepted, I think we'll at a minimum need the following:

  • An easy way for users on Linux, Mac, and Windows to get a complete, composed Drupal checkout. Especially for people on Windows not used to working at a command line, are there good GUI clients for running "composer install", or a way to have that called from a GUI git client?
  • Someone from our testbot infrastructure team to want to do the work of updating that to work with Composer properly.
  • Someone from our d.o. infrastructure team to want to do the work of updating our tarball packaging script.

We should probably figure out the first point before worrying about the second two, though reaching out to the people who can assist with the second two to find out what they think and how much work is involved wouldn't hurt.

katbailey’s picture

I am also very much in favour of #83.2 and this has no bearing on whether we include the composer.lock file, which I also think is a good idea.

Just want to draw people's attention to #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead where there has already been discussion around the various steps needed, with links to a couple of follow up issues should this go ahead, i.e. #1503234: Process Composer components when building Drupal core and #1503240: Process composer.json during core's make().

effulgentsia’s picture

Thanks. Seems silly to have this issue be about #83.2 if we also have #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead. Should we close this as a dupe, or make it about #83.1 only and let the other one be about #83.2?

Crell’s picture

To point 85.1, How many people are going to be actively developing on Drupal core and not be command-line capable? When I look around a DrupalCon I see an Apple convention with a strong Linux faction. When I've given camp presentations on Git (3 times now), I ask people what platform they're on. I think there were maybe 2 Windows people across 3 presentations. Not to sound too myopic, but *are* there people working on Drupal core who can't handle running two copy-and-paste command line operations? And how much effort do we want to put into those 3 people?

Let's recall that our documentation for getting code from Git in the first place is already extremely command line centric. We're talking about a command that's easier to run than the git instructions page we have now.

cosmicdreams’s picture

And in case you didn't know Git works perfectly fine for windows as does command line executable.

See Git for Windows and Powershell.

katbailey’s picture

@effulgentsia well, originally this issue was just about using Composer for updating Symfony and was not talking about removing the components from core's repo at all. I think there is a lot of consensus around getting Composer in, but maybe not so much for taking the next step and getting the Symfony components out. So, it probably makes sense to keep this issue for the first step (getting Composer in) and the other issue for the follow-up step.

But hey, if we think we could actually get Composer in more quickly using option 2 I would very happily roll a version of the patch that rips out the Symfony components ;-)

patcon’s picture

Would this be an appropriate breakdown of the related issues?

Anything else sensible to include?

patcon’s picture

Issue tags: +Composer

tagging

katbailey’s picture

Nice round-up, @patcon! I think that pretty much sums it up.

plach’s picture

Re #88:

Just to confirm: I'm using Windows but I perform most tasks from the command line, so the proposed change would be a non-issue for me.

sun’s picture

Introducing Composer for updating these libraries in core more easily is one thing. I can get behind that idea.

But removing these libraries entirely and completely relying on Composer sounds like a disaster.

Regardless of whether you're using a UI or the command line, this is not guaranteed to work:

> git checkout 8.x-my-issue-dev-branch

Every checkout will have to be followed by a php composer whatever command whenever you are switching branches, because you cannot be sure anymore whether the files you have are complete, correct, and compatible.

While you CLI coders might be OK with that and will write a git alias or shell script, I'm seriously not. I'm using a git GUI for development, and I want to get the proper files when switching branches.

Pretty please don't make contributing to Drupal even harder than it is already.

katbailey’s picture

If no git GUI had existed for Windows back when we were switching to git, would that have been a good enough reason for us to stick with CVS? (I wasn't involved in the discussion so I have absolutely no idea whether such considerations factored into it, but I feel strongly that the answer to this question should be a resounding "NO!").

Changes like this don't affect the end user in the slightest and they affect developers only insofar as they force them to embrace the tools and practices that the wider PHP community has adopted in order to more effectively manage projects. That, to my mind, can only be a good thing.

I'd imagine that the vast majority of pulls from upstream when working on D8 will not include a change to composer.json (or composer.lock), and so no extra step will be required. Does your GUI client show you which files have changed when it merges changes from upstream? Is it possible to add git hooks in the GUI? If not, then I'd say it has some catching up to do, but that shouldn't hold up a change to Drupal that otherwise makes a lot of sense.

don't make contributing to Drupal even harder than it is already

I honestly see this change as having the opposite effect!

sun’s picture

Title: Introduce Composer for Symfony component management » Use Composer for updating Symfony components
Status: Postponed » Needs work

@Dries just decided against #1475510-17: Remove external dependencies from the core repo and let Composer manage the dependencies instead. Highly appreciated.

Clarifying this issue title.

Recent patches were no longer rolled with renames=copies. See http://drupal.org/node/1542048 for configuring git.

This change proposal still means that we will replace Symfony's class loader with the custom and auto-generated by Composer.

It also means that all composer-managed code in /vendor lives in yet another deeper subdirectory level per component (sigh).

patcon’s picture

Title: Use Composer for updating Symfony components » Use Composer for updating Symfony components (without removing Symfony code from repo)

@sun @ katbailey sorry I think the issues were getting confused, as there are a few similar-sounding ones open.

Clarifying title a bit more for those unaware.

I'm going to copy yours and kat's comments into the other thread, as I think they're really valid, just moreso to that discussion.

Just to keep the issues distinct, can we please move conversation related to removing dependency code into the other thread? :)

RobLoach’s picture

Status: Needs work » Needs review
FileSize
353.02 KB

Updated patch...

RobLoach’s picture

Issue summary: View changes

Updated issue summary.

RobLoach’s picture

Issue summary: View changes

Added related issues.

RobLoach’s picture

Issue summary: View changes

f

RobLoach’s picture

FileSize
572.64 KB

Updated patch as there was demand for it in #1591686: Add Twig itself.

  • Latest from the Symfony 2.1.* front
  • SimpleTest dies with services1-1.php (it's PHP 5.4), so let's add it to core/vendor/.gitignore. This will make it so that it's never accidentally added to Drupal.
aspilicious’s picture

+++ b/core/composer.jsonundefined
@@ -0,0 +1,15 @@
\ No newline at end of file

Can we add a newline in the end?

+++ b/core/vendor/.gitignoreundefined
@@ -0,0 +1,2 @@
\ No newline at end of file

Same question

9 days to next Drupal core point release.

RobLoach’s picture

FileSize
573.51 KB

Certainly! Also noticed a couple other things:

  1. Composer uses the components' git for updates. This makes Drupal's git assume we want to use them as git submodules, which we don't want. Adding those to .gitignore fixes the issue. Note: If we get Symfony 2.1.0-alpha1 releases, we wouldn't need this .gitignore block.
  2. Composer has a deprecated .composer folder which it used in the past. It has since moved to "composer", which is more user-friendly. It keeps .composer for backwards compatibility, but we don't need it, so let's add it to .gitignore too.
RobLoach’s picture

FileSize
598 KB

Updated now that Yaml is in there.

cosmicdreams’s picture

This patch removes an aweful lot of symfony. But the issue title says that the intention is to not remove Symfony code from the repo.
Does this just need a reroll?

RobLoach’s picture

It removes symfony/class-loader, as we're just using the Composer one instead. Both support PSR-0, which is what we use.

catch’s picture

Please open a new issue for switching the class loader - if only for discussion. The Symfony classloader is already taking a noticeable amount of time loading classes when profiling 8.x, so there is no such thing as just switching it out for another PSR-0 classloader - we need to look seriously at features like the APC classloader (does composer have that or not?), how it performs without any caching etc.

dww’s picture

Status: Needs review » Needs work

Sounds like #103 needs a re-roll then at least for the class loader. Sorry I can't provide a more meaningful review, but I don't have time/bandwidth for a 600Kb patch right now...

RobLoach’s picture

Status: Needs work » Needs review
FileSize
536.24 KB

The attached patch keeps symfony/class-loader. The reason our use of Symfony's class loader is slow is because we register the "Drupal" namespace as a fallback, when it really doesn't need to be. We should instead register Drupal\Core and Drupal\Component individually like I had in #103.

Regarding Composer ClassLoader and APC, the plan is to allow the generation of a full static 1:1 classmap. This would be fast in APC as the class map array would be loaded directly into memory. Different discussion though, and something we should consider before stable releases.

The last submitted patch, 1424924-108.patch, failed testing.

RobLoach’s picture

FileSize
535.68 KB

Let's .gitignore traits.php.

Crell’s picture

Didn't we already decide that putting a .gitignore into Drupal itself was a bad idea? :-)

RobLoach’s picture

The .gitignore is in the core/vendors directory, and is meant to protect us from accidentally adding the vendor files that the SimpleTest bot can't handle. A big chunk of it won't be needed once the Symfony 2.1 beta comes out.

Crell’s picture

Ah, OK. core/vendors seems like it's probably safe to put a .gitignore. Objection withdrawn.

RobLoach’s picture

FileSize
522.53 KB

Talked with the Composer folk regarding composer.lock, and since we have this, it means we won't need installed.json as that's just cached data generated by Composer. Composer.lock is what keeps all developer versions the same, even if we're asking for a 2.1.* version of Symfony.

catch’s picture

Different discussion though

Yes so let's not randomly change the classloader in this patch then. I'm fine with discussing that in a separate issue though.

RobLoach’s picture

@naderman (author of Composer) and I will be doing a Core Conversation about Composer at Drupalcon, Munich.

catch’s picture

Component: wscci » base system
chx’s picture

Another 500k patch? What happened to clean , lean and extensible?

RobLoach’s picture

The patch gets larger with time because it is bringing in some of the later updates from Symfony. We currently require the latest in the master branch. Once the Symfony 2.1.* betas start coming out, we'll be able to target a specific version, resulting in a smaller patch.

sun’s picture

FileSize
512.62 KB

It's hard to believe, but this 500 KB patch indeed mostly consists of git renames/copies only. I've tried to reduce its size through advanced git diff options pertaining to renames/copies, but the max I was able to cut off is 10 KB ;)

Atttached patch is the identical patch.

That said:

- composer.lock does not seem to specify max/upper bounds for all components.

- I'm not happy with replacing the generic and standardized PSR-0 autoloader with Composer's, since that prevents and disallows to use alternative autoloader approaches (by simply swapping out the autoloader class name). Would it make sense to front-load Composer's static hashmap of bundled components in a separate autoloader that is executed before Symfony's/Drupal's, so we can retain our own and the generic PSR-0 alternatives for all the dynamic stuff we need to register?

- The .gitignore looks good.

- There seem to be a couple of additional .gitignore files in Symfony components. Odd.

catch’s picture

On the Composer autoloader, just in case it's not clear: http://drupal.org/node/1424924#comment-6046204

I have zero intention of changing the autoloader without it being properly discussed/reviewed/profiled first, so this patch will not get committed while that's included (at least with the current level of discussion about it on this issue).

RobLoach’s picture

Status: Needs review » Postponed

Symfony 2.1 beta should be coming out within the next few days. I'd like to wait till then to do a re-roll as it would simplify the patch a lot.

RobLoach’s picture

Two things:

  1. Symfony 2.1 beta very very soon, apparently....
  2. #1591686: Add Twig itself

Regarding performance, Seldaek has a pull request to Automatically generate classmaps for all PSR-0 packages to speed things up. This completely by-passes the PSR-0 namespace checking if the class is already in the classmap, which means there's a direct 1:1 reference between class and path. In APC, this array is statically cached, which means it would be much faster than anything the Symfony ClassLoader has to offer.

RobLoach’s picture

Status: Postponed » Needs review
FileSize
1.34 MB
1.31 MB

This patch:

  1. Updates to the latest in Symfony 2.1.*
  2. Updates to the latest in Twig 1.8.*
  3. Doesn't change the class loader

Fabien hasn't tagged BETA1 for the individual components yet, so we can't use "minimum-stability" to force downloading files rather than doing the git checkouts. That will come in time, for now, this'll work.

Status: Needs review » Needs work

The last submitted patch, 1424924-formatpatch.patch, failed testing.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.34 MB

Same patch.

RobLoach’s picture

Status: Needs review » Needs work
RobLoach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.35 MB

We can now use "minimum-stability". Make sure to apply with git apply --index.

aspilicious’s picture

Ok I spent half an hour on this, reviewing the changes and testing the functionality.
The actual changes in code are minor, most stuff is renaming and updating some symfony code.
Thnx to Robloach for helping me with the testing process.

When this patch is in, updating Symphony is rly easy I could handle it with a single command ;)
It does uglify the menu structure of the vendor folder a bit but
1) How many people are actually going to use that?
2) It's possible most of this will be removed out of drupal core in the end (in favour of packing stuff with the d.o. packaging script)
3) It's needed for composer to do his job

So I'm going to mark this rtbc

chx’s picture

Status: Needs work » Reviewed & tested by the community

OMG Composer yes, yes! We should totally get the packaging script to use Composer instead of committing Symfony components, Twig and soon Doctrine Common (or at least the annotation part of it) into our git repo.

katbailey’s picture

patcon’s picture

Status: Needs review » Reviewed & tested by the community

haha @chx I am so so so happy that you are excited about that prospect too. Like literally, just now, a coworker walked by my couch and was like "look at patcon all smiley" :)

And also @chx, of future interest perhaps:
https://github.com/composer/installers/blob/master/src/Composer/Installe...

webchick’s picture

Sorry folks; we need some serious help on getting below thresholds if we want to commit this anytime soon. :(

sun’s picture

Actually, this patch blocks at least one critical bug: #1447686: Allow importing and synchronizing configuration when files are updated which depends on an improvement we contributed to Symfony upstream.

Thus, committing this change means to unblock a critical bug fix and to contribute to get back under thresholds.

aspilicious’s picture

And we are back under thresholds (after committing some major rtbc bugs)

RobLoach’s picture

#128: 1424924-formatpatch.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Checked in with Dries briefly, and he felt that committing this was fine. I didn't approach the topic of #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead though, since that's a whole other kettle of fish.

The patch is humungous but really all it's doing is relocating some files around in a directory structure that composer likes. It also updates Twig, since we're behind on that now, and finally I confirmed that the class loader swap out that catch was concerned about is not in this patch, so we should be good to go here. Thank you so much for all of the help getting us back under thresholds, folks.

Committed and pushed to 8.x! Yay!

I don't think this needs a change notice?

Also when I applied it I got these whitespace-related messages:


1424924-formatpatch_5.patch:11308: trailing whitespace.
    
1424924-formatpatch_5.patch:13856: trailing whitespace.
     * 
1424924-formatpatch_5.patch:13859: trailing whitespace.
     * 
1424924-formatpatch_5.patch:17969: trailing whitespace.
    
1424924-formatpatch_5.patch:17971: trailing whitespace.
    
warning: squelched 41 whitespace errors
warning: 46 lines add whitespace errors.

But I'm completely fine just cleaning those up wholesale the day before release. :P

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

Anonymous’s picture

Issue summary: View changes

a