Problem/Motivation

In README.md one example lists "overwrite": true under extra > drupal-scaffold.

If I interpret the code correctly, it is not possible to set a default 'overwrite' at scaffold level. \Drupal\Composer\Plugin\Scaffold\ScaffoldOptions::__construct does not define an 'overwrite' default. Overwriting is default, but adding it explicitly to an example at scaffold level gives a wrong signal as if other defaults are possible.

Proposed resolution

Update the documentation.

CommentFileSizeAuthor
#5 interdiff-3101210-2-5.txt444 bytesmrinalini9
#5 drupal-compososer-scaffold-overwrite-3101210-5.patch1.15 KBmrinalini9
#2 drupal-compososer-scaffold-overwrite-3101210-2.patch524 bytesSutharsan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sutharsan created an issue. See original summary.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan at LimoenGroen commented
Status: Active » Needs review
FileSize
drupal-compososer-scaffold-overwrite-3101210-2.patch524 bytes

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs review » Needs work

Thanks for the patch.

Maybe I'm misunderstanding this but I see:

 "overwrite": true,

in

core/tests/Drupal/Tests/Composer/Plugin/Scaffold/fixtures/drupal-drupal/composer.json.tmpl

here:

  "extra": {
    "drupal-scaffold": {
      "allowed-packages": [
        "fixtures/drupal-core-fixture",
        "fixtures/scaffold-override-fixture"
      ],
      "gitignore": false,
      "overwrite": true,
      "symlink": __SYMLINK__,

If what you have in the issue summary is correct, should this be removed here as well?

Moving back to "Needs work" in case that's true.

mrinalini9’s picture

mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commented
FileSize
drupal-compososer-scaffold-overwrite-3101210-5.patch1.15 KB
interdiff-3101210-2-5.txt444 bytes

Rerolled patch to 9.1.x with the changes mentioned in #4 as I think it should be removed here as well.

mrinalini9’s picture

mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs work » Needs review
Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented

Thanks for the update.

1) Reviewed the interdiff and changes seem ok (assuming this is the correct approach).

2) Patch applies cleanly to 8.9, 9.0, and 9.1

[mac:kristen:drupal-8.9.x-dev]$ patch -p1 < drupal-compososer-scaffold-overwrite-3101210-5.patch
patching file composer/Plugin/Scaffold/README.md
patching file core/tests/Drupal/Tests/Composer/Plugin/Scaffold/fixtures/drupal-drupal/composer.json.tmpl
[mac:kristen:drupal-8.9.x-dev]$ cd90
[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < drupal-compososer-scaffold-overwrite-3101210-5.patch
patching file composer/Plugin/Scaffold/README.md
patching file core/tests/Drupal/Tests/Composer/Plugin/Scaffold/fixtures/drupal-drupal/composer.json.tmpl
[mac:kristen:drupal-9.0.x-dev]$ cd91
[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < drupal-compososer-scaffold-overwrite-3101210-5.patch
patching file composer/Plugin/Scaffold/README.md
patching file core/tests/Drupal/Tests/Composer/Plugin/Scaffold/fixtures/drupal-drupal/composer.json.tmpl

3) Searching for "overwrite": true after patching I find:

./core/tests/Drupal/Tests/Composer/Plugin/Scaffold/fixtures/drupal-drupal/composer.json.tmpl:          "overwrite": true
./core/tests/Drupal/Tests/Composer/Plugin/Scaffold/fixtures/drupal-drupal-test-overwrite/composer.json.tmpl:          "overwrite": true
./composer/Plugin/Scaffold/README.md:    "overwrite": true

but these are under file-mapping.

4) I'm hesitant to mark RTBC as I don't know enough about the structure to convince myself the changes are needed and correct. Leaving as "Needs review" for others more familiar with the code.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Needs review » Reviewed & tested by the community

In the ancient past (pre-Drupal 8.8.0, during development), the Scaffold Plugin had a top-level "overwrite" property that could be used to invert the default behavior of the replace op. This property was deemed to not be useful and perhaps confusing, so it was removed. Mostly.

This patch correctly identifies the places where vestigial references to this property remained, and should be removed. "Overwrite" is still a property of the replace op itself (for setting the overwrite behavior on a file-by-file basis); the places identified in #7 where it remains are appropriate and correct references.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Issue tags: +Composer initiative
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 8ac02b5cbd to 9.1.x and 4b72c1c605 to 9.0.x and 8d61bcad63 to 8.9.x. Thanks!

Backported all the way to 8.9.x because this is a docs and template fix. We could also choose to backport this to 8.8.x is we want.

  • alexpott committed 8ac02b5 on 9.1.x 
    Issue #3101210 by mrinalini9, Sutharsan, Kristen Pol, greg.1.anderson:...

  • alexpott committed 4b72c1c on 9.0.x 
    Issue #3101210 by mrinalini9, Sutharsan, Kristen Pol, greg.1.anderson:...

  • alexpott committed 8d61bca on 8.9.x 
    Issue #3101210 by mrinalini9, Sutharsan, Kristen Pol, greg.1.anderson:...

Status: Fixed » Closed (fixed)

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