Problem/Motivation
While working toward #2982680: Add composer-ready project templates to Drupal core, we discovered that the Vendor Hardening Plugin sometimes fails to work, throwing an error:
Fatal error: Uncaught Error: Class 'Drupal\Component\FileSecurity\FileSecurity' not found in phar:///Users/ganderson/bin/composer/src/Composer/Plugin/PluginManager.php(196) : eval()'d code:245
The problem is that Composer plugins do not include the autoload.php
file; instead, they dynamically create their autoload information on the fly at runtime. This allows plugin hooks to work before the autoloader is dumped. The limitation here, though, is that the plugin manager does not allow plugins to hook the dynamic autoload generation, because this would be circular. The upshot is that a plugin can only autoload classes that are located in their default location inside vendor
, because the Composer Installers plugin is not given a chance to fix up any paths. Because of this, the Vendor Hardening plugin cannot find the Core File Security component, because the later is relocated to the /core
directory.
Steps to reproduce:
- Set up a Composer-managed Drupal 8.8.x site (e.g. similar to https://github.com/greg-1-anderson/drupal-drupal-composer)
- Add drupal/core-vendor-hardening version 8.8.x-dev
- Run
composer dumpautoload
- Observe the fire
Proposed resolution
In #3077455: Move Drupal Components out of 'core' directory, we considered fixing this by relocating the Core File Security component back to the vendor directory; however, that solution seemed too invasive to do at this time, so we are exploring potential workarounds instead.
There are two options:
- Preferred: Put a complete copy of the File Security class in a new namespace inside the Vendor Hardening Plugin.
- Alternate: Manually find the /core directory and require_once the File Security class.
Remaining tasks
None.
Follow-on Tasks
- #3077455: Move Drupal Components out of 'core' directory
- #3076600: Create drupal/core-recommended metapackage
- #2982680: Add composer-ready project templates to Drupal core
- #3079890: Re-add drupal/core-file-security in Vendor Hardening plugin composer.json
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
To work around a Composer class loader limitation, the Vendor Hardening plugin now has its own copy of the file security class from drupal/core.
Comment | File | Size | Author |
---|---|---|---|
#44 | Composer update with Drupal 8.8.1 error.txt | 4.24 KB | Peter Buchanan |
#35 | 3079481-5-to-35.interdiff.txt | 1.33 KB | greg.1.anderson |
#35 | 3079481-35.patch | 6.66 KB | greg.1.anderson |
#23 | 3079481-14-to-23-interdiff.txt | 862 bytes | greg.1.anderson |
#23 | 3079481-23.patch | 3.27 KB | greg.1.anderson |
Comments
Comment #2
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedBad patch; please ignore.
Comment #3
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedBad patch; please ignore.
Comment #4
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedBad patch; please ignore.
Comment #5
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedRe-rolled the miss-made patch from #3 (duplicated File Security class). No interdiff, as the intended contents are the same.
This patch demonstrates duplicating the File Security class inside the Vendor Hardening plugin.
Advantages:
- Completely self-contained; does not depend on drupal/core, and will keep on working if the File Security component is relocated or otherwise altered in core.
Disadvantages:
- Duplicate code: if a change to the File Security component is made in core, the same change must also be applied to the Vendor Hardening plugin.
- At some point we will want to relocate the File Security component to the vendor directory and alter the Vendor Hardening plugin to use the File Security class as a dependency.
Comment #6
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFixed typo in #4 (forgot to change project org back). No interdiff; please just ignore patches prior to #5 / #6.
This patch demonstrates installing a class loader to autoload the File Security class.
Advantages:
- Not necessary to duplicate the File Security class.
Disadvantages:
- Has a hidden dependency on drupal/core.
- Writing .htaccess and web.config stops working if the File Security class is relocated, etc. (requires matching changes in Vendor Hardening plugin)
Comment #7
MixologicNothing really to review in Patch #5 since its a C/P from core.
For Patch #6:
These
Should be "drupal/core does not contain the File Security component"
Also, maybe just 'file security', or FileSecurity or core-file-security.
Comment #8
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAdjusted error message as suggested - inserted missing word "not", and changed name of the file security component.
Comment #9
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedBack to 'needs review'
Comment #10
MixologicThis lgtm. Lets get a committers opinion.
Comment #11
larowlanPersonally, I favour the autoloading fix rather than the duplication one. But I will seek another committer opinion
should we return with an error code here, so CI systems will fail in this scenario
do we need a whole new class loader for one class? is there any harm in just using
require_once
here instead? If the class doesn't exist we bail, so we're always going to need it.If someone uses this as a stand alone component, they would require this - so shouldn't we leave it? Or put it in suggests or something?
Comment #12
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented1. We do not want to cause someone's
composer install
orcomposer dumpautoload
command to fail if the vendor directory cannot be protected. We are operating on a best-effort basis.2. Good idea. I'll get rid of the autoloader and just require the class, since we know we are going to use it.
3. If we
require
thedrupal/core-file-security
component, then Composer prints half a dozen lines of warnings, because it tries to require drupal/core (which replaces drupal/core-file-security), and then it cannot find some autoload classes, because/core
has been relocated, but Composer expects to find it invendor
.Comment #13
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedReplace class loader with a simple `require_once`.
Comment #14
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFixed a couple minor errors with #13; need to use 'is_file' instead of 'is_dir' now that we are testing the class file. Also, fixed quoting problem with error message.
Comment #15
MixologicAll changes lgtm
Comment #16
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpdated issue summary.
Comment #17
catch+1 to doing the require_once over duplication here.
Comment #18
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@catch #14 uses require_once and is RTBC. Do you think you could commit? Or do we need more review or other changes first?
Comment #19
Jaesin CreditAttribution: Jaesin at Chapter Three commentedI can confirm this is working. I was able to reproduce the error message when drupal core is not found and when I moved FileSecurity back to `lib/Drupal/Component/FileSecurity/FileSecurity.php` from vendor (in my setup), FileSecurity was loaded via `\Drupal\Composer\Plugin\VendorHardening\VendorHardeningPlugin::autoloadFileSecurity()` and `.htaccess` as well as `web.config` were successfully written to the vendor folder.
Comment #20
Mile23Needs a @todo and a follow-up for, uh... 8.8.x or 9.0.x? dunno. But a follow-up to change this when drupal/core doesn't
replace
drupal/core-file-security. At that point, the plugin can justrequire
drupal/core-file-security.RTBC +1 other than that.
Comment #21
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI can make a follow-up issue to put drupal/core-file-security dependency in the vendor hardening plugin (probably for Drupal 9).
How should we TODO this, though? We can't comment composer.json. Should we put it in a different file, or add a "_readme" or perhaps a "_todo" in the extra section?
Comment #22
Mile23We can add it to the docblock for autoloadFileSecurity().
Comment #23
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAdded follow-on task #3079890: Re-add drupal/core-file-security in Vendor Hardening plugin composer.json and added @todo to autoloadFileSecurity docblock.
Comment #24
Ghost of Drupal PastThis patch , while without a doubt fixes the problem (thanks!) makes me deeply uneasy.
Is it time to step back and look at the wider context? It's not going to be news to anyone that we have a gigantic legacy codebase with relatively little room for changes trying to cooperate with other codebases which are opinionated in their own ways. It's a feeling and I can't offer a specific solution but I think we need to consider moving one of the codebases (in case, Drupal or composer) closer to each other. Multisite is another bag of hurt where the two doesn't want to cooperate well as they are now. And it seems to me that special casing a single file is the kind of solution which just feels wrong and cries for a more generic solution. And I am sure people more familiar with composer have considered some but ... well, I am just asking for considering more :) Perhaps even as a followup -- this, as I said, surely works and solves the problem at hand.
Maybe the current composer plugins are not enough and we need to submit a PR upstream to make composer even more extensible, maybe we just need a "Drupal plugin" ... I do not know. I am just saying, special casing a single file makes me feel uneasy.
Comment #25
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@chx: I totally get where you are coming from. To answer your question, I would like to first direct your attention to #3077455: Move Drupal Components out of 'core' directory. If that patch were committed, this one would not be necessary. All of the Drupal Components (lib/Drupal/Component) would be in the `vendor` directory, where they belong, and the Vendor Hardening plugin would be able to use the File Security component without any Tom-hackery. This would be my preferred solution to this problem.
However, there was concern from multiple quarters that relocating all of the Components would be too invasive an operation to do in 8.8.x. Therefore, we decided to postpone the correct fix until Drupal 9, and work around the problem in one way or another for 8.8. This issue presented two options for that: either special casing the single file, or duplicating the single file in the Vendor Hardening plugin. Between those two choices, the former was preferred by most reviewers.
It is our intention to follow up with #3077455 and #3079890 in the Drupal 9 timeframe to provide a more generic solution. In the meantime, though, we have only a few short weeks before the 8.8.0 alpha code freeze. This issue will unblock #2982680: Add composer-ready project templates to Drupal core, which will greatly improve the Composer experience on Drupal 8.8.x. Sometimes we need to make small sacrifices to take small steps in order to allow us to be successful with our large goals. Hopefully this will in the end prove out to be the best thing we can do right now. If you're still uncomfortable, maybe you could lobby for #3077455 in Drupal 8.8.x; however, I don't think that's going to go.
Comment #26
Ghost of Drupal PastBy no means would I want to hold this patch and I saw the corresponding issue yes. I ... mostly just don't know, I mean, there's drupal-composer/drupal-project which seems to work and as far as I can see the main (only?) drawback of that is it installs a docroot below the project dir but really, is that big of a deal in 2019. All I have are doubts. Anyways, carry on, I will try to gather more understanding in slack.
Comment #27
Mile23+1 on #23, thanks.
FTR: I'm in Camp Duplicate Drupal/Core-File-Security In The Plugin, because it's the least weird of two not-so-great solutions. But either way there's technical debt, and we'll de-Drupalism it later, probably early in D9 dev time but maybe before.
We have consensus from the Composer Initiative people and some core maintainers, so let's do it.
Comment #28
larowlanThe core file security component is brand new, as in not in a tagged release. Is there an option to move just that component?
Comment #29
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@lawrolan: Yes, it would be possible to move just one component, and in fact there is a patch in #3077455 that does that. However, the problem with that solution is that there are then Components in two different locations, which did not seem desirable. It does technically work, though, and we can continue to investigate variations on that patch if you think it preferable.
Comment #30
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedBack to RTBC to either be committed as-is, or postponed / closed in favor of a different technique.
Comment #31
larowlanOk thanks, I'll discuss with some others and get back to you
Comment #32
alexpott@larowlan asked for my thoughts on this issue - whether we should move move the file security component (just that one) to somewhere outside core or go for the approach in #23. My thoughts are that doing the
require_once()
here is not that bad and once we move all components outside of /core and use composer to get them then this code becomes dead.Some thoughts on the patch...
This is not registering a class loader or doing autoloading. It's including the FileSecurity class. So first line of the doc block is incorrect. Given we're planning to remove this method how about making it private. Also the name is odd because as before we're not autoloading.
We should add a note to this file that until components are managed by composer we can't add new dependencies or classes to this component. Since if we do that this will break again. At the moment the
FileSecurity
class is self sufficient. This is a big advantage of copying the class as is ATM and then removing it once we can autoload properly.onPostAutoloadDump
is never fired at a time when all the code included by composer can be autoloaded. I think I've seen multiple autoloader dumps in certain situations with composer but I'm not sure. It'd be great if the issue summary had steps to reproduce the issue.Comment #33
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented1. This file used to install an autoloader, but was switched to a single require_once on request. I'll update the method name and comments.
2. If we did use an autoloader in this class, then the File Security component could at least be split into multiple classes in the future. Would you prefer that we go back to using an autoloader for that reason? Either way, I'll comment the File Security Component appropriately -- unless you'd prefer we copy the class here, as in #5.
3. Sometimes the autoload file is dumped after some other part of the Composer code path has set up autoloader. In that instance, the operation works. If you run
composer dumpautoload
, you can see the failure. You need a project that has been set up to use these components, e.g. https://github.com/greg-1-anderson/drupal-drupal-composer. That project isn't set up with a failure case at the moment. I'll update the issue summary.Comment #34
MixologicGiven the slack discussions, lets have the pendulum swing back to duplication.
The only thing missing then is
Comment #35
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's a new patch that goes back to the duplication of the file security component in the vendor hardening plugin.
I will update the issue summary, rewrite the change notice, and update the old follow-on issue #3079890: Re-add drupal/core-file-security in Vendor Hardening plugin composer.json
Comment #36
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpdate issue summary to reflect latest strategy and document steps to reproduce.
Comment #37
MixologicThe duplication and changes lgtm. Back to RTBC.
Comment #38
alexpottCommitted c74a193 and pushed to 8.8.x. Thanks!
We're not allowed comments here cause coding standards. As there is a comment already about duplication so I think this is okay.
Comment #41
alberto56 CreditAttribution: alberto56 commentedA security update just came out for Drupal 8.8.1 and I'm getting this error when trying to install Drupal using composer.
Comment #42
alberto56 CreditAttribution: alberto56 commentedFor me the fix was #3101956: Add drupal/core-vendor-hardening to https://github.com/drupal/recommended-project
Comment #43
tjtj CreditAttribution: tjtj commentedmy /core/composer/Plugin/VendorHardening directory is empty
Comment #44
Peter Buchanan CreditAttribution: Peter Buchanan commentedI'm not able to upgrade to Drupal 8.8.1 or 8.8.2. My terminal log shows:
Fatal error: Uncaught Error: Class 'Drupal\Composer\Plugin\VendorHardening\FileSecurity' not found in /Users/ThinkGov/Documents/Websites/Teija/Drupal_8_e_commerce/Teija2/vendor/drupal/core-vendor-hardening/VendorHardeningPlugin.php:355
Stack trace:
Is there a work around, I don't like not implementing Security updates to Drupal.
Comment #45
alberto56 CreditAttribution: alberto56 commented@Peter Buchanan
Adding this as a dependency to my composer file helped:
Good luck!
Comment #46
Peter Buchanan CreditAttribution: Peter Buchanan commentedThanks Alberto56, adding the dependency ("drupal/core-vendor-hardening": "^8.8") moves me on a bit.
Like https://www.drupal.org/project/drupal/issues/3101956 I am now getting the following error:
Fatal error: Uncaught Error: Class 'Drupal\Composer\Plugin\Scaffold\ScaffoldOptions' not found in /Users/ThinkGov/Documents/Websites/Teija/Drupal_8_e_commerce/Teija2/vendor/drupal/core-composer-scaffold/ManageOptions.php:55
Any thoughts anyone?
Comment #47
Mile23@Peter Buchanan: It might be more useful to ask on the #composer Slack channel. https://www.drupal.org/slack
Comment #48
Peter Buchanan CreditAttribution: Peter Buchanan commentedThanks Mile23, I've looked at the slack groups and can't see one for Composer. I'd have thought issues and work arounds would be more usefully documented in issues. maybe I'm being an old reactionary, I don't use slack:-)
Comment #49
Peter Buchanan CreditAttribution: Peter Buchanan commentedThanks to the suggestion 46 from Alberto56 I've completed an update to Drupal 8.8.2 on a site that's using Composer. In doing so I discovered that having the right version of PHP is important (I'm still on 7.0.33). It's also important that all modules other than Token, Redirect and Mail System are updated, particularly Pathauto, before the Drupal update.
I used MAMP pro to trial the update and will continue to use it for testing, the PHP log was particularly helpful for troubleshooting.
The update has been a real trial, starting with Composer crashing system limits on my host account. I'll think carefully before recommending Drupal.
Comment #50
s_leu CreditAttribution: s_leu as a volunteer and commentedThe hint in #45 worked for me on Drupal 9.0.2 as well.
Comment #51
quietone CreditAttribution: quietone at PreviousNext commentedpublish the CR