Problem/Motivation
Symfony and Twig have had some updates recently, which we should bring in.
Proposed resolution
- Update which versions of the projects we're targeting in core/composer.json.
- Symfony
- We want to target 2.3 when Drupal 8 is released, so update the composer.json definition to look for
"<2.4"
. This way, it allows downloading 2.2, and 2.3, but won't update to 2.4. - Twig
- 1.11 is out. We were targeting 1.8.*, but Twig has a much faster release cycle. So, let's target
"1.*"
instead so that 1.11 is brought in.
- Update the dependencies by running a composer update...
$ drush dl composer $ cd core $ drush composer update
- Verify the packages are updated using
drush composer show
doctrine/common [2.3.0] : Common Library for Doctrine projects
kriswallsmith/assetic [v1.1.0-alpha1] : Asset Management for PHP
symfony/class-loader [v2.1.3] : Symfony ClassLoader Component
symfony/dependency-injection [v2.1.3] : Symfony DependencyInjection Component
symfony/event-dispatcher [v2.1.3] : Symfony EventDispatcher Component
symfony/http-foundation [v2.1.3] : Symfony HttpFoundation Component
symfony/http-kernel [v2.1.3] : Symfony HttpKernel Component
symfony/process [v2.1.3] : Symfony Process Component
symfony/routing [v2.1.3] : Symfony Routing Component
symfony/serializer [v2.1.3] : Symfony Serializer Component
symfony/yaml [v2.1.3] : Symfony Yaml Component
twig/twig [v1.11.0] : Twig, the flexible, fast, and secure template language for PHP
Remaining tasks
Get a working, RTBC patch.
User interface changes
None.
API changes
There are some updates in the Symfony API, but @api
code we're using stays the same.
Blocking
Updating our out of date dependencies is partially or completely blocking a number of other issues, including:
#1561362: Change file_transfer() to use BinaryFileResponse (API cleanup)
#1854902: Document possible CSRF vulnerability in REST module (critical security issue)
#1855260: Page caching broken by accept header-based routing (Fixed bugs in HttpCache upstream)
#1831074: Send 415 Unsupported Media Type http header when serializer encounters an unsupported format (API cleanup)
#1289536: Switch Watchdog to a PSR-3 logging framework (Revised logger)
(Add more here as they're identified)
Comment | File | Size | Author |
---|---|---|---|
#31 | update-1834594-followup-31.patch | 127.64 KB | effulgentsia |
#28 | update-1834594-followup-28-src-do-not-test.patch | 3.64 KB | effulgentsia |
#28 | update-1834594-followup-28.patch | 81.5 KB | effulgentsia |
#24 | update-1834594-24.patch | 1.06 MB | effulgentsia |
#21 | update-1834594-21.patch | 1.34 MB | effulgentsia |
Comments
Comment #1
Crell CreditAttribution: Crell commentedThat's the wrong Symfony version. It's still tracking stables. We want to be tracking master/dev, as there's patches only in master that we added that we need.
Comment #2
RobLoachNeeded to switch to ClassLoader and ApcClassLoader as there were changes in master that broke using UniversalClassLoader. We wanted to make that change anyway.
Comment #4
Fabianx CreditAttribution: Fabianx commented#2: 1834594.patch queued for re-testing.
Comment #5
sunThe classloader change should not be contained in this monster.
Postponing on #1834594: Update dependencies (Symfony and Twig) follow up fixes for Composer
Comment #6
Crell CreditAttribution: Crell commentedsun: Knock it off. You just postponed an issue on itself.
We really, *really* should not be sweating about the class loader we're using right now. That's a premature micro-optimization. We can futz with the class loader we're using until June if we want to, since it's a performance question. For now, whatever works and works easiest works for me.
Comment #7
sunSorry, that was meant to be #1658720: Use ClassLoader instead of UniversalClassLoader
There's a lot of discussion over there, and it doesn't make sense 1) to repeat that here, nor 2) to review the changes within a 800 KB patch.
Comment #8
catchI'm not switching classloader unless it's in a dedicated issue. Issues that update external libraries should do one thing, and one thing only.
There's no discussion here about what the problem with the classloader even is.
Comment #9
catchAlso please define both 'premature' and 'micro-optimization' before bandying the term around in relation to the autoloader. How small a performance change is 'micro'? Exactly how late do we start dealing with the myriad performance regressions introduced during Drupal 8? Can optimization ever be premature in a 12-year-old legacy system where many of the performance bottlenecks are already well documented?
Comment #10
Crell CreditAttribution: Crell commentedIn this case, I define it as "changing the autoloader is by design independent of the things being autoloated once you're using PSR-0, so we should be focusing on an autoloader that's easy for us to work with right now, not the one that's fastest". At least until we are done adding libraries, so we should focus on "it works" for now, unless someone can demonstrate that we could not change our autoloader in May without breaking all the things. (The whole point of PSR-0 is to allow the code and the autoloader to be independent of each other. It wasn't designed for performance, but for DX.)
I am not saying performance isn't important. I'm saying our focus right now should be functionality and architectural-level performance issues, not API-irrelevant optimizations.
Comment #11
catchThat's not true either:
Which is why this discussion should happen in its own issue. Plugin system needs to be fixed to not rely on UniversalClassLoader before anything can be changed.
Comment #12
RobLoachComment #14
RobLoachComment #15.0
(not verified) CreditAttribution: commentedfds
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedAll that was needed to get past the autoloader problem was to revert core/vendor/composer/autoload_namespaces.php. This does that, but also adds some other Drupal and Symfony changes that were needed to get Drupal to even install locally for me. I'll post that diff separately, but first, curious what other problems bot finds.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedHere's #16 decomposed:
- 'base-min' is just the composer.json change
- 'base-full' is that plus running
composer update
- 'hacks' is the stuff I did on top of that to get #16. Obviously not commit worthy, just a work in progress. Next step is fix the test failures and maybe find proper ways to clean up the hacks.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedComment #21
effulgentsia CreditAttribution: effulgentsia commentedThis interdiff is all of the changes relative to #18's "base-full".
Comment #22
catchThis is blocking #1854902: Document possible CSRF vulnerability in REST module.
Comment #23
scor CreditAttribution: scor commentedchecked #21 but it's missing some changes necessary for #1831074: Send 415 Unsupported Media Type http header when serializer encounters an unsupported format (which will be used in 1 and 2). Are we tracking the right Symfony branch? The commit is in master (committed a few weeks ago).
Comment #23.0
scor CreditAttribution: scor commentedAdd list of dependent issues.
Comment #23.1
Crell CreditAttribution: Crell commentedAdd another blocked issue.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedHah! No, the http-kernel component was downgraded from 2.1 to 2.0 instead of upgraded to 2.2. That's why all those hacks in #21 were needed. The reason Composer chose to downgrade that component, along with event-dispatcher, is because Guzzle requires event-dispatcher 2.1.* (with no ">" sign). I'm not quite sure why that mean http-kernel needed to be downgraded to 2.0 instead of being left at 2.1, but in any case, doesn't matter, since what we need is to go to 2.2.
So, I:
- took the same composer.json as what's in #18's "base-min",
- temporarily removed guzzle/http from it,
- ran
composer update
, which removed core/vendor/guzzle,- told git to discard the removal of core/vendor/guzzle
- restored guzzle/http to composer.json
- discarded changes to core/vendor/composer/autoload_namespaces.php, per #16.
And that's what's in this patch. Hopefully, good enough to commit and unblock the issues depending on this. But future
composer update
runs will break until Guzzle updates their composer.json to allow event-dispatcher 2.2.Comment #25
Crell CreditAttribution: Crell commentedQuickfix filed upstream for Guzzle: https://github.com/guzzle/guzzle/pull/187
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedGiven that this is blocking several issues, I'm gonna be bold and RTBC it. catch or someone else could send it back if the composer process hacks in #24 are unacceptable.
Comment #27
webchickLooks like that change was incorporated quickly, so the concerns in #24 about breaking composer should be addressed.
Committed and pushed to 8.x. Thanks!
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedI tried a composer update, but it didn't quite work.
I had a follow up conversation with the Guzzle maintainer; he needed to make one additional commit, and I had to rework composer.json a bit to be compatible (frankly, it's black magic to me why this works and the original doesn't).
Also, to not require remembering to discard autoload_namespace.php changes every composer update, I fixed bootstrap.inc (and in the process, removed an incorrect comment about PSR-0).
This also includes a couple minor Symfony updates, since we're now tracking their master branch, so it automatically came in when I ran composer update.
Finally, there's still an annoyance that before running composer update, you need to
rm -rf core/vendor
, because we're now tracking git branches, but not committing the corresponding .git directories (thanks to our .gitignore) in order to avoid git submodule hell. But it trips up Composer updates to have git clones without the .git info; cleaning out the vendor directory first makes Composer happier. I have no idea where to document this so that the next person running a composer update knows this.Comment #29
effulgentsia CreditAttribution: effulgentsia commentedComment #30
tstoecklerI assume due to the
rm
business, this patch removes the .gitignore and README.txt files from core/vendor, which I think is unintended.Comment #31
effulgentsia CreditAttribution: effulgentsia commentedThanks. I ran #28's src again with:
File's larger, because it pulled in another day of upstream Symfony changes.
As a side note, given that #28 passed bot, we may now want to remove that core/vendor/.gitignore file, and since we're using Composer to manage the vendor directory, I don't know if core/vendor/README.txt is useful either, but that's a topic for a separate issue. This patch is just about getting
composer update
to be as smooth as possible.Comment #33
effulgentsia CreditAttribution: effulgentsia commented#31: update-1834594-followup-31.patch queued for re-testing.
Comment #34
effulgentsia CreditAttribution: effulgentsia commentedWould be nice for Composer to not be in a broken state in D8 HEAD, so RTBC'ing, under the assumption that the "src" file in #28 and commands in the comment text of #31 are straightforward enough for a core committer review to be sufficient.
Comment #35
RobLoach+1
Comment #36
webchickCommitted and pushed to 8.x. Thanks!
Comment #37.0
(not verified) CreditAttribution: commentedAdd more dependent issues.