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.

CommentFileSizeAuthor
#59 3067645-59.patch92.9 KBgreg.1.anderson
#59 3067645-55-to-59-interdiff.txt1.13 KBgreg.1.anderson
#55 interdiff_52.txt4.5 KBMixologic
#55 3067645-55.patch92.76 KBMixologic
#52 3067645-52.patch92.97 KBgreg.1.anderson
#44 interdiff.txt7.17 KBMile23
#44 3067645_44.patch137.61 KBMile23
#43 interdiff.txt7.19 KBMile23
#43 3067645_43.patch130.45 KBMile23
#42 interdiff.txt164.89 KBMile23
#42 3067645_42.patch136.87 KBMile23
#31 3067645-28-to-31-interdiff.txt4.42 KBgreg.1.anderson
#31 3067645-31.patch43.68 KBgreg.1.anderson
#31 3067645-31.patch72.44 KBgreg.1.anderson
#28 3067645-28.patch39.77 KBgreg.1.anderson
#27 3067645-26.patch133.79 KBgreg.1.anderson
#21 3067645-21.patch165.1 KBgreg.1.anderson
#21 3067645-20-to-21-interdiff.txt5.64 KBgreg.1.anderson
#20 3067645-20.patch220.82 KBgreg.1.anderson
#14 3067645-14.patch16.57 KBgreg.1.anderson
#14 2-to-14-interdiff.txt1.66 KBgreg.1.anderson
#2 3067645_2.patch17.42 KBgreg.1.anderson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greg.1.anderson created an issue. See original summary.

greg.1.anderson’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
17.42 KB

Here'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 of modules.README.txt, and so on) if desired.

andypost’s picture

The only downside in plain assets is hard to diff with original version

Status: Needs review » Needs work

The last submitted patch, 2: 3067645_2.patch, failed testing. View results

greg.1.anderson’s picture

@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?

greg.1.anderson’s picture

Hm 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.

greg.1.anderson’s picture

#2 can be applied via:

git apply < 3067645_2.patch

The test runner must already be using git-apply here; otherwise, the test results would not have come out as they did.

Ghost of Drupal Past’s picture

I 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

greg.1.anderson’s picture

@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).

Mile23’s picture

I was able to do an entire install like this:

# Remove settings files, existing .gitignore, etc. Use sudo so you can modify restricted files.
$ sudo git clean -fdx
$ sudo git apply 3067645_2.patch
$ sudo rm -rf vendor/
$ sudo chown -R paul .
$ composer install

That gave me logging like this:

Scaffolding files for drupal/core-scaffold-assets:
  - Link [web-root]/.csslintrc from assets/csslintrc
  - Link [web-root]/.editorconfig from assets/editorconfig
  - Link [web-root]/.eslintignore from assets/eslintignore

I was then able to visit my local server and install Drupal.

It all looks like this:

$ ls -al
total 440
drwxrwxr-x  27 paul  staff     864 Jul 14 17:25 .
drwxr-xr-x  29 paul  staff     928 Jul 13 11:47 ..
lrwxr-xr-x   1 paul  staff      51 Jul 14 17:25 .csslintrc -> vendor/drupal/core-scaffold-assets/assets/csslintrc
lrwxr-xr-x   1 paul  staff      54 Jul 14 17:25 .editorconfig -> vendor/drupal/core-scaffold-assets/assets/editorconfig
lrwxr-xr-x   1 paul  staff      54 Jul 14 17:25 .eslintignore -> vendor/drupal/core-scaffold-assets/assets/eslintignore
lrwxr-xr-x   1 paul  staff      55 Jul 14 17:25 .eslintrc.json -> vendor/drupal/core-scaffold-assets/assets/eslintrc.json
drwxrwxr-x  16 paul  staff     512 Jul 14 17:25 .git
lrwxr-xr-x   1 paul  staff      55 Jul 14 17:25 .gitattributes -> vendor/drupal/core-scaffold-assets/assets/gitattributes
-rw-r--r--   1 paul  staff     190 Jul 14 17:24 .gitignore
lrwxr-xr-x   1 paul  staff      55 Jul 14 17:25 .ht.router.php -> vendor/drupal/core-scaffold-assets/assets/ht.router.php
lrwxr-xr-x   1 paul  staff      50 Jul 14 17:25 .htaccess -> vendor/drupal/core-scaffold-assets/assets/htaccess
lrwxr-xr-x   1 paul  staff      53 Jul 14 17:25 INSTALL.txt -> vendor/drupal/core-scaffold-assets/assets/INSTALL.txt
lrwxr-xr-x   1 paul  staff      52 Jul 14 17:25 README.txt -> vendor/drupal/core-scaffold-assets/assets/README.txt
-rw-r--r--   1 paul  staff     315 Jul 14 17:25 autoload.php
-rw-r--r--   1 paul  staff    3181 Jul 14 17:24 composer.json
-rw-r--r--   1 paul  staff  208922 Jul 14 17:24 composer.lock
drwxr-xr-x  44 paul  staff    1408 Jul 14 17:24 core
lrwxr-xr-x   1 paul  staff      59 Jul 14 17:25 example.gitignore -> vendor/drupal/core-scaffold-assets/assets/example.gitignore
lrwxr-xr-x   1 paul  staff      51 Jul 14 17:25 index.php -> vendor/drupal/core-scaffold-assets/assets/index.php
drwxr-xr-x   3 paul  staff      96 Jul 14 17:25 modules
drwxr-xr-x   3 paul  staff      96 Jul 14 17:25 profiles
lrwxr-xr-x   1 paul  staff      52 Jul 14 17:25 robots.txt -> vendor/drupal/core-scaffold-assets/assets/robots.txt
drwxr-xr-x   8 paul  staff     256 Jul 14 17:25 sites
drwxr-xr-x   3 paul  staff      96 Jul 14 17:25 themes
lrwxr-xr-x   1 paul  staff      52 Jul 14 17:25 update.php -> vendor/drupal/core-scaffold-assets/assets/update.php
drwxr-xr-x  41 paul  staff    1312 Jul 14 17:25 vendor
lrwxr-xr-x   1 paul  staff      52 Jul 14 17:25 web.config -> vendor/drupal/core-scaffold-assets/assets/web.config
Mile23’s picture

Mixologic’s picture

What 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 into core/composer.json? Do any of those files have value *outside* of using them alongside drupal/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.

greg.1.anderson’s picture

The 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.

greg.1.anderson’s picture

Here 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).

greg.1.anderson’s picture

Status: Needs work » Needs review

Let's test that patch.

Mixologic’s picture

+++ b/core/lib/Drupal/Component/Scaffold/GenerateAutoloadReferenceFile.php
@@ -62,7 +62,7 @@ public static function generateAutoload(IOInterface $io, $package_name, $web_roo
-    $location = dirname($autoload_path->fullPath());
+    $location = $autoload_path->fullPath();

+++ b/core/lib/Drupal/Component/Scaffold/Operations/ReplaceOp.php
@@ -105,6 +105,7 @@ protected function copyScaffold(ScaffoldFilePath $destination, IOInterface $io)
+    $this->source->addInterpolationData($interpolator);

What's going on here? are these bugs with the scaffold plugin?

Mixologic’s picture

I 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.

Mixologic’s picture

+++ b/composer.lock
@@ -3657,6 +3714,45 @@
+            "name": "drupal/core-composer-scaffold2",

one more thing.. why is this 2?

greg.1.anderson’s picture

The "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.

greg.1.anderson’s picture

Status: Needs review » Needs work
FileSize
220.82 KB

Here'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.

greg.1.anderson’s picture

This 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.

greg.1.anderson’s picture

I 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.

greg.1.anderson’s picture

+++ b/composer.json
@@ -49,7 +59,6 @@
-        "post-autoload-dump": "Drupal\\Core\\Composer\\Composer::ensureHtaccess",

Removed 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.

greg.1.anderson’s picture

I 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.

greg.1.anderson’s picture

I 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.

greg.1.anderson’s picture

Status: Needs work » Needs review

Here'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 replaced 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.

greg.1.anderson’s picture

Forgot to upload the patch. :P

greg.1.anderson’s picture

Here's an update that is the same as #26, except without the dependency updates to composer.lock.

Mixologic’s picture

The '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.

greg.1.anderson’s picture

We've got the following differences:

  - Removing behat/mink (dev-master a534fe7)
  - Installing behat/mink (dev-master 9ea1ceb): Downloading (100%)         
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Removing behat/mink-selenium2-driver (dev-master 8684ee4)
  - Installing behat/mink-selenium2-driver (dev-master 93474c6): Downloading (100%)         

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.

greg.1.anderson’s picture

Moving 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.

mxr576’s picture

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.

Well 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

greg.1.anderson’s picture

Status: Needs review » Postponed

I 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

Mixologic’s picture

Title: Move scaffold files into a separate component » Utilize scaffold plugin for drupal core

Im retitling to make this more generic as the solution has evolved over time.

Mixologic’s picture

Issue tags: +Composer initiative
Mixologic’s picture

Adding a related, probably will be fixed issue once this goes in.

Mixologic’s picture

Are 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...

greg.1.anderson’s picture

We 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.

Mixologic’s picture

Ah, yes. I concur. less of a 'vendor cleanup' and more of a 'vendor security tightening' plugin

greg.1.anderson’s picture

Assigned: Unassigned » greg.1.anderson
Status: Postponed » Needs work

This is unblocked now that #2912387: Stop using wikimedia/composer-merge-plugin has been committed. I'll re-roll this tomorrow.

Mile23’s picture

#3057094: Add Composer vendor/ hardening plugin to core does now handle placing .htaccess and web.config files in the vendor directory.

Mile23’s picture

Status: Needs work » Needs review
FileSize
136.87 KB
164.89 KB

Here'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 friend ManageGitIgnoreTest.

Mile23’s picture

autoload.php might be helpful, and I'm not sure how those changes to workspace module got in.

Mile23’s picture

Ahh, the extra is stored in composer.lock, so you have to do the branch dance to update that, too.

Branch dance consisting of:

$ git checkout 3067645 // work branch
# do some work, commit your work.
$ git checkout 8.8.x
$ git merge 3067645 --squash
$ git checkout 8.8.x composer.lock
$ rm -rf vendor/
$ composer update drupal/core-scaffold-asset
$ git diff composer.lock > lock.patch
$ git reset --hard
$ git checkout 3067645
$ git apply lock.patch
$ git add composer.lock
$ git commit -m 'now with lock file for the testbot. path repos ftw!'

See #42 for some explanations.

greg.1.anderson’s picture

Neat.

It moves our Composer packages to a folder in the root called composer/.

I like this relocation. We should move the Scaffold plugin to its final location in #2912387: Stop using wikimedia/composer-merge-plugin, though.

It adds a path repo to a package called drupal/core-scaffold-assets.

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.

There are a few failing tests, notably ComposerHookTest and our old friend ManageGitIgnoreTest.

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.

Status: Needs review » Needs work

The last submitted patch, 44: 3067645_44.patch, failed testing. View results

Mile23’s picture

OK, 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.

Mile23’s picture

Title: Utilize scaffold plugin for drupal core » Add core scaffold assets to drupal/core's composer.json extra field
greg.1.anderson’s picture

Issue summary: View changes

Updated issue summary to reflect current strategy to increase focus on backwards compatibility.

greg.1.anderson’s picture

Issue summary: View changes

Update list of asset files in release notes summary.

greg.1.anderson’s picture

Issue summary: View changes

Update list of scaffold files.

greg.1.anderson’s picture

Here 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.

greg.1.anderson’s picture

Issue summary: View changes

I 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.

greg.1.anderson’s picture

Issue summary: View changes

Added 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.

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
92.76 KB
4.5 KB

There 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.

Mixologic’s picture

Issue summary lgtm.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/assets/scaffold/README.txt
@@ -0,0 +1,10 @@
+The scaffold files will be deleted from their original location in Drupal 9.

+++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -169,6 +169,77 @@ public function testAllModulesReplaced() {
+   * In Drupal 9, the files at the destination path will be removed. For the
+   * remainder of the Drupal 8 development cycle, these files will remain in
+   * order to maintain backwards compatibility with sites based on the template
+   * project drupal-composer/drupal-project.

Can we get a follow-up issue for this opened against the 9.x branch, and reference it here.

greg.1.anderson’s picture

Coming right up.

greg.1.anderson’s picture

Added follow-on issue #3075954: Remove duplicate scaffold files and included links to it in tests and scaffold files README.txt.

greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

Follow-up issue created and cross-linked, and tests came back green; back to RTBC.

Mixologic’s picture

Solid. +1

greg.1.anderson’s picture

Assigned: greg.1.anderson » Unassigned

Unassigned.

greg.1.anderson’s picture

What 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:

  1. drupal-composer/drupal-scaffold sites that upgrade to 8.8.x-dev
  2. Drupal tarball download generation on drupalorg infrastructure

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.

catch’s picture

The 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.

greg.1.anderson’s picture

I 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.

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed 577ba8b and pushed to 8.8.x. Thanks!

  • larowlan committed 577ba8b on 8.8.x
    Issue #3067645 by greg.1.anderson, Mile23, Mixologic: Add core scaffold...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

hchonov’s picture

I 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?