Problem/Motivation
In order to be able to do #2982680: Add composer-ready project templates to Drupal core, it is first necessary to provide a list of scaffold files in the drupal/core project. This will allow the Scaffold component from #2982684: Add a composer scaffolding plugin to core to move all of the "scaffold" files (index.php et. al.) thus provided to their original location so that a project based on the subtree split of drupal/core may still assemble a complete and functional Drupal site.
Proposed resolution
This patch creates duplicate copies of every scaffold file. Each file remains in its original location, and will be used there by drupal/drupal (Drupal core development) and also by sites based off of the drupal-composer/drupal-project template
. Each file will additionally have a copy in a new location, core/assets/scaffold
. The original locations for each of the scaffold files will be considered deprecated by this issue, and will be removed in Drupal 9. To ensure that no mistake is made during the remainder of the Drupal 8.8.x cycle, unit tests will be added to compare the contents of each copy of the scaffold files, and throw an error if someone makes a change in just one location.
Remaining tasks
None.
Follow-on Tasks
#3075954: Remove duplicate scaffold files
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
The Drupal "scaffold" files (index.php and so on) now exist in two locations: core/assets/scaffold/files, for use by the Drupal Composer Scaffold plugin, and their original location, used in Drupal Core development, and with downloaded copies of Drupal. Any change made to one of these files will need to be made to both locations.
Comment | File | Size | Author |
---|---|---|---|
#59 | 3067645-59.patch | 92.9 KB | greg.1.anderson |
#59 | 3067645-55-to-59-interdiff.txt | 1.13 KB | greg.1.anderson |
#55 | interdiff_52.txt | 4.5 KB | Mixologic |
#55 | 3067645-55.patch | 92.76 KB | Mixologic |
#52 | 3067645-52.patch | 92.97 KB | greg.1.anderson |
Comments
Comment #2
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's the patch. Let's see if the test bot can chew on it.
With this patch, the only files/directories needed at the root of the Drupal repository are:
- composer.json
- composer.lock
- .gitignore
- core
Note also that the scaffold files were all placed in a single flat directory. This was done because there are not so many of them, and it makes it easier to view all at once. Similarly, the "dot" files were renamed to not begin with a dot in their source location to make them easier to view. It would be possible to make the dot files still have the same name, or mirror the directory structure of the scaffold files (e.g.
modules/README.txt
instead ofmodules.README.txt
, and so on) if desired.Comment #3
andypostThe only downside in plain assets is hard to diff with original version
Comment #5
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@andypost What do you mean? Are you referring to this patch, or future patches?
- In this patch, the originals only moved; none of them changed.
- In future patches, the diffs should be made against the assets in their source locations (lib/Drupal/Component/ScaffoldAssests/assets), which should be clean.
Did you have a completely different concern?
Comment #6
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHm I am not sure that my patch file in #2 is working correctly. When I try to apply it, it ignores all of the git mv operations, and I end up without any assets directory in the ScaffoldAssets component.
Comment #7
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#2 can be applied via:
The test runner must already be using
git-apply
here; otherwise, the test results would not have come out as they did.Comment #8
Ghost of Drupal PastI can't pretend to understand half of what's going on here but the words "tarball" and "symlink" caught my eye: I'd recommend against putting symlinks in the Drupal tarball because of Windows. For eg https://superuser.com/a/125981/41259
Comment #9
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@ChX: Yes, symlinks in the tarball is an undesirable side effect of this patch. Our ultimate goal is to remove the wikimedia merge plugin. The intention is in the end to have the tarballs generated by the drupal.org infrastructure come out exactly the same as they currently do. Fixing up the symlink setting might need to be follow-on work, or perhaps the infra code could change in anticipation of this patch before it is merged.
Alternately, we could change this patch to use symlink: false (good for tarballs, bad for core development), then change infra to always set symlink: false, then have another patch to put the symlink default back to true (good for core development).
Comment #10
Mile23I was able to do an entire install like this:
That gave me logging like this:
I was then able to visit my local server and install Drupal.
It all looks like this:
Comment #11
Mile23Comment #12
MixologicWhat do we get out of making the assets a separate sub project, vs just putting the files into
core/assets
, and putting the extra section with the composer scaffold data intocore/composer.json
? Do any of those files have value *outside* of using them alongsidedrupal/core
?I get concerned when we have subcomponents that are version pinned to each other, i.e. 8.8.1 of core will always require 8.8.1
drupal/core-scaffold-assets
, thus potentially requiring a `composer update drupal/core drupal/core-scaffold-assets` in some future scenario.Comment #13
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThe scaffold component can't scaffold from drupal/core if we load it from the wikimedia merge plugin. There are two solutions to that:
- Don't use the merge plugin for drupal/core.
- Add a feature to the scaffold plugin to "include" the composer.json file from drupal/core (sort of like a mini composer-merge plugin just for scaffolding). This feature would only be used in the git clone of the Drupal repository.
I think it's okay for drupal/core to require a separate assets project via "self.version", though. The problem in #12 typically happen when two projects that do **not** depend on each other (are related only by 'conflict' statements, not by 'require' statements) must be pinned to the same version. When drupal/core requires cores-scaffold-assets, then `composer udpate drupal/core --with-dependencies` is sufficient to keep them in sync.
Comment #14
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere is another patch that is pretty much the same as #2 except that drupal/core-scaffold-assets have been moved from core/lib/Drupal/Components to core/assets.
Note that `self.version` works fine for drupal/core-composer-scaffold, but it does not work for drupal/core-scaffold-assets. I believe this is because the former exists in the 8.8.x-dev release, whereas the later does not. Before merging this, we might need to add an unused drupal-core-scaffold-assets project to the 8.8.x branch so that we can then come back and revisit this patch, and use `self.version` for drupal/core-scaffold-assets as well (presuming that we haven't obviated the need for this by solving one of the blocking issues in #13 instead).
Comment #15
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedLet's test that patch.
Comment #16
MixologicWhat's going on here? are these bugs with the scaffold plugin?
Comment #17
MixologicI think my main concern with making core's assets a separate component that gets split out, is ideally, core would demonstrate a clean way to use the scaffold plugin as an example for others to follow. Perhaps that doesn't doesnt actually matter. Another issue though, is that once a namespace is defined in composerland, theres not a great way to go back to not using, it, so, if, someday we get rid of this wikimedia merge thing and end up with a saner situation, there will always be other templates/projects/installation profiles/distributions that end up relying on it.
I did end up playing around with the scaffolding directives to see what happens when theres not an additional component, and I see now that essentially, drupal/core isnt 'installed' at all because of the 'replace' that is in the root composer.json, and as such, the scaffold plugin doesn't ever get any of its instructions stored in its composer.json.
Given all that, Im +1 on the path forward being a separate component.
When I was testing this, the only other difference I saw was that on the current 8.8.x branch, a composer install will only have a vendor folder that is not in git, but with this patch, modules, profiles, sites, and themes all appear to be untracked directories to git, which I think isnt something we can reliably work around since empty directories arent something that is *stored* in git.
Comment #18
Mixologicone more thing.. why is this 2?
Comment #19
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThe "2" was a mistake; I was seeing some strange behavior, and wanted to try a new package name to make sure the cache wasn't involved. I forgot to put it back in #2, and I fixed it in #14 in composer.json; maybe I accidentally reset composer.lock before making the patch. I don't remember what the issue was; some composer-ism. I'll make sure the lock gets updated in the next patch.
Yes, this patch fixes a couple of bugs in the scaffold plugin. I'll document them.
I'm not sure if we can do #2943842: Allow component namespaces to be autoloaded independently and also add drupal/core as a path repository. It seems that in theory that should work, but I don't remember what issues I had last time. I can try one more time.
Comment #20
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's a work-in-progress patch. This takes work from #2943842: Allow component namespaces to be autoloaded independently and removes the wikimedia merge plugin entirely. With this patch, the scaffold command runs and scaffolds assets from
drupal/core
just fine. The resulting site is not a working Drupal site because I haven't finished moving some of the "replace" items to "require". Also, phpunit does not work, and I do not know why. It is require-dev'ed from drupal/core, which is included as a path repository from the root composer.json file, but for some reason it is not included in composer.lock / vendor.I know this also messes up testing of the components, so maybe we're not ready to go this way. If that's the case, we can always return to #14 rather than continuing here.
Comment #21
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis patch is getting much closer.
First, as for phpunit/phpunit, that was an obvious one. The `require-dev` appeared in drupal/core, which is considered part of the root-level composer when using the wikimedia merge plugin, but is not when including drupal/core via a path repository. I moved the `require-dev` components to the project-level composer.json, and phpunit is now working.
Another thing worth mentioning is that I have temporarily changed the type of drupal/core from `drupal-core` to `project`. Even when I removed the composer installers instruction to relocate drupal/core to the `core` directory, that directive still exists by default in the composer/installers project itself. We'll either have to get that removed, or just change the type of drupal/core to something not yet used for 8.8.0, as it definitely does not work to relocate a path repository at the directory /core to itself at /core.
Finally, if I `require` the drupal/core-version component in drupal/core, then everything goes to hell and Composer tries to load drupal/core from Packagist instead of from the local path repository. At the moment, I have no idea what it is about drupal/core-version that causes this behavior, so I've left it out. This makes the Drupal site built by this patch incomplete and probably non-functional (although I haven't tried). That's the only missing component, though, so once we solve that problem we should be getting somewhere.
Comment #22
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI tried renaming drupal/core-version to drupal/core-version-util just to see if there was some odd cache information involved here. Even with a new name, though, including this component caused Composer to become confused and include drupal/core from Packagist. Still mystified as to what's up with this one component.
Comment #23
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedRemoved this temporarily, because it wasn't working earlier. Now this works again when put back in the composer.json. I'll put this back in a future patch. We may want to replace this script with scaffold instructions instead, or maybe make it part of the vendor cleanup plugin.
Comment #24
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI had a theory that maybe there was some odd limit on the number of path repositories (or on the number of repositories) that Composer allowed in a project, so I tried removing a few. It did not help, though; requiring drupal/core-vendor still caused the same odd behavior with one less repository.
Comment #25
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI re-tried #24 again, and this time I was able to successfully include drupal/core-version with fewer path repositories defined in the root composer.json file. It appears that there is some limit to the number of repositories / path repositories that a project can use. This bears further investigation.
Comment #26
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's a fix. If this patch were a Pokemon, we might say that it is not yet in its final form. This is definitely a powerful evolution, though.
- Wikimedia merge plugin has been removed.
- Scaffold plugin is being used to scaffold files.
- Drupal installs and runs.
- Scaffold phpunit tests run and pass.
Setting to 'needs review' to check to see if the rest of the tests pass as well.
To achieve this magic without running up against the Composer limitation on path repositories, I
replace
d the component projects in the root-level composer.json, and added a psr-4 autoloader for them, also in the root-level composer.json. This means that during core development, the component projects from lib/Drupal/Components will be used in-place. One limitation of this is that the requirements from the various Drupal components must be copied to the root-level composer.json when doing Drupal core development. This seems to be Symfony's strategy for their monorepo; they only have one 'path' repository.When using drupal/core from some other project, though, the components will be brought in via the `require` statements in the drupal/core composer.json file. This means that a project similar to drupal-composer/drupal-project would end up using the copy of these components in the `vendor/drupal/*` directory. The source copies of these components in `vendor/drupal/core/lib/Drupal/Components/*` would still be present, but would not be used.
Can we move the `replace` statements from the root level composer.json into the drupal/core project, and also do the same for the autoload directive and the component's dependencies? Maybe. This tactic would NOT work for the Scaffold component, as it must be an actual independent project with a type `composer-plugin`. We might get around this if we made drupal/core itself a composer plugin, but that seems inadvisable. While the composer plugins must not be `replace`d in drupal/core, perhaps the other components could be.
Note also that I still have the type of drupal/core set to `project` here, per #21. This must be fixed (e.g. moved back to drupal/core) so that it may be relocated to the `core` directory when used by drupal-composer/drupal-project and similar. We cannot allow this to happen when doing Drupal core development, though, because drupal/core cannot be copied over itself.
Comment #27
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedForgot to upload the patch. :P
Comment #28
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's an update that is the same as #26, except without the dependency updates to composer.lock.
Comment #29
MixologicThe 'build successful' thing there is due to nightwatch.js failing. The other tests did pass, so, awesome.
Im kindof at a loss as to how anything purely javascript related would have been affected by this patch. I wonder if its something transitory.
Comment #30
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWe've got the following differences:
That's moving from this patch back to HEAD of 8.8.x. I'm not sure if behat/mink or behat/mink-selenium2-driver could affect the test as observed. I would suspect not, although I don't know.
We could wait for some unrelated tests to run, and see if they fail the same way.
Comment #31
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedMoving as much as possible back to core/composer.json. The scaffold component is still included as a path repository and NOT replaced; all the other components are replaced. Also moved the psr-4 autoloader to drupal/core.
With this change, sites based on drupal-composer/drupal-project layout styles will require drupal/core, and will use most of the Drupal components embedded inside drupal/core, save for the scaffold component. The scaffold component will be downloaded separately from the subtree split on Packagist; an unused copy will also exist in drupal/core.
Next step will be to remove the scaffolding directives and move this PR back to #2912387: Stop using wikimedia/composer-merge-plugin. Note that we still have the issue of the type of drupal/core that must be resolved before committing. I think #2943842: Allow component namespaces to be autoloaded independently will be unnecessary after this.
This issue will then be postponed on 2912387.
Comment #32
mxr576Well sometimes it could... this is the reason why I opened https://www.drupal.org/project/drupal/issues/2956279 because we bumped to this long time ago: https://github.com/minkphp/Mink/pull/760
Comment #33
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI determined that bumping the dev dependencies was not a factor in this instance. I am very much in favor of #2956279: [Policy] Do not use dev-master as a dependency for composer packages though.
This patch is currently postponed on #2912387: Stop using wikimedia/composer-merge-plugin
Comment #34
MixologicIm retitling to make this more generic as the solution has evolved over time.
Comment #35
MixologicComment #36
MixologicAdding a related, probably will be fixed issue once this goes in.
Comment #37
MixologicAre we able to scaffold files into the vendor directory with this?
Specifically Im looking for a way that we can scaffold these two files in place and eliminate the need for this composer script: https://git.drupalcode.org/project/drupal/blob/8.8.x/core/lib/Drupal/Cor...
Comment #38
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWe should probably make scaffolding into vendor a separate issue. It would be possible, but we should provide an automatic path replacement [vendor-dir] to facilitate this.
For the specific use-case mentioned in #37, though, I would favor making this the responsibility of the vendor cleanup plugin, since it's sort of odd to have to touch vendor, and this is only necessary in situations where the vendor cleanup plugin is also being used.
Comment #39
MixologicAh, yes. I concur. less of a 'vendor cleanup' and more of a 'vendor security tightening' plugin
Comment #40
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis is unblocked now that #2912387: Stop using wikimedia/composer-merge-plugin has been committed. I'll re-roll this tomorrow.
Comment #41
Mile23#3057094: Add Composer vendor/ hardening plugin to core does now handle placing .htaccess and web.config files in the vendor directory.
Comment #42
Mile23Here's a patch that coexists happily with the merge plugin.
It moves our Composer packages to a folder in the root called composer/.
It adds a path repo to a package called drupal/core-scaffold-assets.
There are some problems with the scaffolding plugin because it has trouble moving default files to within sites. There are extra special permissions things going on in there that are a special case. We'll have to address those. This patch leaves those files in place for now, without scaffolding them.
There are a few failing tests, notably
ComposerHookTest
and our old friendManageGitIgnoreTest
.Comment #43
Mile23autoload.php might be helpful, and I'm not sure how those changes to workspace module got in.
Comment #44
Mile23Ahh, the extra is stored in composer.lock, so you have to do the branch dance to update that, too.
Branch dance consisting of:
See #42 for some explanations.
Comment #45
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedNeat.
I like this relocation. We should move the Scaffold plugin to its final location in #2912387: Stop using wikimedia/composer-merge-plugin, though.
I believe we previously decided that the scaffold files should just go in drupal/core, not a separate package. If we have a separate package, then, for users of template projects, we have to worry about drupal/core requiring the exact right version of drupal/core-scaffold-assets (like the drupal/core + drupal-dev-dependencies / drupal-core-strict relationships today). This causes the `composer update` dx to take a big hit. This isn't a problem if we just put the assets directly in drupal/core.
I also had this problem when I was relocating the Scaffold plugin. The cause in my case was namespace directives that hadn't been updated to reflect the move yet.
Comment #47
Mile23OK, as per chat with @greg.1.anderson:
The focus here is copying the scaffolding files to a safe and happy place within core, so that core can specify where they belong in drupal/core's composer.json file.
We don't need to have drupal/drupal use the scaffold plugin to perform any moves.
We also leave the scaffolded files in place, so that we have BC with drupal-composer/drupal-project, and also so that we don't ruin the day for core devs.
This allows us to move forward with the tarball legacy package while still having BC with everyone's core dev workflows.
Comment #48
Mile23Comment #49
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpdated issue summary to reflect current strategy to increase focus on backwards compatibility.
Comment #50
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpdate list of asset files in release notes summary.
Comment #51
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpdate list of scaffold files.
Comment #52
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere is a new patch that duplicates rather than moves the scaffold files. The Composer Scaffold plugin will not be used during drupal/drupal development -- at least not until Drupal 9, at which time the destination copy of the scaffold files will be removed from the repository.
Comment #53
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI tested this with greg-1-anderson/drupal-drupal-composer, and from that found #3075621: Scaffold plugin sometimes fails to write autoload.php. Other than that issue, it worked well, producing an installable Drupal site with the same file layout as a tarball download. The vendor directories are not in alignment because I removed drupal-core-strict from my test to simplify things. It would probably work to put it back in, but vendor alignment is for a follow-on story. I think that things are working well enough to move forward here. We just need the CR and docs updates.
Comment #54
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAdded a change record. Since this patch does not change behavior yet, I think that the change record is the only documentation we need at this time.
Comment #55
MixologicThere were a couple of coding standards messages, and a couple of extraneous reformats that I fixed.
I didn't really change the contents of the patch, so Im going to go ahead and mark this RTBC.
Comment #56
MixologicIssue summary lgtm.
Comment #57
larowlanCan we get a follow-up issue for this opened against the 9.x branch, and reference it here.
Comment #58
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComing right up.
Comment #59
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAdded follow-on issue #3075954: Remove duplicate scaffold files and included links to it in tests and scaffold files README.txt.
Comment #60
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFollow-up issue created and cross-linked, and tests came back green; back to RTBC.
Comment #61
MixologicSolid. +1
Comment #62
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUnassigned.
Comment #63
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWhat if we do not want to maintain two copies of the scaffold file?
If we immediately remove the "destination" (original) location of the scaffold files, then two things will break:
It seems like we should provide backwards compatibility for drupal-composer/drupal-scaffold sites until Drupal 9. The Drupal tarball generation is an internal process on drupal.org, though, and we are in control of it. If we want to go to single-copy-scaffold files sooner than Drupal 9, my recommendation for maintaining reliable availability of Drupal download files would be to do as follows:
In short, I am recommending that we continue here as planned, and handle [edit: removal of] backwards compatibility support as follow-on work. It would be a good idea, though, to decide when we are going to do #3075954 (during 8.8.x or not until 9) before committing here.
Comment #64
catchThe files don't change very often, so copying them like this with the phpunit test seems like a very simple approach that moves us forward with only a small amount of extra mess.
I think it's fine if it all stays in until Drupal 9, if there's another way to provide bc before then that also seems OK.
Comment #65
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI don't believe there is a better way to provide bc than to double-commit the scaffold files for a time. We could also help by submitting a transition path for the existing community template projects. This is already planned.
Comment #66
larowlanCommitted 577ba8b and pushed to 8.8.x. Thanks!
Comment #69
hchonovI am sorry for writing here, but I think it might be the best place to ask this question.
We commit those files so that we track changes on them and adapt our project whenever there are changes.
However the same is true for the root level composer.json. Would it be a valid issue to include it as well?