Problem/Motivation
This issue creates infrastructure which allows a Composer-based Drupal installation to mimic an archive-download or git-clone based one.
In order to support the build process with Composer, we'll need a plugin that is capable of deploying the various scaffold files and directories that exist in the Drupal git repository to their proper locations.
This plugin will be a very important dependency of the 'drupal/drupal-project-legacy' package. The purpose of the 'drupal/drupal-project-legacy' component is to replicate the current zip file or tarball you download from drupal.org. This allows the Drupal core product to be compatible with the tarball approach, while reducing or eliminating the complications that arise from switching a tarball installation to a Composer-based one. Details: #2982680: Add composer-ready project templates to Drupal core
Proposed resolution
Model the plugin after the community plugin that already exists:
https://github.com/drupal-composer/drupal-scaffold
This plugin should be able to recreate exactly the existing drupal tarballs that we have, and also be configurable/ flexible enough to allow for various kickstart packages to place scaffold files wherever a maintainer may want them. (to support various docroots etc)
This plugin is intended to by usable by distribution maintainers to setup their distros.
Remaining tasks
Determine where this lives in the directory treeLives in the Component directory as a subtree split.Determine what to name this scaffolddrupal/core-composer-scaffold
Determine what features it needs to support, and how that differs from the existing plugin. (existing plugin is independent of core, and has logic for different core versions, this one will share core versioning tags etc)See current documentation page: https://www.drupal.org/docs/develop/using-composer/using-drupals-compose...
Comment | File | Size | Author |
---|---|---|---|
#225 | 2982684-225.patch | 219.67 KB | greg.1.anderson |
#225 | 2982684-210-to-225-interdiff.txt | 3.15 KB | greg.1.anderson |
#214 | 2982684-214.patch | 219.52 KB | greg.1.anderson |
#214 | 2982684-210-to-214-interdiff.txt | 1.83 KB | greg.1.anderson |
#210 | 2982684-3-alt-209.patch | 219.66 KB | alexpott |
Comments
Comment #2
webchickThis is a core requirement of the Composer initiative, so escalating to Major.
Comment #3
MixologicThis exists now:
https://github.com/drupal/drupal-scaffold
The main difference with it and the original its forked from is that every file, including all the README.txts are being synced over.
Comment #4
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedThis is mostly the plugin the same code as the current stable release of the plugin. I removed the support for multiple Drupal versions in the plugin, because we release the plugin together with Drupal core anyway.
Lets see if the tests run on Drupal CI.
Comment #6
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedThe namespace should be updated.
Not required anymore. Should be removed.
Not required anymore.
Comment #7
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedReroll, my repo was outdated.
Comment #9
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #11
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedRemoved the useless assertion.
Comment #13
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedAdding "prefer-stable: true" for faster tests, because otherwise all packages are fetched from git repos because of minimum-stability: dev.
Comment #15
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedJust the CS fix. 👌
Comment #18
MixologicNeed to tell phpunit to put out junit in a folder where drupalci can find it. Im looking now where that should be.
Comment #19
bojanz CreditAttribution: bojanz at Centarro commented1) The plugin has optional integration with hirak/prestissimo. Feels a bit odd to not have a decision here, why not require it if it's nice to have?
2) The plugin doesn't follow Drupal coding standards. CamelCase is used for method arguments sometimes, and phpdoc is generally missing.
Example:
3) The plugin specifies PHP 5.4.5 as the minimum. Why wouldn't this minimum be in sync with core?
4)
$override is a boolean, why doesn't it have a default?
5) README bikeshedding:
I'd switch the two sentences around. Say that both vendor locations (in docroot/outside docroot) are supported, but that outside docroot is recommended.
It was not obvious to me based on name alone what the "initial" key does. Might make sense to have a longer but more descriptive name.
Wasn't obvious to me that this warning applies only when using a -dev version of core. Would make sense to preface the explanation with that.
Comment #20
Mixologicadding
--log-junit /var/lib/drupalci/workdir/container_command.drupal_scaffold/junitxml/drupal_scaffold.xml
to see if we can get drupalci to view the test result.
Comment #21
MixologicComment #23
MixologicWrong patch. Here's the right one
Comment #25
MixologicOkay, here again.
Comment #27
MixologicLets mkdir the artifact dir and put the files there.
Comment #29
Mixologicremove the sudo's
Comment #31
MixologicAdded sudos back in, hacked in a chmod.
Comment #32
Mixologicwhoops
Comment #35
Mixologicmore tweakage
Comment #36
MixologicComment #37
kim.pepperExcited to see this progressing!
My feedback is just basically codesniffer issues. Should we add
core/scripts/composer/scaffold/src/
to the phpcs.xml and do a phpcbf to automatically fix them?s/it/them
Need docblocks for these?
s/it/them
Missing @param docblock
Missing @param docblocks
Needs proper grammar.
Needs docblocks
Needs comments
Needs docblock
Needs description
string[] ?
Needs @return
Needs @return
Needs @return
Comment #38
phenaproximaNice to see this is working, and that Drupal CI is passing its tests.
This is a pretty shallow review; I didn't get all the way through it. There are a lot of coding standards things we need to fix first, before this is really "reviewable", because many methods are undocumented and without that, it's going to be hard to parse what's going on.
Do we still need this?
Can we replace the ellipsis with "etc"? And can we say "requiring" instead of "using", since the key in composer.json is "require"?
What does "name of the Drupal root" mean?
I think that Composer should be consistently capitalized. Shouldn't it be capitalized here?
"...by providing some settings..."
The default source is cgit.drupalcode.org, not drupal.org.
It seems to me that the {path} and {version} placeholders assume a cgit.drupalcode.org-style URL. Perhaps this should be abstracted a bit (maybe specify a class which can be used to resolve the URL of each scaffold file)? If not, maybe we should hard-code cgit.drupalcode.org for now and open a follow-up to reintroduce this option.
I find the combination of 'includes', 'excludes', 'omit-defaults', and 'initial' to be rather confusing. I propose that we remove these options entirely in favor of a single option, called 'files', which explicitly explains what do with each scaffold file -- i.e., where to save it, and when/if to overwrite. It would look something like this:
Obviously, this would break BC with the current plugin. So whether we can even do this really depends on whether we will be taking ownership of the drupal-composer/drupal-scaffold namespace on Packagist. If we are, then this will mean that we're rolling a new major version of the plugin. If we're not, then we can do this with no consequences to existing sites that use drupal-composer/drupal-scaffold.
This should be "composer.json scripts:"
This last sentence needs to be rephrased: "Commit the scaffold files to your repository after running `composer install` for the first time."
Apropos to my settings proposal before -- this would be a new namespace (the current plugin is drupal-composer/drupal-scaffold).
"Plugin" should be lowercase.
Seems to me that this should be moved into the Drupal\Composer namespace, since this is being rolled into Drupal itself, and also because we might eventually add more commands to this command provider.
Doc comment needs to start with a verb.
Can we just call this "ScaffoldCommand"? It's already very freaking obvious that it has to do with Drupal. :)
These need descriptions.
Description needs to come before @var.
These all need doc comments.
The parameters need to be documented.
This should be optionally injectable.
The parameters need to be documented.
Ditto.
Ditto.
These constants need doc comments.
Need descriptions.
Description before @var.
Parameters need descriptions.
This comment could use more information.
Needs doc comment.
Doc comment needs description, parameter descriptions, and @return.
All variable names should be $snake_case.
Comment #39
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedThanks @kim.pepper and @phenaproxima for the reviews. I will fix all the things in the original drupal-scaffold plugin and post a new patch afterwards. I would like to keep both code bases every close together, at least as long the behavior does not change.
Comment #40
AaronMcHaleLove the progress here, keep it up! Just doing a bit of cleaning.
Comment #41
AaronMcHaleComment #42
MixologicI was experimenting with having a lock file in the template, and it turns out that the scaffold plugin does not trigger if theres a lock file.
I had to add the following to the plugin to make it work with the right event:
Comment #43
Mile23Needs docblock and importantly
@group
. Also needs a core test suite or at least some form of test discovery.This is similar to the problems in #2943856: [meta] Reorganize Components so they are testable in isolation but here we have the opportunity to set up the tests the way they should be for a subtree split project. This might be better as a follow-up but maybe not.
Really not sure about the phpunit.xml.dist, because of the way we run tests for core. Do we add a test runner phase where we run these tests independently? Do we discover them and run them in one big run-tests?
Comment #44
MixologicOkey dokes. I did a bunch of stuff on this to keep momentum moving forward.
I've implemented most of the reviews in #37/#38, but there are still some bits that are less "review" and more "architecture decisiony" etc.
#37 1-14 : all done. I did not make any changes to phpcs.xml for now, I just ran them locally.
#38-1: depends on how we test the components/plugins. See last half of #2 of #43.
#38-2: done
#38-3: reworded to be clearer, I hope.
#38-4-6: fixed.
#38-7: {path} and {version} are presumably the two things you would need from any http api to a git repo. Seems like cgit vs gitlab vs github those would all work.
#38-8: I agree that we ought to have a better interface to make it clear the difference between "copy this file once, and never change it, to copy this file always if it changes, vs default omitting etc. I didnt address this because too much other tidying went into this patch, and it seemed like we ought to chat through this.
#38-9: I dont think this should be 'composer.json' scripts. It refers to using it *within* a composer script you write yourself.
#38-10: done
#38-11: yes. This will be an official subtree split into drupal/drupal-scaffold, and shouldnt have much conflicts with upgrades from the old drupal-project/drupal-scaffold. This entire project is for getting new things off the ground, but we should discuss the upgrade path from existing sites to this mechanism.
#38-12: Done
#38-13: I didnt change the namespace, but yes, it should be changed.
#38-14: done
#38-15: no, this is defining a new *composer* command and as such has the potential to namespace collide with somebody elses composer commands that are being defined external packages. So, Drupal ensures we're not going to have collisions.
#38-16-19: done
#38-20: Agreed, injectable, but I didnt do it yet.
#38-21-31: all donezo.
#39 - changes are all pushed upstream do drupal/drupal-scaffold, so maybe we can make a pull request somehow.
#43: I didnt change anything in the testing, so yes that should still be addressed.
This patch cleans up the aforementioned review points, and also introduces an additonal thing: It implements the code cleanup facility from core that cleans the vendor packages. This way, since the scaffold will be the first thing downloaded, it will be able to be called from the template, even on the very first installation from a lock file in a composer project.
There is a travis test over at https://travis-ci.org/drupal/drupal-project-legacy/builds/448029534 showing that this, in conjunction with the lock file idea from #42 means that a template, and a scaffold plugin are all we need.
So one question is how do we make sure this scaffold plugin will satisfy the build needs of a distribution maintainer?
Other questions are the bold points above that we should still address. 1. the api and its design, and whether it needs to be BC with existing scaffold, or whether thats on its own and we need to provide transition instructions.
2. What should the namespace be, if this is external to Drupal/Composer and will live outside of core on its own?
3. What should we do about testing external plugins in general. Maybe now is, or is not, the time to make them testable on their own?
Comment #45
Mixologic->NR
Comment #46
fgmComment #47
Mile23#44.1: If we were going to do a PR for this new work, then it makes sense to say it needs BC. But it's really a fork. I'm not sure there's much difference in behavior, but we should generally try to document what's different. We could even name it drupal/scaffold.
#44.2: How about PSR-4: DrupalScaffold => src? I think it should reside outside of core/, since core is where drupal/core lives, but that's another bikeshed for another day.
#44.3: The changes to drupalci.yml in #44 bring to mind: #2961531: Allow host_command and container_command to create artifacts and #2837112: Add a test definition for a generic PHPUnit test run The big question is whether we'd want that on every test run of core, or whether it should have some other expectation. The test run takes less than a minute locally, so maybe not a hardship.
Comment #48
alexpottIt'd be a shame to have to maintain this list twice. Here and in \Drupal\Core\Composer\Composer::$packageToCleanup
Can we make this a public static and only have it once?
Also having both core/scripts/composer and core/lib/Drupal/Core/Composer is a bit confusing.
Comment #49
Mile23@alexpott We've been discussing exactly that in #2998829-12: Add a composer script method to Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup
Comment #50
Mile23Comment #51
Mile23Poking at #2837112-6: Add a test definition for a generic PHPUnit test run to support an arbitrary phpunit test run.
Comment #52
MixologicThat the hacks to drupalci.yml are merely a proof that the tests we have pass, but are not intended in any way to be part of the eventual patch.
I do not think that testing components in isolation is in scope at all, so I'm working on moving the tests where everything else lives at this point (core/tests/Drupal/Tests/Component).
Comment #53
MixologicOkay, so I took in the feedback from #48, and realized that we don't actually need any of the vendor cleanup scripts in Drupal\Core\Composer\Composer.php, because those exist as a security safety mechanism, and one of the fundamental goals of the composer initiative is that *nobody should be running the git repo in production*. As such, we only need that mechanism to exist when you have the project created with composer originally (via packaging or via composer). So rather than have that in two places, I've removed it entirely.
Also, I moved the scaffold to a component, moved the tests to where they would live, and in this patch, adjusted the drupalci.yml to only run the unit tests (to see if the 2 existing tests pass without attempting to redo testing for components.
What remains to be done here is coming up with a better design for handling existing files. Im wondering whether we need to concern ourselves with existing users of the drupal-project/drupal-scaffold, or whether that can come later, like a script to help people transition from one to the other.
Comment #54
MixologicAlso, I left in a 'deprecation notice' for the vendortestcleanup script in
Drupal\Core\Composer\Composer.php
This is because people have taken to meddling with their drupal/drupal composer.json, swapping the 'replace' with 'require' for drupal/core, and as such would end up with calls to these scripts even after an upgrade, and if that script call completely went away, composer update would crash for those users.Comment #56
Mile23Well, hang on a sec, because we're still going to allow people to use Composer to put their vendor directory in the docroot. In fact, that's the first template we're going to make, right? So we still need to do the cleanup.
Let's do the move-or-remove as a follow-up. Then we have the scaffold in core sooner and we can iterate sooner and so can drupal-composer.
Add composer/composer as a dev dependency. Then we're back to exactly why we should test these in isolation, and why merge is bad. #2943856: [meta] Reorganize Components so they are testable in isolation
Comment #57
fgm@mile23 about 1. are we, really ? is there any remaining reason to promote that template versus the docroot one level down ?
Comment #58
Mile23@fgm: #2982680: Add composer-ready project templates to Drupal core
Comment #59
Mixologic#1 We still do the cleanup. Its been moved into the scaffold component, and is a plugin now. We dont need to do the cleanup when somebody runs composer install on a git clone of the drupal repo, so we no longer need the scripts in Composer.php. And we need it in the scaffold in order to "allow people to use Composer to put their vendor directory in the docroot", thus it cant be a followup.
#2 While it would be nice to test these in isolation, and nice to get rid of the merge, thats not the situation we're dealing with currently, and can be postponed until after we get this composer work done. Testing the components in isolation has a whole slew of implications, and is a major undertaking itself.
In trying to get the tests to pass locally, we might need to figure out better testing. Right now the scaffold tests assume that it is being tested in isolation, so we'll probably need to redo the way we test these.
Comment #60
fgm@mile23: thanks I can't believe I missed that issue.
Comment #61
Mile23Fixed the assumptions baked into PluginTest, so now it knows about the rest of the core codebase.
It still fails locally, but for real reasons. I have to stop here for the day or I'd investigate further.
Also added composer/composer as a dev requirement.
Re: #59.1: I don't see where the removal of test files is performed.Never mind.Comment #63
Mile23PluginTest now runs Composer in a separate process, and the Scaffold component now has a classmap instead of PSR-4.
That's how I got the tests to pass locally.
We'd much prefer to have PSR-4 since a classmap for our plugin will eat up memory on every request.
Comment #65
Mile23I think we need to find a tmp dir within DRUPAL_ROOT/sites/simpletest if we're running under the testbot. This means adding that case as a conditional, since the test should also be able to run without the rest of the core codebase. (It's a component, after all.)
Scaffold's composer.json should suggest hirak/prestissimo.
Comment #66
Mile23The test tries to use the simpletest directory.
Comment #67
Mile23Restores the full core/drupalci.yml.
Comment #68
Mile23Somewhere from #63 we're able to get the plugin classes to load, but it's also quite strange that we need to use a classmap instead of PSR-4. This is the main not-ready-for-prime-time aspect of this. This is only to get it to run under test. It works fine as PSR-4 in other circumstances.
Comment #69
larowlani might be missing something here but shouldn't this be double quotes so that
$relativeVendorPath
is replaced?Comment #70
Mile23Good sleuthing but that's inside a heredoc, so the variable should expand.
It turns out I never did try reverting to PSR-4 after isolating the Composer calls within their own process. So here's a patch with that, and it works locally. Will the testbot disagree?
Also CS from #67.
Comment #72
pingwin4eg@Mile23 you've changed PrestissimoFileFetcher constructor arguments order, but not in the caller. Here's the fix.
Comment #73
Mile23Yah you get to the end of a thing and you make that kind of mistake, thanks.
+1
Comment #74
Mile23Patch still applies, re-running test.
Comment #76
timmillwoodMy concern here is it relies on cgit.drupal.org, see https://github.com/drupal-composer/drupal-scaffold/issues/71 for where this has been an issue.
Comment #77
vijaycs85Comment #78
vijaycs851. Rerolled against 8.8.x
2. Added draft CR and updated comment.
Comment #79
Mixologicre#76: we're about 2 weeks away from cgit being replaced with gitlab, which should be tremendously more stable, and will have all appropriate redirects.
Comment #80
larowlanare these changes intended? bad reroll?
Comment #81
vijaycs85@larowlan definitely exist in #72 and #70.
Comment #82
Mile23Same for seld/jsonlint.
If you say
composer why composer/composer
, it tells you nothing depends on composer/composer. And that's because we merge the scaffold instead of requiring it.Here's why we have composer/composer:
Comment #83
amateescu CreditAttribution: amateescu commentedjustinrainbow/json-schema
was just committed to 8.8.x (and soon to 8.7.x too) as part of #2843147: Add JSON:API to core as a stable module (see #3036210-33: Add justinrainbow/json-schema as a composer dependency so JSON:API can use it to validate its responses), so this patch needs a reroll :)Comment #84
Mile23Needed re-roll per #83.
Comment #85
Mile23Updating IS a little.
The 'needs followup' tag was from #49, and refers to #2998829: Add a composer script method to Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup which we've decided to implement as part of the plugin here.
Adding 'needs change record update' because the change record is a stub.
Comment #86
Mile23Updated change record.
Comment #87
Mile23Needed re-roll.
Comment #88
MixologicSo, documenting some impending changes wants:
@phenaproxima wants to polish the design of the configuration of the scaffold to make the includes/excludes/defaults a more sensible design.
@alexpott would like to see a way for the scaffold to have a fallback for when retrieving files it has somewhere else to pull from.
Potentially instead of retrieving the files from drupal.org git repo we should take anything that people want to scaffold, and move them into a subdirectory of /core
And if we have a plugin that is capable of "moving files around" then maybe it should be able to "move any files around" as part of composition such that we can eventually use it to solve the assets
Comment #89
Mile23Based on heroic efforts by @grasmash, @Mixologic, @greg.1.anderson, @phenaproxima and others at DrupalCon 2019 in Seattle, we have new patch based on work being carried out in https://github.com/grasmash/composer-scaffold
This new patch addresses concerns from #88.
This patch is part of a WiP which will likely reflect more 'upstream' changes in the near future. Presented here in order to get an opinion from the testbot, and any early review people would like to offer.
Comment #91
Mile23Unrelated fail, restarting test.
Comment #92
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAll of the issues in https://github.com/grasmash/composer-scaffold have been resolved. Thanks @mile23 for offering to re-roll the patch. Thanks to everyone who collaborated on this at DrupalCon Seattle and afterwards.
Comment #93
Mile23Let's try this out. :-)
I have a fork of gramash/composer-scaffold over here: https://github.com/paul-m/composer-scaffold/tree/component This branch contains all the changes you can do in order to make the repo look like a Drupal component, without actually making it a component. You can see it passing tests here: https://travis-ci.org/paul-m/composer-scaffold/builds/534277774
A couple things to note about the patch:
require-dev: "phpunit/phpunit": "^4.8.35 || ^6.5"
and this is a lie. The tests use PHPUnit 6 idioms, but we can't merge the component unless its constraints mimic those of core. Until core drops PHPUnit 4 support formally we can't be honest about the scaffold.Significantly different from #89, so no interdiff.
This is the work of @grasmash, @Mixologic, @greg.1.anderson, @phenaproxima, @jhedstrom and probably a bunch of others. :-)
Fingers crossed....
Comment #95
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThree sets of errors in the previous patch. The first and last appear to be unrelated. The middle one is caused by some scaffold tests that use git; apparently, the drupal.org test runner needs a little more setup than TravisCI does.
Comment #96
Mile23The first fail was DrupalComponentTest, which was only making sure that our license files are identical. (They are now.)
The second was from ManageGitIgnoreTest, where we're making commits to a repo we created in the test. I've addressed it here, but it might be better to get DrupalCI configured with a global git user, because we're probably going to be doing this kind of thing a lot after #3031379: Add a new test type to do real update testing
The third is a big ol' shrug, and I think it's unrelated. Let's see if it fails again here.
Comment #98
Mile23Well, it does help to actually read the fail message sometimes...
So that's YamlTest discovering the scaffold test fixture files, and not being pleased with the way they are formatted. The files it found contain only a comment line.
This passes locally, which leads me to believe it's YamlPecl throwing the exception since I don't have that extension locally.
So the question is: Do we solve by adding a key to scaffold's fixture files? Or do we solve by figuring out why YamlPecl fails on valid YAML files? #3003300: PECL YAML errors at valid YAML file which only contains comments
Comment #99
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAh, that is our thing then. Cool.
So the thing about the fixtures is that they use filenames that are the same as files in Drupal, but they don't necessarily have vaild contents, since we are just checking to see if the file was placed correctly. My recommendation would be to just copy a profile.default.services.yml from somewhere else. Make sure the existing comment stays in the file. That should resolve things.
Comment #100
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedActually profile.default.services.yml is just something we made up. It would probably be fine to just put one key `foo: bar` in here to satisfy the yml linter. Just guessing from the message, didn't actually look at the code.
Comment #101
Mile23Added dummy keys to fixture files, since there's an issue for dealing with YamlPecl.
Comment #102
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedSet to RTBC based on the following criteria:
- Tests are green
- Patch is a faithful conversion of https://github.com/grasmash/composer-scaffold
- Community members have provided a lot of feedback on aforementioned repository in the last two Composer in Core Initiative meetings. See https://www.drupal.org/project/composer_initiative/issues/3053800 and https://www.drupal.org/project/composer_initiative/issues/3051086
I think this is ready for a core committer to look at.
Comment #103
Mile23Thanks. Caveats in #93.
Comment #104
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOh, yeah, I forgot to comment on the phpunit 4.x issue. It would be easy enough to fix the tests to work with phpunit 4.x. There are only a couple of convenience methods used. I'd be happy to switch these over to equivalent phpunit 4.x APIs if that would make things better for core. (n.b. existing tests work fine with phpunit 5.x, which is how we tested php 5.x in the github repo.)
Comment #105
alexpottMissing docs.
That's a lot of allow in one sentence. I think this is a @todo and can be clearer about what might be done.
Potentially this could be a bit more descriptive. It took me a couple of seconds to work out you are referring to the DRUPAL_ROOT autoload.php file. For example.
Other composer commands don't call the parent here - which goes to the Symfony class and is empty. I don;t think we should either.
Maybe it is worth using the setHelp() function - something like https://github.com/composer/composer/blob/522ea033a3c6e72d72954f7cd019a3...
This package? Is this a package or a class? Or should it be a "package listener"? Also this sentence is very long and wordy. For example, "manages examining" could be "examines". I think maybe something like:
This package listener updates the allowed packager manager whenever a newly-required package contains scaffolding instructions.
might be better.Looking at this some more I'm not sure we should subscribe the allowed package manager to the event and remove this class entirely - it seems to exist for the single condition which calls a static. And imo doing this can fall under the allowed package manager's responsibilities without messing up a clean architecture.
Too long comment.
This needs improvement. And the second comment is long.
Missing docs
I think metadata normalisation belongs inside the create operation and not on the public API of the operation factory. Then the factory has one bit of public API which is the operation creation which feels pretty good for a factory class.
I'm not sure we need a stateful
GenerateAutoloadReferenceFile
class and also a staticautoloadPath
on the ScaffoldFilePath object. We could have a single static method on aAutoloadGenerator
object that takes the$this->rootPackageName(), $this->getWebRoot()
arguments and does the business and returns a ScaffoldResult object. That the autoload generation will be encapsulated in a single class.$manager is very generic.
Let's call this create - the class name is OperationFactory - we're creating operations.
Let's not maintain these in components just yet.
Comment #106
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThanks for the feedback, @alexpott. I'll start working on making those changes. Just one bit of clarification:
> Let's not maintain these in components just yet.
Was that comment intended to refer only to phpcs.xml.dist and phpunit.xml.dist? We need composer.json here for the subtree split, right?
Comment #107
alexpott@greg.1.anderson yep we have composer.json in all the other components - we just have not done phpcs or phpunit config files before and this doesn't feel like the right issue to do that in.
Comment #108
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI only had time to make the following changes right now:
5. Removed call to parent.
8. Removed DetectAddingPackagesWithScaffolding as suggested.
12. I agree that this does not belong in the factory. I was sort of inclined to make a static class just to hold this one method, but instead I put it in the Handler class as suggested.
13. Altered as suggested. Did you want to make the vendor path also be a parameter to generateAutoload, and make the class static?
15. Renamed method to 'create' as suggested.
I will continue; here's a patch in case someone else wants to pick up some of this.
Comment #109
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI accidentally added composer.lock, so you might want to ignore #107/#108, and continue reviewing #101. I'll fix it up after I make more changes. Setting to 'needs review' so that the test bot will run.
Comment #111
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI have a fix for the testUnmanagedGitIgnoreWhenGitNotAvailable failure that I will include in my next patch. I'll also provide an interdiff next time.
Comment #112
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedMore stuff.
1. No worries! Thanks for reviewing!
2. Added docs for these class fields, and also for any other class field that was missing docs.
3. Clarified that what was proposed here was prompting the user. This functionality is optional; we could also remove the todo.
4. Rephrased this comment to be more succinct, but kept it brief.
6. Added a brief setHelp call with some placeholder text. Might need to iterate on contents here.
7 + 9. This class went away.
11. See 2.
14. Used a more specific name.
16. Removed these, along with some supporting information in composer.json etc.
So, #6 at least is still TBD, but I am expecting there are probably going to be more comments coming per #105, so I'm going to drop of another patch here and iterate on the help text along with those.
Comment #113
grasmash CreditAttribution: grasmash at Acquia commentedThank you for converting the contrib project into a core patch @greg.1.anderson!
Comment #114
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@Mile23 did that in #101 - I just addressed some of the review comments.
Comment #115
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedSame as #112, but with better help text. No interdiff, because this is nearly the same as #112. See the interdiff in #112.
Setting back to RTBC to indicate that all of the review comments given so far have been addressed, and we're ready for more.
Comment #116
alexpottNo need for this to be public anymore. Or even really exist. We could do this in the event method with no loss of clarity.
I think these lines are superfluous.
I guess we shouldn't be pointing to github (or gihub). Can we put a placeholder page somewhere in the drupal docs?
There's no need for GenerateAutoloadReferenceFile to be a concrete object. We could have a class called
AutoloadReferenceFile
which has one static method calledgenerate
which takes 3 arguments - $vendor_path, $web_root, and $root_package_name and I'm not sure that $root_package_name is actually required because ScaffoldFilePath is used unnecessarily in the object atm. Ah I see. You need it because ScaffoldResult needs it. Anyway the point still stands that GenerateAutoloadReferenceFile is stateful and exposing of API neither of which seems to be required.No comments on the top constants and there's a lack of spacing.
We can initialise on the property definition.
Can be protected - only called from :: getFileMappingsFromPackages() - also perhaps these tow methods should be closer in the file.
The normalisation of $value can occur inside the OperationsFactory - this doesn't feel like it should be the concern of the general scaffold command.
Is it worth noting to the user that scaffolding has done nothing. I think maybe we'd want this if you call the scaffold command yourself.
This is very dense some new lines before the comments wouldn't go amiss.
This is only publicly called from a test so should it be part of the public API?
This can be protected. No public usage.
This is interesting. Why is it necessary to create the vendor dir - I would have thought that that is composers job.
Is this
string[]
Too long and needs to begin with something like
Allows
Too long. Needs that active verb with an s too.
Too long and active verb with s
replaced can be on the previous line.
Too long
Not flowed correctly.
As above.
Too long and Finds...
Gets
Determines
This can be said with less words:
TRUE if the specified file is ignored, FALSE if not.
Can be protected.
Determines... but be careful of length of the comment.
Determines
Can be protected.
This construction results in unnecessary git operations. If $isIgnored is TRUE there's no point to checking the tracking status.
Can be protected
No need for this to be public and changing a file in a getter is surprising - given this is only used from addToGitIgnore how about doing the \n manipulation there. There's part of me that feels this method is unnecessary and should be folded into addToGitIgnore since the behaviour of returning '' for non existent files is also surprising.
Gets
Should be fully qualified.
Missing fullstop
Needs a verb at the start.
fully qualified classname
Missing full stop.
Creates...
Ensures.
Missing @return
Comment #117
alexpottRather than have someone else do many of the nits from #116 I'm going to do the nitty stuff myself.
I'll un-assign myself once I've got a new patch up. I'll not touch larger architectural stuff like #116.5
Comment #118
alexpottSo here's a full review of nits and minor stuff in the component - I've not looked at or touched the tests yet. Only confirmed that my changes still result in a pass. I've tried to address very small architectural things where I saw them. In this initial patch we should try to keep things a protected as possible. Protected to public is easier to change than the other way around.
I didn't do #116.5 so someone else can feel free to have a go at that. Or #116.31 which I've not addressed.
We still need to look at #116.14 and .12 and .10 and .4
I will try to post more review in the coming days.
Comment #120
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThanks @alexpott, I appreciate the review and the fixup. I'll pick up the remaining items identified so far in #116.
Comment #121
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented4. I still need to make a placeholder page and write some preliminary docs for this.
5. Made GenerateAutoloadReferenceFile a static class.
10. Added a warning when scaffolding is not enabled for any packages.
12. Removed 'getWebroot' method entirely, as it is not needed. This had the side-effect of also removing the last unit test in the project, so we now have only integration and functional tests.
14. The `ensureDirectoryExists` on the vendor directory was overly conservative. Typically, the code that runs this check will be **inside** the vendor directory at the time the check is made, so it's sorta-kinda-pointless to take the time to create it. I removed that call.
31. Added an extra level of nesting to avoid making an unnecessary git call.
I did not confirm that the issues in #116 not called out in #118 were fixed in #118. We're ready for more feedback again -- although note that I did add one @todo, so we're not strictly speaking done. I still set this to "needs review" so that the test bot will run.
Comment #122
alexpottSo I think that operations should be immutable value objects. I.e there's no need for any setters. This could all be a constructor.
I'm not a fan of this trait and interface. I wonder if we could architect this to have a conjunction operation that joins two operations together in its process and has a constructor which takes two operations.
Same here
These are used in tests and internally - I think we should not have them can use the properties internally.
Can be protected
In some ways I think we could have just one method here and less state on the object. We never call one method without the other. And calling process before collate would not make any sense.
Comment #123
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented1. I like the setters because they are more readable than constructor parameters. Immutable objects is a fine pattern, though, so I went ahead and converted the operations plus ScaffoldFileInfo and ScaffoldResult, while I was at it, to be so.
2. Yeah, that did work. Adjusted as suggested.
3. Done.
4 - 6. Makes sense, but out of time for now. TBD.
Also note that item 4 in #116 also remains undone. Making a patch in case someone else wants to pick up here. Setting to 'needs review' so that the tests will run.
Comment #124
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedReroll.
Comment #126
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedForgot the lock file.
Comment #127
Mile23121 -> 126 interdiff.
Comment #128
Mile23In this Composer initiative meeting we decided to have a separate plugin/script for vendor cleanup: #3053800: Agenda for 5-15-2019
So now we also have this issue: #3057094: Add Composer vendor/ hardening plugin to core
Comment #129
Mile23Added a stub documentation page which is just a copy/paste of README.md: https://www.drupal.org/docs/develop/using-composer/using-drupals-compose...
Updated the CR with new scope (since it doesn't do vendor cleanup now): https://www.drupal.org/node/3041017
Comment #130
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented4. Removed accessors.
5. Made the method protected.
6. Made `collateScaffoldFiles` and `processScaffoldFiles` protected, and created a new public method `process` that calls them. Removed fields $listOfScaffoldFiles and $resolvedFileMappings and had `collateScaffoldFiles` instead return their values for use in `processScaffoldFiles`.
Also resolved item 4 in #116 by pointing to the placeholder page that @Mile23 created in #129.
This resolves all review notes to date.
Comment #131
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedCoding standards.
Comment #133
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#131 failed on the first run because space aliens; it passed after re-queueing. Ready for the next round of reviewing.
Comment #134
Mile23Opened this issue: #3057459: Add composer/composer as a dev dependency of core so that we can test Composer plugins
We need to go through the add-a-dependency process for both this issue and #3057094-7: Add Composer vendor/ hardening plugin to core
Comment #135
alexpott#131 is updating all the composer dependencies - probably for PHP7 but that shouldn't be happening here.
Comment #136
alexpottThis text can be on the previous line
by this
will fit.Let's make this final and add a private constructor. That way this stays a class that only provides static methods and they are its API.
Missing $vendorPath documentation.
$this->handler->scaffold() and we can remove the Handler::onPostCmdEvent() - scaffold is going to stay public. Less code ftw.
This seems funny to do in methods. I think this would be better to do like this.
Again less API and code and easier readability.
Can remove this imo cause keeping this inline with core will be a pain.
phpunit 4 is no longer supported. Also we've not added this anywhere else.
It looks worth documenting these params.
The patch attached does the above.
Comment #137
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented2. 👍
4. 👍
5. 👍 Existing code was premature abstraction that is not necessary here.
6. 👍 Not sure why this was here.
I think it is undesirable to use `instanceof` with a concrete class instead of an interface. Even though we currently have only one operation that can override another operation, it is preferable to maintain the interface for the instanceof test, even if it is only a marker interface (with no methods). In this one respect I prefer the previous implementation. I am mostly indifferent toward the new implementation where process is called twice (once for each op) rather than calling a new method of the (formerly called) preprocess interface. The changes here are an improvement in all other ways. The renaming to ConjuctionOp is better, for example.
I am, of course, 👍 on charging forward here as presented if you disagree with my disagreement (can't use the marker interface without changing the factory, so the utility is stylistic only), so I'm not sure whether to move this to "needs work" or "rtbc". LMK what you want to do.
Comment #138
Mile23I think basically this and the vendor cleanup issue are postponed on #3057459: Add composer/composer as a dev dependency of core so that we can test Composer plugins so that we can have composer/composer as a dependency.
Comment #139
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAs discussed in the Composer in Core Initiative meeting:
- This issue should not be blocked on #3057459: Add composer/composer as a dev dependency of core so that we can test Composer plugins.
- My comment in #137 can be addressed as follow-on work, as it is not important.
Back to RTBC.
Comment #140
phenaproximaSorry to derail this temporarily, but about #137: the reason why we need the
instanceof AppendOp
is kinda obscure (it was explained to me in Slack and to be honest I didn't quite grok it), so I think we should add a comment explaining why it's there.Comment #141
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAlso the marker interface I mention in #137 is necessary, because ConjunctionOp needs to implement it as well. We should also have a test demonstrating that appending on top of an append still works.
Comment #142
alexpottPatch attached adds the comment @phenaproxima requested to explain the ConjunctionOp stuff. The patch also cleans up all the usages of camelCase in method parameters - it Drupal core these should be snake_case. Also gone through the tests for coding standards. I've not done much reviewing of the tests themselves (yet).
We still need to ensure we have test coverage for appending on top of an append.
Comment #143
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI have made a test for appending on top of an append. I was offline; I'll need to roll it in to #142.
Comment #144
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's the append-over-append test I mentioned in #143. Also, note that it was not necessary to make ConjunctionOp implement the interface, as only the new op (in our case, only the append op) needs to be marked as "conjoinable". I added the marker interface here since I feel it better style, even though it is not necessary from a functional standpoint.
I have more scaffold test cleanup to do that I will post later.
Comment #145
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis patch is the same as #144 with all of the same tests, except that the file ScaffoldText.php has been reorganized to be more readable / rational. I have included an interdiff, although the file changed so much that it's probably better to just read the new ScaffoldText.php and ignore the diff.
Ready for the next round of reviews.
Comment #146
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThere were a few coding standard errors in #145. This fixes those, and also converts two tests into one with a data provider.
Comment #147
kim.pepperComment #148
MixologicI'll take on this reroll. Should be easypeasy.
Comment #149
MixologicRerolled to remove the composer/composer dev dependency.
Comment #150
MixologicI think Im 0/∞ in remembering to reset the status when I upload a patch.
Comment #151
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWhile working on an experimental drupal/drupal layout project using drupal/drupal-scaffold, I discovered two bugs in this patch:
1. If there is no "location" in the composer scaffold 'extra' section, then the destination path is defaulting to '/' instead of './'
2. The scaffold operation only runs on 'composer update' (or if explicitly called). It does not run on 'composer install', as it should.
I'll fix these bugs shortly.
Comment #152
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFixed the missing 'web-root' bug. The expected result was to get an error message if no web-root was defined, but the actual behavior was a "permission denied" on a bad path. I'll make a patch with the fix after I fix the 'composer install' bug as well.
Comment #153
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's a patch + interdiff with both bugs mentioned in #151 fixed.
Comment #155
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFailed to add the new file to the patch in #153. Here's the same patch, but with all of its parts included.
Comment #156
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented... and now with the correct status too.
Comment #157
Mile23Just a minimal review from some time reading the code...
Should expect a \RuntimeException.
Add the word 'configuration' before 'is'.
Nit: This sentence could be clearer...
Give some context for this error... Like what file wasn't found and which package needs it.
"We use a handler object to separate..."
Comment #158
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAddressed comments from #157.
Regarding #157.4, this exception is logically impossible to throw; it is only included to catch errors introduced by the caller. I enhanced the information in the message anyway.
Comment #159
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThe documentation stated that the `web-root` location defaulted to `.`, but the code required it to be set. This patch makes the code match the documentation, per @mixologix @phenaproxima & @grasmash.
Comment #160
MixologicReviewed all the interdiffs, Im certainly pleased with the outcome of this, and really excited to get this into core.
Comment #161
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAdding @grasmash and @phenaproxima to draft commit credit.
Comment #162
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOK I guess only core committers can modify that field. I recommend adding @grasmash and @phenaproxima and @jhedstrom based on their work at DrupalCon Seattle. (Not sure if we can credit someone who has not commented on the issue, though.)
Comment #164
dwwI think since I am/was a core subsystem maintainer, my account seems to have perms to update credits for core issues. It is possible to credit people who didn't directly comment on the issue.
Yes, excited to see all the progress here! This is fantastic. No time for a careful review or testing, but some of the luminaries of our community have worked on this, reviewed and tested, so I'm confident with leaving the status as RTBC.
Thanks to everyone who helped on this!
-Derek
Comment #165
Mile23Updated CR a little bit for clarity. https://www.drupal.org/node/3041017
IS was a little behind as well.
Comment #166
larowlanThis looks great, the docs are thorough, the feature-set is awesome and it is a huge step to get us to a better place for composer tooling, especially the feature that consideration for people who used a tarball moving to composer. Sorry the review took so long, its a big patch
nit: we could just return here without the assignment
nit: > 80
should we check the file doesn't exist before we write it? Is it possible folks might have added some modifications and/or have this in their version control?
we have a mix of camel and snake case here - should we be consistent?
nit: do we need the intermediate variable here?
nit: this else isn't needed, we return above
nit: normally this should start with a verb, e.g. Defines or in this case, probably Injects
Is there anything we can re-use here in our existing Token code? Should we be creating a token component and making the core utilities extend from that? Happy for that to be a follow-up if so
should we be documenting that these values are an associative array, i.e the keys are significant?
Now we're no longer targeting old versions of php - should we be using the null coalesce operator here instead?
should we document the return value and delineate between an operation that did something and one that did nothing?
should we have a local variable here to avoid calling this three times?
According to the symfony docs we should be passing these arguments using array syntax to allow symfony/process to deal with irregularities between different flavours of shells - is there a reason why we don't?
nit: do we need the intermediate variables here?
should this be files and not filed?
nit: mix of snake and camel case here too, should we standarize? - the patch has a mix of both throughout
Are we sure we want to add this comment? People can work out what implementations there are - it feels like something that could get out of alignment quickly
TYe » The?
in some instances we use the interpolation for this, but here we don't? is there a reason why we mix the two
anytime we have to do a multi-value return, its normally an indication that there's a value object we're missing - is that the case here?
nit, > 80
do these warrant constants?
In most cases we're using value objects, but we're modelling $metadata with an associative array - is there a reason?
nit: this else isn't needed
nit: does the
== TRUE
add anything here?we're mixing native php file operations and symfony here - any reason not to use symfony/filesystem's copy to bypass vagueries of various OS's and stream wrappers?
any reason not to use ::class constant here?
jon » json
❤️ - these docs are awesome 💪
there is only one usage of this - any reason not to inline it in the calling code?
any reason not to use phpunit's stringContains here? looks like that's what we're using it for anyway, none of the regex appear to be anything other than plain strings
should we just return the process and leave fetching the stdout and stderr to the caller? gives them more options? multi value array returns are not pretty
any reason we can't use vfsstream here instead of generating actual directories? Realise that's a big pivot so happy if that is a follow-up to explore feasibility.
is it worth documenting that this is a string rather than a boolean because its being replaced with interpolation?
why do we hard-code $is_link to FALSE but then have logic for it being in different states? can't we just hard-code that too?
should we assert this operation succeeded using !file_exists before we re-run to avoid a false positive? i.e. we think we deleted the file in the test and that it was re-added with the next run, but in fact, the first one was not removed.
nit > 80
is this wrapper needed?
this return is not documented, and also, a multi-value return always feels like a code smell - should we have a value object here?
nit: >80 here
oh, here is one using the regex format. Should we just split into an array of expected strings instead of overloading with regex? - it will make future debugging easier when the assert will fail with the missing line instead of the blob of multiple lines
should we also be checking for the specific content in the file, e.g. they all contain 'from drupal/core' but we're not really checking that the correct file was copied if we don't vary the expected text - we should be able to build a unique expected string as each file also contains the file name as well as 'from drupal/core'
Comment #167
larowlancross-post
Comment #168
dwwFixing status from x-post based on the 42 points from #166. ;) Thanks for the thorough review!
Not sure that @Mixologic is going to work on addressing #166, so unassigning for now.
Cheers,
-Derek
Comment #169
jibranI don't see a reason why Drupal FileSystem can't be used to achieve the same result.
Comment #170
Chi CreditAttribution: Chi for DXPR commentedI suppose it is not reliable to access Drupal container in that script.
Comment #171
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI will address the issues in #166, probably by end-of-day.
#166.3: I don't see any reason why anyone would customize this file, and I think it would probably be a bad idea. However, it's possible today, and something that's possible might likely be done. I'll change the code so that it skips rewriting the autoload.php file if it exists and has been added to the repository.
#166.8 / #169: I think it's best to avoid using code from the project (in this case Drupal) from within a Composer plugin. While technically possible, bad things can happen, e.g. if you call some Drupal code prior to `composer install`, or if code changes out from under the plugin during a `composer update`. The later was the cause of an inconvenient upgrade bug in drupal-composer/drupal-project in the past.
#166.10: We were using a bunch of PHP 7.0+ features in this project in an earlier incarnation, but had to roll them back because it's my understanding that it's still core policy to code to PHP 5.6+ to make backporting easier.
#166.19: This exception is logically impossible to ever be reached (unless the code in the caller grows a bug), so I guess the answer is "laziness". Originally this exception didn't have any variable references at all. I can fix it up for consistency anyway.
#166.20: Yes, a value object would be better here. However, this method is only used internally to the class, so it seemed like too much effort to break out the code for a class file that's already pretty short. I won't do this refactor unless someone feels strongly that it should be done.
#166.23: Metadata comes to us from Composer `extra` as an associative array. We could make a value object wrapper for this if it was felt to be necessary.
#166.33: Scaffolding involves creating a lot of files. I'm not sure there's a convenient way to use vfsstream for these tests.
#166.34 / 35: I am tempted to remove symlink as a replacement and just duplicate any fixture where both the symlink and non-symlink variants are used. In some cases I used variable names for some things that are hardcoded because I hadn't yet decided if each given test needed a data provider to improve coverage. At this point I think the coverage is pretty good; your opinion on that prior to simplification of the code would be appreciated vis-a-vis symlink permutation coverage. It should work about the same way for most of these tests, so I tend to think we don't need to spend the time to do more permutations than already exist.
#166.39: I agree that a value object would be better here too, but I tend to be more casual with the test fixture code. I'll make a value object if you think it would be better.
The rest I will address as suggested, unless I run into some issue when I'm fixing that I didn't consider when skimming the suggestions.
Thanks for the thorough review - I appreciate it. Please do not feel shy about asking me to do any of the things ^^ that I think are not-so-important if you really do think they are important.
Comment #172
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@larowlan: I'd also like to suggest that @phenaproxima, @grasmash and @jheadstrom be re-added to this list of folks receiving commit credit here. Although they do not have any patches on this issue, they all spent > 1 day at DrupalCon Seattle working on the code that went into this patch.
Comment #173
phenaproximaAttempting to add credit (not sure if I can do that, but I'll give it a shot).
Comment #174
dwwRe: #172: @grasmash, @jhedstrom and @phenaproxima all have credit on this issue now (since #164), unless a core committer undoes the change. Their checkboxes are selected by default when I view the form element. All this code is weird, and I don't fully understand all the edge cases for the behavior. *shrug*
Re: #171: mostly agree with all you've written. However, for #166.10, I don't see the core committers back-porting this to 8.7.x and earlier. I suspect that's too big a change inside a "stable" release series. So I don't see any reason to support PHP 5.6 since 8.8 already forces PHP7. Release manager input would obviously override this, but I suspect this is *not* going to be backported. Hopefully I'm wrong. ;)
Cheers,
-Derek
Comment #175
dwwp.s. @Mile23: can you confirm if https://www.drupal.org/node/2982684/revisions/view/11467471/11467626 was an accident? Seems like @larowlan in #166 undid your changes from #165. Tentatively restoring those fixes with this comment. Please re-fix if I've messed anything up.
Thanks,
-Derek
Comment #176
Mile23Re #175
Yes, that looks like an accident. Thanks for fixing it.
Comment #177
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@dww: I understand your reasoning regarding #166.10, but please see the comments in https://github.com/grasmash/composer-scaffold/issues/36#issuecomment-490.... I don't want to go back and forth on PHP 7.0 support. Since we already took it out once, let's make further adjustments as a follow-on issue.
#174: Commit credit was added in #164, removed in #166, and re-added in #173. I think it is now 👍
#166.13: I tried to make this change, and made a shocking discovery. Since the scaffold tool runs as a Composer plugin, any class that Composer uses before our plugin is called will continue to be used by our code. Composer 1.8.5 uses Symfony/Process 2.8.48. This means that we cannot use the array variant of the Process constructor, because is is not supported in Symfony/Process 2.8. The other implication of this is that when a new version of Composer comes out that uses a newer version of Symfony/Process, then we'll spontaneously start using the newer API, regardless of what is in our lock file. Because of this, I am wondering if we should remove all calls to Symfony/Process (and potentially other Symfony componets as well) from the scaffold plugin.
Comment #178
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedPer @phenaproxima, we should use ProcessExecutor and Filesystem from the Composer API here instead.
This is a good solution, so I'm going to move forward with it throughout the plugin.
Comment #179
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#166.20: I am refactoring the code to avoid the multiple return values. This is in progress.
Comment #180
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis is taking me a while, so I'm posting a progress patch.
Addresses some of the review comments from #166:
- 1. Simplified
- 2. Wrapped
- 3. Added check to see if autoload file committed to repo
- 4. Converted all local variables that were camelCase into snake_case
- 5. Removed intermediate variable
- 6. Removed 'else'
- 7. Improved comment
- 8. No change; not safe to use Drupal code from a Composer plugin
- 9. Improved phpdocs
- 10. No change; not using PHP 7+ yet
- 11. Not sure what to do here; caller does not act on return value.
Could potentially print some progress output when managing .gitignore
- 12. Used local variable to store accessor result
- 13. Converted to use Composer Filesystem instead of Symfony Filesystem for
safetly / stability. Composer Filesystem does not support array syntax.
- 14. Removed unnecessary intermediate variables
- 15. Fixed typo
- 16. Changed camelCase into snake_case
- 17. Removed unnecessary comment
- 18. Fixed typo
- 19. TODO: No change yet; didn't want to instantiate an interpolator for
an exception that will never be thrown. Maybe this should be deleted?
- 20. TODO: Not sure how to resolve; still considering.
- 21. Fixed wrap
- 22. Added constants...
- 23. ... and renamed 'metadata' to OperationData and created wrapper object for it
- 24. Removed unnecessary 'else'
- 25. Remove redundant `== TRUE`
- 26. Use Composer::Filesystem instead
- 27. Used ::class constant
- 28. Fixed typo.
- 29. Thanks!
- 30. Inlined getInterpolator() method
Still need to do the tests, and a couple of the items as noted above. Setting to 'needs review' just to ensure that the tests run.
Comment #181
Mile23You mean composer's ProcessExecutor?
Re: #177:
You're referring to the version baked into the composer.phar file... Interesting.
Comment #182
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@Mile23 Yes that was a typo. I did mean Composer's ProcessExecutor. I used Composer's Filesystem elsewhere as well.
Comment #183
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedProgress patch:
#166.19: The scaffold file parameter already has an interpolator, so this suggestion was actually trivial once I noticed that. (doh)
#166.20: I was reluctant to make a class just to hold two values; however, I discovered that if I factored out the responsibility for collating the scaffold files into a separate class, then the second value could remain protected in that class, and did not need to be exposed to the caller at all.
Still need to address review comments on the tests (#166.32-42)
Comment #184
yogeshmpawarComment #185
yogeshmpawarFew coding standard issues fixed & added an interdiff between patches.
Comment #187
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@yogeshmpawar, thank you for taking the time to contribute to this patch. I'm skipping #185 for now because it's not clear to me why it is failing, and I don't have time to fix it this morning. if you would like to re-apply your changes to this patch, that would be appreciated. You can test locally via:
./vendor/bin/phpunit -c core --group Scaffold
If not, I'll see if I can circle back and integrate #185 later.
This patch addresses the rest of the comments from #166 (but not #185):
- 31. Replaced all regex asserts with simple 'assertContains'
- 32. None of the tests assert on stderr, so I changed ExecTrait to return only stdout. I did not want to return the process object as I want to shield the tests from which process runner the trait is using.
- 33. vfsstream is more flexible than I had imagined it to be; I think it could work well for most of the tests here. However, the Composer hook tests could not use vfsstream, I think, because these tests `exec` the Composer binary. Perhaps some future investigation could be done into using vfsstream. It might be possible to rewrite the hook tests without exec.
- 34. Noted why 'true' must be a string rather than a boolean.
- 35. Code such as `$is_link = false` was placeholders for eventual dataproviders, if needed. Replaced these where they appeared.
- 36. Added assertFileNotExists after unlink
- 37. Wrapped to < 80
- 38. Removed thin wrapper 'execComposer'. This was done initially to maintain alignment between the 'exec' tests and the 'runComposer' tests (that call the Composer API) so that test snippets could be copied between them more easily. This isn't such a concern any more, and in any event, adding or removing the word 'composer' is not hard.
- 39. Wasn't sure if a larger refactor was warranted here (similar to reoslution of #166.20), but settled for making a simple value wrapper.
- 40. Fixed > 80
- 41. Same as 31.
- 42. Made scaffold file asserts more specific
Comment #188
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedRe-applying improvements from #185. Thanks, @yogeshmpawar.
Comment #189
MixologicTrivial extra whitespace at the end of these lines caused patch application to moan.
Comment #190
borisson_I didn't look at the tests in detail, but I went trough the main part of the patch and I found some nitpicks. Overall this has really good documentation and looks really good.
the
if ($package)
here doesn't add anthing since the return value can either be null or something of PackageInterface.I also personally prefer to have positive assertions where possible.
So I think something like this is better:
if ($page instaceof PackageInterface && isset($allowed_packages[$name])
Before we get this in, we should only have @todos with a link to a drupal.org issue. So we should create one for this and link to it from here.
A normal todo without a link to d.o will either lead to some duplicate issues or the issue being forgotten.
Should we wrap this under 80 cols? Or will that lead to a blank line in the output as well.
This is the first time we introduce a private constructor in core. I like this - but it already a really big patch, should we use that to also introduce a new coding style as well?
I like this a lot - so we can probably keep this.
I think there needs to be an empty line before an @return
Should be FALSE in caps.
Again, should this be an @todo with a link to d.o?
Needs an empty line.
Can this be ReplaceOp::ID?
Currently it is not allowed according to our documentation standards to have both {@inheritdoc} AND something else in the same docblock. There is an issue to change that: https://www.drupal.org/project/coding_standards/issues/1994890
In this case, we either copy the entire doc or remove the short description from here.
the title should be only one line, with additional documentation seperated by a blank line.
^
Needs docs.
Needs docs.
Needs a blank line after class declaration.
This documentation is awesome! Thank you so much.
Comment #191
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThanks for the feedback, @borisson_. I addressed your comments as follows:
- 1. Simplified 'if' a bit; however, `isset` and `array_key_exists` work about the same way, so the negation is still needed in the expression in order to maintain its current meaning.
- 2. Added reference to an issue: https://www.drupal.org/project/drupal/issues/3064990. This issue could use some reworking to follow drupal.org conventions.
- 3. The URL on its own is >80 col. Unless we care to make a shorter URL, we cannot get this block down to < 80 columns on every line, so I left it as-is.
- 4. The private constructor is appropriate here, but this is an internal class, so the impact of removing it would be minimal. Still, I will leave this in unless someone decides it must be removed.
- 5. Added blank line.
- 6. Corrected FALSE
- 7. There is no requirement for large scaffold files, so it seems unnecessary to create an issue. Instead, I removed the suggestion.
- 8. Added blank line.
- 9. Used constant.
- 10. The additional comment didn't add anything over the comment at the top of the file, so I just removed it.
- 11 & 12. Corrected formatting of comment blocks.
- 13 & 14. Added docs.
- 15. Added blank line.
Comment #192
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedForgot to set to 'needs review' for the test bot.
Comment #193
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedin #191 I accidentally saved the interdiff as the .patch file. Here is a corrected patch. For the interdiff, see 189-to-191-interdiff.txt
Comment #194
borisson_I don't see anything else that needs to be changed or even could be changed to make this more ready for inclusion into core.
Comment #196
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWell, now I'm embarrassed. I made a minor change to the wording of one of the error messages, and did not update the test to match. Here's a new patch to fix it (along with more appealing shade of blue on the message text).
Comment #197
Mile23PostgreSQL and SQLite tests will probably fail due to #3059332-33: Mark kernel tests that perform no assertions as risky
I'm going to RTBC here because it LGTM. :-) But then I'm going to start up some testbot runs for PHP 7.0 and 7.2.
The next step is #3057094: Add Composer vendor/ hardening plugin to core, so we can move on to #2982680: Add composer-ready project templates to Drupal core
Comment #198
Mile23It looks like the PHP 7.0 build failed with this:
Trying again.
Comment #199
Mixologic7.0.x-dev is probably not the environment you wanted to run. (didnt realize that was even available)
Comment #200
Mile23Restarting the sqlite test after #3059332-42: Mark kernel tests that perform no assertions as risky
Also I just now learned that apparently we won't be supporting PHP 7.0 in core 8.8.x, so therefore the failed PHP 7.0 builds in #196 aren't an issue.
Comment #201
Mile23OK, so PHP 7@stable is supported for 8.8.x, so I started that test on #196.
Comment #202
borisson_All envs are green, so that's great! There are 3 new coding standards issues introduced in the latest patch (they are mentioned on https://www.drupal.org/pift-ci-job/1335751), we should fix those as well. Not pushing this back to rtbc for just that though.
Comment #203
Mile23Fixes CS. Docblock changes only.
Comment #204
Mile23Setting back to RTBC from #197.
Comment #205
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI reviewed the interdiff and patch files one more time, and also believe this is rtbc now.
Comment #206
MixologicSo very reviewed, much tested. +1
Comment #207
alexpottSome thoughts that have been bothering me for a bit.
This is not really a collection. It's a processor. Or a collection processor.
This could be a collection and we could extend from Iterator and be something we could loop around and maybe hide away some of these arrays that we're passing around.
Sorry for setting this back to needs review but maybe someone who is more adept at iterators can think of an even better way of doing this but I think the attached patch fixes a couple of subtle bugs and results in more reliable code as now \Drupal\Component\Scaffold\Operations\ScaffoldFileCollection is a static iterator generator with no state to keep in sync and the whole override thing has been replaced by the existing SkipOp.
Comment #208
Ghost of Drupal PastIf someone slacks me with info about what iterator problems we have here I might be able to help, reading such a huge enough patch to comprehend is a bit beyond my free time right now. A tiny note, not worth keeping up the patch for but if it's rerolled:
return isset($this->data[self::OVERWRITE]) ? $this->data[self::OVERWRITE] : TRUE;
no need for that any more$this->data[self::OVERWRITE] ?? TRUE
.Comment #209
MixologicThe comment in #166 point 10 spawned a discussion in slack whether or not using the NULL Coalesce operator was okay or not, as its among the "php7 only features". And while this patch is adding entirely new code, we have to be careful about php7isms, particularly if our phpcs/dev process doesnt *yet* support them or know what to do with them.
What we found out is that core already includes one instance of a Null Coalesce operator (https://git.drupalcode.org/project/drupal/blob/8.8.x/core/lib/Drupal/Cor...), and phpcs didnt raise any flags on that patch, ergo Null Coalesce is fair game, that being said, however, there was initially an abundance of caution to not make the patch keep flip flopping due to the grey area that is "Drupal 8.8's allowable php7isms", so its not surprising that there are instances of ternary isset's checking for nulls.
I don't think we have any 'iterator problems' per se.
@alexpott: I gave your changes a pretty thorough examination, and yay, less code doing what Im somewhat mostly sure is the same thing, sans state management. My only concern is that the test we have that covers this change, and the resultant updated test both feel somewhat artificial to me, and Im not entirely confident that the tests prove that the code functions according to the intended use case. But I feel like the only better way to test what we're doing there is down the path of #3031379: Add a new test type to do real update testing
I'm going to re-set this to RTBC. For new code like this, I'd rather not let perfect to be the enemy of the good.
Comment #210
alexpottHere's some slight improvements with a bit less complexity. And our ScaffoldFileCollection is now the iterable it's name hints it should be. yay.
Leaving at rtbc because of #209 - the changes are small.
Comment #211
MixologicI've updated the pseudo subtree split here: https://github.com/drupal/drupal-scaffold
Which handily gives us a nice interdiff of everything we've fixed since #189: https://github.com/drupal/drupal-scaffold/commit/74ffdf4ad5c4312993107ce...
Comment #212
Mile23"I'd rather not let perfect to be the enemy of the good."
+1
We'll be iterating the heck out of this and #3057094: Add Composer vendor/ hardening plugin to core to get to #2982680: Add composer-ready project templates to Drupal core.
Comment #213
jibranShould we mention these folks in commit message?
Comment #214
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI don't want the perfect to be the enemy of the good either, but I think that the
unset
would be confusing here, as it would make the appended scaffold file disappear from the project output for its original package.Instead, I put the SkipOp in place invariantly, regardless of whether the original location is being overridden by a completely new operation or a conjoinable operation.
Comment #215
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAlso, +1 on #213
Comment #224
webchickAdded everyone from https://github.com/drupal-composer/drupal-scaffold/graphs/contributors to the issue credit, except for ojacquet who I couldn't find.
Comment #225
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedActually, #214 is more a case of the imperfect being the enemy of the good, as I forgot to update the tests.
Here is a corrected patch with interdiff.
Comment #227
webchickSolved the @ojacquet mystery! (Thanks, @borisson_!)
Comment #228
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedRTBC-ing my own thing because yolo and the change is very small. Anyone who disagrees can "Needs Work" me.
Thanks for adding collaborators to the issue credit, @webchick.
Comment #229
larowlanAdding review credits
Comment #231
larowlan🚀 we have lift off
published change record
Committed 3bb50b2 and pushed to 8.8.x. Thanks!
Comment #232
Mile23Wow thanks everyone. :-)
Up next: #3057094: Add Composer vendor/ hardening plugin to core
Comment #233
vijaycs85🎉🎉🎉
Comment #234
kim.pepper👏👏👏
Comment #235
fazni CreditAttribution: fazni commentedThanks +1
Comment #236
ultimikeWoot!!!
Comment #237
mondrakeIs it possible that this broke testing in HEAD for PHP 7.3.dev? I see all commit tests https://www.drupal.org/pift-ci-job/1345923 failing since then.
EDIT - #2978261: \Drupal\Tests\system\Functional\Cache\AssertPageCacheContextsAndTagsTrait::assertCacheContexts() is unhelpful after conversion to phpunit seems related.
Comment #238
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI don't think that the failure is related to this patch. All of the failures are reported on the same line in an unrelated file. I looked at the source and was uncertain why the error,
Undefined variable: match
, is happening, since$match
is invariantly assigned a few lines up.Comment #239
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWhatever was wrong with the 7.3-dev build has been cleared up; tests are passing again. Not related to this patch.
Comment #240
jhodgdonFYI -- I had a random test fail in ManageGitIgnoreTest yesterday. Hit retest and it passed (on the same patch). I created an issue for that:
#3067768: Random test fail in ManageGitIgnoreTest
This was not PHP 7.3, it was PHP 7.1 & MySQL 5.7. There may be a problem...
Comment #241
FeyP CreditAttribution: FeyP as a volunteer and at werk21 commentedUnfortunately I just experienced what I think is another random test fail related to this issue. I filed #3070853: Race condition during composer cache clear in functional composer scaffolding plugin tests for that. I'd appreciate it, if any of you could look into it.
Comment #242
MixologicComment #244
Krzysztof DomańskiI added a follow-up #3086118: Correct the example in composer/Plugin/Scaffold/README.md.
Comment #245
xjmThis probably merits a mention in the release highlgihts?
Comment #246
Mixologic+1 to #245