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.
Comment | File | Size | Author |
---|---|---|---|
#128 | 1424924-formatpatch.patch | 1.35 MB | RobLoach |
#126 | 1424924-formatpatch_4.patch | 1.34 MB | RobLoach |
#124 | 1424924-diff.patch | 1.31 MB | RobLoach |
#124 | 1424924-formatpatch.patch | 1.34 MB | RobLoach |
#120 | composer.update.120.patch | 512.62 KB | sun |
Comments
Comment #1
Crell CreditAttribution: Crell commentedI'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.)
Comment #2
RobLoachThanks for the renames = copies note :-).
Comment #3
Crell CreditAttribution: Crell commentedI 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.
Comment #4
RobLoachThis 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:
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.
Comment #5
RobLoachComment #6
RobLoachIn future patches, to upgrade components, all you have to do is change the version numbers in composer.json accordingly, and then run:
Comment #7
sunI 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.
Comment #8
RobLoachThis patch...
Comment #9
RobLoachWhat fallback directories would we be requiring? I can see us registering module /lib directories later on in the bootstrap.
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.
Comment #10
sunCan't you make it so that it puts the files where they should be according to our PSR-0 directory structure?
Comment #11
Crell CreditAttribution: Crell commentedPSR-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
Comment #12
Crell CreditAttribution: Crell commentedCross 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.)
Comment #13
sunStill, 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.
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:
So far, so good. But this train of thought has to be continued:
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.)
Comment #14
Crell CreditAttribution: Crell commentedSymfony 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.
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.)
Comment #15
Seldaek CreditAttribution: Seldaek commented@sun, a few comments (from a perhaps biased Composer lead dev:)
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.
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.
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.
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.
Comment #16
RobLoachWe should actually have composer.json in /core, so that users arn't inclined to edit it.
Comment #17
RobLoachIt'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.
Comment #18
Crell CreditAttribution: Crell commentedI 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?
Comment #19
RobLoachWe 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:
Is there anything that we're missing here from Symfony's ClassLoader here? The above would have to be in a follow up patch.
Comment #20
RobLoachLet's wait for #1321540: Convert DBTNG to namespaces; separate Drupal bits to hit to get this in.
Comment #21
sunSplit the second part of #13 into #1451056: [policy] How to handle unforeseen diversion of Symfony code in stable/API-locked Drupal core?
Comment #22
Crell CreditAttribution: Crell commentedThe DBTNG conversion is in. Back to active.
Comment #23
RobLoachEDIT: Updated the issue summary.
Comment #23.0
RobLoachUpdated summary
Comment #23.1
RobLoachUpdated issue summary.
Comment #23.2
RobLoachUpdated issue summary.
Comment #24
Lars Toomre CreditAttribution: Lars Toomre commentedA 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.
Comment #25
RobLoachautoload.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 ;-) .
Comment #26
Lars Toomre CreditAttribution: Lars Toomre commentedThanks for the explanation Rob.
Comment #27
webchickCan 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. :)
Comment #28
sun@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:
Comment #29
RobLoachWe 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.
Comment #30
RobLoachI'll open up a Composer issue to allow use of an unstripped verison of the ClassLoader.
EDIT: https://github.com/composer/composer/issues/386
Comment #31
Crell CreditAttribution: Crell commentedAgreed 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.
Comment #32
RobLoachStof 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:
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?
Comment #33
RobLoachComposer now doesn't compress ClassLoader.php since it's user-facing, so we should update the patch to get the unstripped ClassLoader.
Comment #34
RobLoachPassing 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.
Back to "needs review"!
Comment #36
RobLoachWe should actually have the .lock file in there to prevent that from happening. Let's try this again.
Comment #38
RobLoachFound it.
Comment #38.0
RobLoachUpdated issue summary.
Comment #38.1
RobLoachUpdated issue summary.
Comment #39
Seldaek CreditAttribution: Seldaek commentedRob, 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.
Comment #40
RobLoachThanks for the note, Seldaek. I've updated the patch with that small doc fix.
Comment #42
RobLoachComment #43
Crell CreditAttribution: Crell commentedI'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. :-)
Comment #44
dwwSorry, 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.
Comment #45
Seldaek CreditAttribution: Seldaek commented@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
Comment #46
RobLoachUntil the packager and test bot handle Composer installations, HttpFoundation will have to stay in there.
Should we be updating CHANGELOG.txt here?
Comment #47
Crell CreditAttribution: Crell commentedSo 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.
Comment #48
sunThe 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?)
Comment #49
RobLoachHe'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.
Comment #50
RobLoachWe must update with EventDispatcher, HttpKernel, and Routing!
Comment #51
RobLoachThis patch includes 2.0.10 of EventDispatcher, HttpKernel and Routing. Also addresses some documentation notes brought up by andremolnar and Stof from #drupal-wscci.
Comment #52
RobLoachDrupal 8 was dead... Re-upping this one.
Comment #53
RobLoachComment #54
RobLoachLet's try a whole new patch...
Comment #55
andremolnar CreditAttribution: andremolnar commentedOn 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.
Comment #56
Crell CreditAttribution: Crell commentedThe 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.
Comment #57
RobLoachComment #58
RobLoachUpdates to 2.1.x-dev.
Comment #59
RobLoachI uploaded the wrong patch there... This one should fix it.
Comment #60
RobLoachUpdated now that #1467126: PSR-0 namespace auto registration for modules has been committed.
Comment #62
RobLoachComment #63
RobLoach#62: 1424924-62.patch queued for re-testing.
Comment #64
katbailey CreditAttribution: katbailey commentedI 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.
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.
Comment #65
RobLoachComposer seems to be a perfect fit for Drush: #1316322: Add PSR-0 autoloader to drush. Maybe that's the first step?
Comment #66
RobLoachphpBB 3.1 now uses Composer to bring in Symfony's EventDispatcher.
Comment #67
RobLoachComment #68.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #69
RobLoachComment #71
RobLoach--binary.
Comment #73
RobLoachRemoving traits.php for testbot.
Comment #75
RobLoachUpdated without the other trait test.
Comment #76
webchickTossing this to Dries to comment on, since this would add barriers to contributors.
Comment #77
RobLoachI'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.
Comment #78
effulgentsia CreditAttribution: effulgentsia commentedFor when this is unpostponed, some questions:
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?
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?
Comment #79
Seldaek CreditAttribution: Seldaek commented@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?).
Comment #80
effulgentsia CreditAttribution: effulgentsia commented@Seldaek: thanks. That link says:
#75 does not include the composer.lock file. Am I understanding you right that the next patch here should include that file?
Comment #81
Seldaek CreditAttribution: Seldaek commentedWell 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.
Comment #82
patcon CreditAttribution: patcon commented@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 usingcomposer 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... :)
Comment #83
Crell CreditAttribution: Crell commentedSo, 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).
Comment #84
patcon CreditAttribution: patcon commentedMy 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.
Comment #85
effulgentsia CreditAttribution: effulgentsia commentedI 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:
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.
Comment #86
katbailey CreditAttribution: katbailey commentedI 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().
Comment #87
effulgentsia CreditAttribution: effulgentsia commentedThanks. 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?
Comment #88
Crell CreditAttribution: Crell commentedTo 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.
Comment #89
cosmicdreams CreditAttribution: cosmicdreams commentedAnd in case you didn't know Git works perfectly fine for windows as does command line executable.
See Git for Windows and Powershell.
Comment #90
katbailey CreditAttribution: katbailey commented@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 ;-)
Comment #91
patcon CreditAttribution: patcon commentedWould this be an appropriate breakdown of the related issues?
You Are Here ^^
Core: About adding composer.json (and composer.lock) to D8 codebase (not deleting deps yet)
Can be done right now with no change in process for those who don't want to deal with it, as
composer update
will just kablooie the vendor/ dir for those who want to. Everyone else can pretend thatrobots.txt
invited a friend as far as immediate effects on their lives ;)This might help us work out how we'd like to use properties like "require" vs "require-dev", and other things that
Core: About removing the dep code from our repo. Will hinge on many subtasks...
Infra: About having Composer used by tarball packaging scripts on d.o
Drush: About having Composer used by make command
Infra:
Docs:
Anything else sensible to include?
Comment #92
patcon CreditAttribution: patcon commentedtagging
Comment #93
katbailey CreditAttribution: katbailey commentedNice round-up, @patcon! I think that pretty much sums it up.
Comment #94
plachRe #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.
Comment #95
sunIntroducing 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:
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.
Comment #96
katbailey CreditAttribution: katbailey commentedIf 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.
I honestly see this change as having the opposite effect!
Comment #97
sun@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).
Comment #98
patcon CreditAttribution: patcon commented@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? :)
Comment #99
RobLoachUpdated patch...
Comment #99.0
RobLoachUpdated issue summary.
Comment #99.1
RobLoachAdded related issues.
Comment #99.2
RobLoachf
Comment #100
RobLoachUpdated patch as there was demand for it in #1591686: Add Twig itself.
Comment #101
aspilicious CreditAttribution: aspilicious commentedCan we add a newline in the end?
Same question
9 days to next Drupal core point release.
Comment #102
RobLoachCertainly! Also noticed a couple other things:
Comment #103
RobLoachUpdated now that Yaml is in there.
Comment #104
cosmicdreams CreditAttribution: cosmicdreams commentedThis 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?
Comment #105
RobLoachIt removes symfony/class-loader, as we're just using the Composer one instead. Both support PSR-0, which is what we use.
Comment #106
catchPlease 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.
Comment #107
dwwSounds 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...
Comment #108
RobLoachThe 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.
Comment #110
RobLoachLet's .gitignore traits.php.
Comment #111
Crell CreditAttribution: Crell commentedDidn't we already decide that putting a .gitignore into Drupal itself was a bad idea? :-)
Comment #112
RobLoachThe .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.Comment #113
Crell CreditAttribution: Crell commentedAh, OK. core/vendors seems like it's probably safe to put a .gitignore. Objection withdrawn.
Comment #114
RobLoachTalked 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.
Comment #115
catchYes so let's not randomly change the classloader in this patch then. I'm fine with discussing that in a separate issue though.
Comment #116
RobLoach@naderman (author of Composer) and I will be doing a Core Conversation about Composer at Drupalcon, Munich.
Comment #117
catchComment #118
chx CreditAttribution: chx commentedAnother 500k patch? What happened to clean , lean and extensible?
Comment #119
RobLoachThe 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.
Comment #120
sunIt'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.
Comment #121
catchOn 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).
Comment #122
RobLoachSymfony 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.
Comment #123
RobLoachTwo things:
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.
Comment #124
RobLoachThis patch:
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.
Comment #126
RobLoachSame patch.
Comment #127
RobLoachThe components are now tagged.
https://twitter.com/RobLoach/status/217279614282776576
Comment #128
RobLoachWe can now use "minimum-stability". Make sure to apply with
git apply --index
.Comment #129
aspilicious CreditAttribution: aspilicious commentedOk 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
Comment #130
chx CreditAttribution: chx commentedOMG 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.
Comment #131
katbailey CreditAttribution: katbailey commented@chx see #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead
Comment #132
patcon CreditAttribution: patcon commentedhaha @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...
Comment #133
webchickSorry folks; we need some serious help on getting below thresholds if we want to commit this anytime soon. :(
Comment #134
sunActually, 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.
Comment #135
aspilicious CreditAttribution: aspilicious commentedAnd we are back under thresholds (after committing some major rtbc bugs)
Comment #136
RobLoach#128: 1424924-formatpatch.patch queued for re-testing.
Comment #137
webchickChecked 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:
But I'm completely fine just cleaning those up wholesale the day before release. :P
Comment #138.0
(not verified) CreditAttribution: commenteda