Use a single vendor directory in the root

The reason, apparently, is the testbot. The syntax checker might have been configured to skip syntax checks for all files within core/vendor. I saw a few other testbot logs (like one here) for patches modifying external libraries and they skip syntax checks for everything within core/vendor. Unfortunately, I don't know to whom this should go. Maybe someone at IRC can help.

The syntax error is because this particular file needs PHP 5.5. The file in the Drupal core right now needs it too but since they normally skip syntax checks, this did not fail earlier.

Some of this work is related to #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead

I can't review the existing patch because it's 1.4mb.

Here's my go. It requires manual testing, using these steps:

• Pull the repo.
• Make sure you're in the root directory.
• Type composer install and let it finish.
• Apply the patch.
• Run PHPunit: ./vendor/bin/phpunit -c core/

You have to apply the patch after composer install because it re-installs core, obliterating any changes we just made. Also the patch might not apply because we're not sure when the core package was last updated.

But you can read the patch and get the idea.

PHPUnit will fail a few tests on the UnroutedUrlAssemblerTest test class, because they apparently don't know how to deal with a different core root directory.

Hopefully this patch should pass normal testing, because it also allows for the previous behavior of a non-customized vendor directory.

Room for improvement: Create core/autoload.php which does this for us in only one place, and change all references to vendor/autoload.php to load it.

Here's a better version which introduces core/autoload.php.

It illustrates the concept, but it won't apply after you do composer install, but the library is closing so I'll quit here.

Although, it doesn't allow someone to have the vendor directory above the web-root (which is typically where it would be anyways). My solution didn't allow for this either now that I'm thinking about it.

No, vendor shouldn't be above the web root. It should be alongside whichever composer.json is active. Thus far, it looks like Drupal only lets you have composer.json in one of two places.

Rather than doing a file_exists() on every request

Some systems need auto load without the dependencies of the container.

If Drupal weren't wrongheaded about composer, this would be a non-issue. But here we are :-)

We are defining the composer.json file, so it's reasonable to assume that vendor/ will be where we say it is.

If someone needs to put vendor/ somewhere else, they can modify core/autoload.php, and by doing so magically change it everywhere.

OK, here's the same idea, just in /autoload.php.

It should pass the testbot because it works either way. :-)

Manually test it under composer control:

• Make sure you're in the root directory.
• Type composer install and let it finish.
• Apply the patch (Use git apply --reject in case the patch doesn't apply cleanly.)
• Run PHPunit: ./vendor/bin/phpunit -c core/

Will need some other way to check out if the simpletest tests work under composer. Probably involving a .travis.yml file.

Solved the problem with SimpleTestBrowserTest, but the problem with SessionHttpsTest remains because https.php is devil code.

Hmm. Looks like that fixed itself.

This is a duplicate: #2406681: Add an autoload.php in the repo root to control the autoloader of front controllers

That issue has patches by a core maintainer, so let's mark this one a dupe.

OK, baby steps.

This changes the composer.json file to put its vendor directory in the root directory.

You can apply the small patch and then do this:

$ cd core/
$ composer install

This should put all the stuff into vendor/ and all the autoload.php stuff will work.

This patch just deletes the classmap section of the composer.json file. We can re-add this as needed.

Also: Big patch is big.

Added some desired behaviors to the meta issue here: #2002304: [META] Improve Drupal's use of Composer

This patch supports those goals by making it so no one is required to edit autoload.php to point to a different vendor/ directory.

Patch in #41 not applying on head for me

Fixed a problem while making this work: #2445497: Decouple ContainerBuilderTest from Symfony's tests.

Either this one or that one will need a reroll depending on which lands first.

So just making this patch was a pain, mostly because we keep vendor/ in the repo. Which is kind of wrong-headed, but there you go.

When applied, this patch will introduce whitespace errors, for the same reason.

In order to make the patch, I eventually had to do this:

$ git config diff.renameLimit 5000
$ git diff -M --binary 8.0.x > 2380389_49.patch

Anyway, if you can't make the monster patch work for you, there's a smaller patch of just core/composer.json and autoload.php. Apply that, apply the working patch from here: #2445497: Decouple ContainerBuilderTest from Symfony's tests. and then do this:

$ cd core
$ composer install

Now you can run unit tests from the root directory, like this:

$ ./vendor/bin/phpunit -c core/

#49 fails due to the testbot ignoring the wrong directory: #2460991: Ignore root level vendor/ during testbot lint

Thanks for cross linking, didn't know this was already going, I updated your regex-fu slightly on the pifr patch.

Conceptually wouldn't it make more sense to package vendor/ as the vendor directory of drupal/drupal, instead of specifying it as "../vendor" for drupal/core?

@bojanz: Eventually we'll undo #2406681: Add an autoload.php in the repo root to control the autoloader of front controllers and then it will all just make sense.

But for now, baby steps. :-)

....And now #2460991: Ignore root level vendor/ during testbot lint is in, so yay! We can proceed here.

We're still waiting on a review of this: #2445497: Decouple ContainerBuilderTest from Symfony's tests.

Patch no longer applies. Special reroll info in #49.

At yesterdays BOF organised by @bojanz we discussed all things composer - through discussion we realised that moving core/vendor outside of core we'd prevent people updating core using a tarball after using composer with a contrib module that needs it. @bojanz is going to explore using https://sculpin.io/documentation/embedded-composer/ to use two vendor directories - one for contrib and one for core. A summary of BOF will be produced sometime soon...

I have been thinking long of using embedded composer for this scenario. I think it fits perfectly. @bojanz, please let me know if I can help in any way.

@alexpott: If a user installed or updated a contrib module with composer already, why is it not feasible to enforce the core update through composer?

#60 how can we enforce that?

Restating the problem:

If we have root vendor/, it gets overwritten by the tarball because we're distributing vendor/ with the tarball. That means contrib has to find a way to re-add their requirements.

If we have core/vendor/ it gets overwritten, but that's ok because it's only the vendor directory for core.

If we've used composer create-project to create the site, this is not a problem, because core is then a dependency which we can manage with composer update, and in fact modules could declare their dependency on specific core versions.

This is a good behavior, but it's also not one that we can rely on since we need to distribute as tarballs.

I've created #2489646: Start using $root/vendor for non-core dependencies only since it's a completely different approach. Please comment and review.

Something else that occurred to me sometime after the BoF: Using Composer as a first-class citizen for installing modules is going to force our hand on #1612910: [policy, no patch] Switch to Semantic Versioning for Drupal contrib extensions (modules, themes, etc). If Packagist/Composer is going to be installing modules, then those modules must have a semver-compatible version string. It just won't work otherwise.

I still think we should do it; it's just one more thing we have to factor into our implementation/policy changes.

Here's a reroll.

Patch introduces whitespace errors because it's moving around non-Drupal code.

If you can't make the big patch apply, try the smaller one and then do:

$ cd core/
$ composer update

Note that the changes to \Drupal\Core\Composer\Composer illustrate the brittle nature of that kind of premature optimization.

This issue will conflict with #2389243: Remove unnecessary BC-layer in install_drupal() needing a reroll in one or the other.

I'm a little late to the game, so please point me to the appropriate sources if I am missing any important information.

I've been going through the code base, as well as this issue and several others for the past few days, and what I do not understand is why we treat drupal/core differently from regular packages:

1. drupal/core is its own package, so one could argue it should be usable as such. It also has a Composer package type of drupal-core, which makes it special, yet I have not been able to find documentation on how and why this type of package is special. drupal/drupal is also the package that provides the Composer installer to put drupal/core in the correct subdirectory within the application, yet this isn't clear when looking at drupal/core itself, for instance if one wants to write a custom front controller.
2. drupal/core is supposed to be installed within a bigger application. Keeping a separate vendor directory for drupal/core undoes Composer's core benefit. This issue is about removing that directory, but why do we keep vendor information in ./core/composer.json and \Drupal\Core\Composer\Composer?

@xano

#68.1:

So if you look at the root-level composer.json file, you'll see that it requires composer/installers.

composer/installers is a composer plugin that knows what a drupal-core package is, and that it should go into the core/ directory. The readme: https://github.com/composer/installers and specifically: https://github.com/composer/installers/blob/7a388b041ee5b6489173b1027e75...

So if you do this:

$ cd DRUPAL_ROOT
$ composer install

Here's what happens:

Composer sees the root-level composer.json. It looks at the composer.json files of its dependencies (composer/installers and drupal/core). It sees that composer/installers is a plugin and then uses it while installing other packages. composer/installers takes over and puts drupal/core into the core directory.

Basically it overwrites the current core/ directory with the one it just downloaded from d.o.

So now you'd have a versioned core that composer is managing. Unfortunately for us right now, the subtree split isn't completely up to date, so the core you'll then have won't be @dev. The good part is that when/if the subtree split is set up on d.o, you'll be able to update Drupal core by saying composer update --prefer-stable.

If you want to make a bespoke front controller, having a separate drupal/core package is good news for you. You can now make your own project, and have it depend on drupal/core and not get any of the root-level files. But if you don't also depend on composer/installer, then D8 core will be filed away under /vendor/drupal/core/. This breaks Drupal, so don't do it. (In fact, core should be the one to depend on composer/installers, but let's address that elsewhere.)

#68.2:

Composer also generates class maps for autoloading. The class maps arrays in /core/composer.json and that Composer class are there so common classes don't have to be looked up by the autoloader and are always present in the class map as literals. It's a performance optimization.

@Mile23: thank you for taking the time to explain this so clearly. I was familiar with most of the technical aspects, but it's good you've confirmed what I already thought. My main concern is how all this is documented. There is no README file in drupal/core, or documentation in that package's Composer file.

Composer also generates class maps for autoloading. The class maps arrays in /core/composer.json and that Composer class are there so common classes don't have to be looked up by the autoloader and are always present in the class map as literals. It's a performance optimization.

I was wondering if those shouldn't also be moved to the application level, but I now realize that would require any front controller package to be bigger than the bare minimum

Also, note that the template provided in https://github.com/drupal-composer/drupal-project already works pretty good.
Only drawback is that you get duplicated code between (root)/vendor and (root)/web/core/vendor, with the latter being just dead code - that is until we stop committing vendor code in our core repo (aka #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead).

A 36MB patch, but moved vendor from"/core" to "/". Also updates it, adds composer.lock to root, removes composer.lock and .gitignore from "/core".

Ideally I'd like to remove the vendor directory, but this seems like a first step.

Let's try this

If we're using a single vendor directory I think it should go in the /core directory, this patch updates "composer.json" to put vendor in core.

By putting it in core by default it allows those who manage core manually to just copy the latest "core" folder from a zip file to the FTP server. It also allows people to run `composer install` in the root directory and update all the dependencies.

It does however mean most custom dependencies will end up in core, and one using composer you need to continue. This is not such a bad thing, just something that needs to be dictated on a per project basis.

If you are a Composer user, you don't manage core manually, you simply execute composer update and composer will update everything in ./core for you (as well as resolve all of the dependencies).

It does however mean most custom dependencies will end up in core, and one using composer you need to continue. This is not such a bad thing, just something that needs to be dictated on a per project basis.

The problem with these statements is that we'll have projects telling people to use composer to install the project - for example Rules and Commerce and then instructions everywhere telling people to download core and copying it over. The once you've used Composer always use Composer is easy to say but it's going to be hard to enforce. This is what discussion in LA concluded and why #2489646: Start using $root/vendor for non-core dependencies only was opened.

@alexpott - Thanks for the comments the problem with #2489646: Start using $root/vendor for non-core dependencies only as mentioned in there is it means having to require two autoload.php's, once I updated the autoloader-suffix so it's different in each composer.json I thought I was on to something, but no luck. Also what if core and module X require the same package? will we end up with conflicts?

I am looking to have Drupal 8 is a way (like many other PHP projects) that it can be fully managed via composer, currently that's not an option. I'm happy to go with whatever the consensus as long as it works.

@davidwbarratt - I mainly didn't see it as being possible because I cloned the Drupal git repo, ran `composer update` and I now had two copies of everything, one in /vendor and one in /core/vendor.

I mainly didn't see it as being possible because I cloned the Drupal git repo, ran `composer update` and I now had two copies of everything, one in /vendor and one in /core/vendor

See https://github.com/drupal-composer/drupal-project/issues/23
One drawback of https://github.com/drupal-composer/drupal-project is indeed that you end up with duplicate code between vendor and core/vendor. It's only a cosmetic annoyance, because the project makes sure that the autoloader only ever sees the former (composer managed), and the latter (pulled with drupal/core since it's currently hard-committed in our repo) is only dead code, never actually loaded at runtime. It's just a (very actual) pita when doing searches in your IDE.

But other than that drupal-composer/drupal-project does seem to work fairly well.

Taking a step back I think we need the d.o packager and testbot working with composer before we can move forward.

#1923582: Add ability for testbot to run 'composer install' during installation and #2315545: Install composer dependencies to D8 when packaging run composer install in the core directory, this should therefore work before and after any changes we make.

One vendor to rule them all: #2554849: phpunit testing crashes if a root vendor directory is used

I tried to approach the problem again with the composer-merge-plugin from wikimedia (https://github.com/wikimedia/composer-merge-plugin)

composer-merge-plugin-2380389.patch contains just the changed composer.json the another one moved core/vendor back to vendor.

On the fence about this issue, but looking forward to discussing it on Tuesday afternoon (if not before!).

The composer-merge-plugin allows us to to a few things.

• /composer.json is a working example
• /composer.json is not polluted with core dependencies, it includes the dependencies from core/composer.json
• composer create-project drupal/drupal just works
• Get rid of core/composer.lock
• Follow-up: Get rid of autoload.php, because the autoloader is always in vendor/autoload.php
• Follow-up: Composer manager could manipulate /composer.json. Less specific login in the composer manager module.

Diggit. :-)

So composer-merge-plugin is nice and lets me type composer install at the top level and Drupal still works, which means people new to Drupal won't destroy their site by doing what comes naturally.

We can still do the subtree split for create-package, and if we don't, we can easily pull the two composer.json files together and remove the dependency on composer-merge-plugin.

+1.

Just one thing:

/composer.json is a working example

That's not true in this patch. The top-level composer.json file is a working working. :-)

Also, this mechanism of composer-merge-plugin could be extended to merge in other arbitrary composer.json files as well, but that's way outside scope here.

I am completely convinced by #88.

It gives us a single vendor directory, and no need to hack autoload.php anymore.
It also resolves the main problem alexpott and others had with the "single vendor" proposal, which is the inability to just replace the core directory if there's been a security update. Meanwhile, nothing's stopping people from modifying composer.json to download core as well if they want to.
I've always felt that blowing away core/ only makes sense if you're starting with Composer (ala Drush Make) in the first place, which is not always the correct assumption.

It allows people to simply do "composer require drupal/commerce" and get the module and its dependencies without any additional steps or dangers.
This finally makes the main part of composer_manager obsolete.

Testing contrib will require a way to point Composer to a local dir for a module, but that should be doable with the new local repository feature Composer has.

I'm a massive +1 to this issue. I think it finally delivers what we want with composer. There are still some tricky things to sort out with respect to managing your project with a mixture of methods - for example starting with composer and then downloading core - but that can be solved with documentation as this is truely an edge case - but this does address my main concerns. Can we file a followup to remove autoload.php since this patch makes it obsolete.

Also we need a CR and proof that existing sites can work happily with this change. It would also be great to get an issue summary that explains the concerns and how the approach addresses them.

webflo++

We also need to move the scripts from core/composer.json to composer.json, the merge plugin doesn't merge them and they make sense on the root composer.json anyway.

I've checked the rebased autoload paths (since a merge-plugin issue said it can sometimes be buggy), and they are all rebased correctly.

Also, you should be able to remove this line since there's no need for Composer Installers if you're not using the subtree split. This should also prevent the dependency from being installed.

Untrue. That would prevent you from being able to require modules ("composer require drupal/commerce")

That approach looks awesome.

Just wondering : https://github.com/wikimedia/composer-merge-plugin/issues/72 seems to hint that the new 'path' repository type added recently in Composer possibly makes composer-merge-plugin moot, so could we solve this with a native 'path' repo ?

@yched
No, we cannot. That allows a local directory to satisfy dependencies, but you still need to specify those dependencies in the first place.

As for:

I would not remove ./autoload.php because if we remove it, we are assuming the ./vendor directory is in the docroot, when that is not necessarily the case. I always move it out of the docroot for security reasons, if we remove ./autoload.php I wouldn't be able to do that. :(

Feels like a good enough reason to keep it, but we'll need to update the docblock.

@bojanz #101

[Composer's 'path' repo] allows a local directory to satisfy dependencies, but you still need to specify those dependencies in the first place.

Right,
- the root composer.json
declares /core as a local 'path' repository,
and requires 'drupal/core'
- /core/composer.json exposes the 'drupal/core' package and lists its 'require's
?

Then if you want to use the subtree split instead, just remove the /core 'path' repo from the root composer.json, and remove /core from your project's git ?

Sorry for the disruption, I'm most probably missing something here, but can't see what atm.

By the same argument, we should add Drupal Packagist to ./composer.json. Your example only allows you to install packages that are on the standard Packagist.

There's a parallel discussion going on in #2551607: [meta] Path forward for Composer support about using Drupal Packagist VS Packagist (by automatically creating d.o projects there). Initial consensus was to use Packagist, but if Drupal Packagist wins after all, then it will definitely need to be added. That's out of scope for this issue.

Also, what happens when Drupal core updates the scripts? Does the user need to manually merge the script list? Doesn't this bring us back to the same problem we had before with the dependencies?

Yes, any additional scripts would need to be manually added.
If we make composer-merge-plugin also merge scripts, then that would resolve your concern and fix the problem before we encounter it.
The current scripts explicitly make sense on the root composer.json and should go there, though.

@yched
It would mean we'd need to keep core/vendor, something we'd want to avoid for confusion sake.
It would also mean we'd need to reinvent merging all of the other keys (such as replace, conflicts, autoload) that composer-merge-plugin handles for us.

It would mean we'd need to keep core/vendor, something we'd want to avoid for confusion sake.

Really ? I'd expect that, once the drupal/core package is found (locally in that case), its dependencies would get resolved and downloaded to the root /vendor, just like any dependencies of any other packages required by the root composer.json ? So for the tarball we'd just ship core's dependencies in /vendor (i.e. mv core/vendor vendor) ?

It would also mean we'd need to reinvent merging all of the other keys (such as replace, conflicts, autoload) that composer-merge-plugin handles for us.

Right, indeed. But do those keys make more sense in core/composer.json rather than in the root one ?
I mean, if you use the subtree split by requiring packagist's drupal/core in your root composer.json, those keys from core/composer.json are lost as well, right ?

It seems to me that using a 'path' repo keeps 1:1 consistency between "/core comes from the tarball" or "/core is fetched through the subtree split package" : in both cases it's just the drupal/core package, you just modify where it is fetched from (packagist repo or local 'path' repo), otherwise everything else works the same ? That consistency seems important if we want to support both ?

Okay, I've just tried to implement "path" as an alternative to the merge plugin.

This is my composer.json:

{
  "name": "drupal/drupal",
  "type": "project",
  "require": {
    "composer/installers": "^1.0.21",
    "drupal/core": "~8.0"
  },
  "minimum-stability": "dev",
  "prefer-stable": true,
  "config": {
    "preferred-install": "dist",
    "autoloader-suffix": "Drupal8"
  },
  "repositories": [
    {
      "type": "path",
      "url": "core"
    }
  ]
}

So basically I kept it all as-is, but also added a pointer to the core directory as one of the repositories.
This didn't work because Composer empties the target directory before download, so core/ was emptied in preparation for core being downloaded, then Composer looked into core/ to find the package... Two conflicting features.
I also tried to remove drupal/core from "require" and add it to "replace" like webflo's patch did, but this resulted in the core dependencies not being downloaded at all.

So, as it stands we still need the composer-merge-plugin.
I think it's a good idea regardless of the problem above, because it also allows you to add a module to modules/ manually (custom module, for example) and then add it to the merge list to still get its composer dependencies. Useful for testing.

I'd wager it's composer-installer that's emptying the core/ directory.

Anyway, +1 on #109: Don't let the perfect be the enemy of the adequate in this case. We can amend in other issues.

Patch in #88 looking good.

Meanwhile I was testing the 'path' approach myself, and hit the same issue as @bojanz mentions, with Composer's PathDownloader deleting the target dir before trying to symlink to it since it's also the source dir.

That's IMO a bug in the (still recent) 'path' feature : if the local package is already in its final destination (here "/core", as specified by composer-installer for a package of type "drupal-core") then there's nothing to be done to "download" the package, and PathDownloader::download() shouldn't do any deletion or symlinking.

I fixed it with a relatively simple composer patch (will open a PR asap), and things seem to work fine after that.

I still think that the 'path' repo approach is worthwhile, because of :
- native composer support without depending on an additional corpus of code :-)
- the consistency argument at the end of #108 : "use the tarballed /core" and "use the packagist drupal/core subtree split" being just a switch of repository, with the rest of the composer resolution and processing being strictly equal, is IMO very precious if we intend to support both. I'm a bit worried that merging some keys from core/composer.json in one case and not in the other can be very hairpulling in the long run...

However, I agree we shouldn't wait on that being fixed upstream in Composer. If the merge plugin lets us "mv core/vendor vendor" at last, we should probably go for it, and possibly revisit 'path' later ?

I think [composer-merge-plugin] is a good idea regardless of the problem above, because it also allows you to add a module to modules/ manually (custom module, for example) and then add it to the merge list to still get its composer dependencies. Useful for testing.

Yeah, that topic (handling the composer.json of local custom modules) is of high interest to me too, see https://github.com/drupal-composer/drupal-project/issues/25 :-)

But that's a different topic IMO. composer-merge-plugin might very well be the right tool for that case, but I'm not sure it's the right tool for the /core thing, for the reasons in #108 / #111.

PR for the Composer 'path' repo deleting the local folder : https://github.com/composer/composer/pull/4431

Right, indeed. But do those keys make more sense in core/composer.json rather than in the root one ?
I mean, if you use the subtree split by requiring packagist's drupal/core in your root composer.json, those keys from core/composer.json are lost as well, right ?

I'm a bit worried that merging some keys from core/composer.json in one case and not in the other can be very hairpulling in the long run...

Sure, "path" is conceptually cleaner due to there being one magical class less in the process.
On the other hand, supporting local modules (custom development or testing) and a composer_manager-like workflow in general will end up requiring
the merge plugin anyway, bringing us back to the start.

That said, I'm not sure I see the same problem with the merge plugin.
Keep in mind that Composer already merges these keys at runtime.
If drupal/drupal requires drupal/core, then Composer will download drupal/core, parse its composer.json, and merge its requirements, replacements, autoload information, etc. The only thing the merge plugin adds to the process is doing that merge up front for the specified composer.json files. The mechanism is the same.

+1 on the concept, shown in 2380389-117-composer.json_.patch.

If only we didn't have to mess around with hairy, delicate patches to vendor/. #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead

Reverted the usage of DRUPAL_ROOT.

Found two other instances of core/vendor.

Not sure why it's not failing tests, it should be passing (passes on DrupalCI)

Drafted a change record on #2574533.

I think issue summary looks good too.

We still need to update this comment in autoload.php:

 * This file can be edited to change the autoloader if you are managing a
 * project's dependencies using Composer. If Drupal code requires the
 * autoloader, it should always be loaded using this file so that projects
 * using Composer continue to work.

I suggest we just remove it completely.

We also need to move the "scripts" from the core composer.json to the root composer.json

Here's the for-review patch. Generated with the following, after the patch is applied with --index in a clean repo:
git diff --cached `git status --short | grep -v 'vendor/' | grep -v 'composer.lock' | sed -e 's/M //' -e 's/A //' -e 's/?? //' -e 's/D //'`

X-Post with the above, but the script should work for future patches as well.

+++ b/.gitattributes
@@ -51,3 +51,4 @@
+*.ttf     -text diff

I am probably missing something. Why do we have this?

Changing as per #152. Review patch generated with help of #153. I have not changed anything for #154 yet as I am not clear if that was intentional.

I tested and confirm that applying the patch does not affect/break existing sites. I created a new D8 site, applied the patch, and then checked that the site was still functional. No cc is necessary.

We still haven't moved the scripts :)

Quick before it needs a reroll! :-)

What's the difference between #165 and #166?

I forgot to add the Plugin in #166

So! Reviewed this with @webflo tonight at DrupalCon.

Looks like all of the main Composer-y people I know of are on board with this, so that sounds great. The idea of being able to run composer install from root again is lovely. And while @alexpott was initially opposed earlier in this issue, looks like he came around in #2380389-96: Use a single vendor directory in the root. He also requested two things; a change record (which is done) and testing on an existing site to make sure it doesn't break.

On my local I have Acquia Dev Desktop running PHP 5.5.27 w/ APC cache enabled. I went through the following steps:

1. drush si against latest HEAD
2. Log in as admin, load the home page in my browser.
3. Apply patch.
4. Reload the page.
5. BAM. Fatal error.
   

   Warning: require(/Users/webchick/Sites/8.x/core/vendor/symfony/http-foundation/Response.php): failed to open stream: No such file or directory in /Users/webchick/Sites/8.x/vendor/symfony/class-loader/ApcClassLoader.php on line 114
   
   Fatal error: require(): Failed opening required '/Users/webchick/Sites/8.x/core/vendor/symfony/http-foundation/Response.php' (include_path='.:/Applications/DevDesktop/common/pear:/usr/lib/php') in /Users/webchick/Sites/8.x/vendor/symfony/class-loader/ApcClassLoader.php on line 114
   

Basically the APCu cache is holding onto the old file paths and isn't notified that the file paths have changed. What we found is one of two things can get you out of this situation:

1. Restart Apache (which clears the APCu cache)
2. (Once rebuild.php is fixed, see below) Edit your settings.php and add the line $settings['rebuild_access'] = TRUE, then hit /core/rebuild.php, which rebuilds your APCu cache. (No idea how someone is supposed to know how to do that from a PHP fatal error, though. :\)

Things that do not work include: going to update.php (same fatal error), deleting the sites/default/files/php directory (no effect, because old files are cached). And, if you revert the patch you have to restart Apache again because now it'll cache the new files. ;)

So we need a few things here:

1) rebuild.php needs to be fixed because it's still pointing to /vendor/autoload.php after this patch (should be /../autoload.php)
2) Also need automated tests of rebuild.php of any kind (separate issue, major?)
3) Make really sure that both the change record and the beta16 release notes call out that this could happen to D8 sites w/ APC and what to do about it.

Needs work for #1.

Oh, another thing that doesn't seem to work is drush cr. I guess it doesn't clear APCu cache as it goes.

In additional hilarity, drush cr could never work, because it's clearing the CLI cache and not the Apache cache. ;) /via @webflo

We tested this with ./php -r "apcu_clear_cache();" directly.

1) rebuild.php needs to be fixed because it's still pointing to /vendor/autoload.php after this patch (should be /../autoload.php)
2) Also need automated tests of rebuild.php of any kind (separate issue, major?)

I think we can do both in one issue. Created #2575267: Add test coverage for rebuild.php. I am gonna work on it during the extended sprint.

So what *should* happen is that you solve this by typing composer install, and it calls the same code that's used by rebuild.php to uncachify some stuff, particularly APC. And it should also be called by update.php as a matter of course.

So the way forward is:

1. Refactor the functionality of rebuild.php into some modular code.
2. ...Which must not be cached. :-) Or at least immune to being moved around.
3. Which is tested.
4. Call it from rebuild.php.
5. Call it as part of the build process in composer.json to reset the caches.
6. Call it from update.php as well.

How much of that is in scope here? Not enough. There should be another issue and this one should be postponed on it.

I just did a round of manual testing as per #169.

1. Install latest HEAD using browser.
2. Saw that site is working.
3. Apply the patch in #166.
4. Apply the fix for rebuild.php in #2575267-3: Add test coverage for rebuild.php.
5. Saw the same exception as webchick reported in #169.
6. Update settings.php to add $settings['rebuild_access'] = TRUE;.
7. Accessed core/rebuild.php in browser.
8. Saw that site is working fine.
9. Reverted the patches.
10. Saw the same exception.
11. Ran core/rebuild.php in browser (rebuild_access is still TRUE).
12. Saw that site is working fine.

We should note that reverting the patches and rebuild works because rebuild loads the autoloader within core/vendor. The fix is still required.

Since the fix for rebuild.php is in #2575267: Add test coverage for rebuild.php, should we postpone this on that? Or should we bring in the fix here and deal with the tests in that issue? I have no strong preference because there are pros and cons on each side. If we bring the fix here, we can commit this quicker but doing it there helps us preserve git history.

I created #2575495: Invalidate APCu Class Loader automatically if necessary to deal with composers autoload and apcu.

See also #2575747: Proposed workflow for Composer + module download on cheap hosting / FTP

In #2575495: Invalidate APCu Class Loader automatically if necessary #39 there's a way to deal with the issue of ApcClassLoader getting confused when you move the vendor directory (as you might with this patch).

I've folded that in here, because it solves the immediate problem here. There are other concerns it doesn't solve, but those are out of scope in this issue.

See the interdiff for the details.

Using this technique, we overcome the manual test errors in #169, repro'd in #174, so that if you have a Drupal installed with APC, you can just pull the core repro and everything works.

Ewps. Forgot this.

Also, any clues on how to make a patch that isn't 35 MB would be appreciated. :-)

You have to use git diff -C -M.

I think my previous patch broke the testbot, so let's try again with -C -M.

Rerolled the patch. The patch is bigger than because i run "composer update --lock" from repo root.

We're good to go.

Patch in #181 was corrupt.

/me wonders why #181 passed... :/

Re #184:
@webflo killed the test run before it finished.

https://www.drupal.org/pift-ci-job/43543 shows that no test results came back, so the "All classes passed!" (which leads to the green color) is theoretically correct, but it really means "No classes passed!" as well. I have no idea where the "14,121 pass" in the summary here came from.

I also saw the actual fatal in the Jenkins output a minute ago, but I can't find it right now. Very strange! In any case that's a DrupalCI issue.

@Mixologic: #181 >> https://www.drupal.org/pift-ci-job/43543 >> https://dispatcher.drupalci.org/job/default/18834/consoleFull Search for "Drupal\system\Tests\System\HtaccessTest"

K, repeating steps from #169 now that #2575495: Invalidate APCu Class Loader automatically if necessary is in.

1. drush si against latest HEAD
2. Log in as admin, load the home page in my browser.
3. Apply patch.
4. Reload the page.
5. BAM. Fatal error.
   

   Warning: require(/Users/webchick/Sites/8.x/core/vendor/symfony/http-foundation/Response.php): failed to open stream: No such file or directory in /Users/webchick/Sites/8.x/vendor/symfony/class-loader/ApcClassLoader.php on line 114
   
   Fatal error: require(): Failed opening required '/Users/webchick/Sites/8.x/core/vendor/symfony/http-foundation/Response.php' (include_path='.:/Applications/DevDesktop/common/pear:/usr/lib/php') in /Users/webchick/Sites/8.x/vendor/symfony/class-loader/ApcClassLoader.php on line 114
   

6. Went to rebuild.php. Fatal error:
   

   Warning: require_once(/Users/webchick/Sites/8.x/core/vendor/autoload.php): failed to open stream: No such file or directory in /Users/webchick/Sites/8.x/core/rebuild.php on line 23
   
   Fatal error: require_once(): Failed opening required '/Users/webchick/Sites/8.x/core/vendor/autoload.php' (include_path='.:/Applications/DevDesktop/common/pear:/usr/lib/php') in /Users/webchick/Sites/8.x/core/rebuild.php on line 23
   

7. Ran drush cr. Reloaded home page. Same fatal error:
   

   
   Warning: require(/Users/webchick/Sites/8.x/core/vendor/symfony/http-foundation/Response.php): failed to open stream: No such file or directory in /Users/webchick/Sites/8.x/vendor/symfony/class-loader/ApcClassLoader.php on line 114
   
   Fatal error: require(): Failed opening required '/Users/webchick/Sites/8.x/core/vendor/symfony/http-foundation/Response.php' (include_path='.:/Applications/DevDesktop/common/pear:/usr/lib/php') in /Users/webchick/Sites/8.x/vendor/symfony/class-loader/ApcClassLoader.php on line 114
   

Ummmm.... ?