Problem/Motivation

The README.md is not clear about the preferred status of drupal/core in respect to allowed scaffolding. drupal/core (and drupal/legacy-scaffold-assets) are implicitly allowed to scaffold files. See a.o. \Drupal\Composer\Plugin\Scaffold\AllowedPackages::getAllowedPackages

One allowed package example uses drupal/core as allowed-package, but that is not a realistic example. Therefore I propose to change it.

Proposed resolution

  • Mention that drupal/core is allowed to scaffold files.
  • Modify the example that uses drupal/core as allowed-package
CommentFileSizeAuthor
#7 interdiff-3101214-2-5.txt665 bytesmrinalini9
#5 drupal-compososer-scaffold-allowed-scaffold-3101214-5.patch1020 bytesmrinalini9
#2 drupal-compososer-scaffold-allowed-scaffold-3101214-2.patch1018 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-allowed-scaffold-3101214-2.patch1018 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
Issue tags: +Novice

Thanks for the patch.

1) Patch applies cleanly to 8.9, 9.0, and 9.1.

[mac:kristen:drupal-8.9.x-dev]$ patch -p1 < drupal-compososer-scaffold-allowed-scaffold-3101214-2.patch 
patching file composer/Plugin/Scaffold/README.md

[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < drupal-compososer-scaffold-allowed-scaffold-3101214-2.patch 
patching file composer/Plugin/Scaffold/README.md

[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < drupal-compososer-scaffold-allowed-scaffold-3101214-2.patch 
patching file composer/Plugin/Scaffold/README.md

2) Looking at the code:

+++ b/composer/Plugin/Scaffold/README.md
@@ -77,7 +77,8 @@ web hosting service provider might take its scaffold files from:
+over the projects named before. drupal/core is implicitly allowed and will be
+placed at the top of the list. The top-level composer.json itself is also

Nitpick: To be consistent with the rest of the README, add quotes around `drupal/core`.

3) Back to "Needs work" for 2. And marking Novice as this is simple.

mrinalini9’s picture

mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commented
FileSize
drupal-compososer-scaffold-allowed-scaffold-3101214-5.patch1020 bytes

Rerolled patch to 9.1.x along with the changes mentioned in #4.

Kristen Pol’s picture

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

@mrinalini9 It's good to add an interdiff file when updating a patch. I'll try to review later today unless someone else does.

mrinalini9’s picture

mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commented
FileSize
interdiff-3101214-2-5.txt665 bytes

@Kristen Pol, Thanks for letting me know, it seems like I missed to attach. Please find the interdiff file for the patch #5.

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
Status: Needs review » Reviewed & tested by the community

Thanks for the update and interdiff.

1) Changes seem fine.

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

[mac:kristen:drupal-9.1.x-dev]$ cd89
[mac:kristen:drupal-8.9.x-dev]$ patch -p1 < drupal-compososer-scaffold-allowed-scaffold-3101214-5.patch
patching file composer/Plugin/Scaffold/README.md
[mac:kristen:drupal-8.9.x-dev]$ cd90
[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < drupal-compososer-scaffold-allowed-scaffold-3101214-5.patch
patching file composer/Plugin/Scaffold/README.md
[mac:kristen:drupal-9.0.x-dev]$ cd91
[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < drupal-compososer-scaffold-allowed-scaffold-3101214-5.patch
patching file composer/Plugin/Scaffold/README.md

3) Reviewed the wording again and it seems ok to me.

4) Tests pass (for 8.9).

5) Marking RTBC to see if this will be considered for a documentation update.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan at LimoenGroen commented
Issue tags: +Composer initiative

Adding 'composer initiative' tag to improve visibility.

alexpott’s picture

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

Committed and pushed 23d5106efe to 9.1.x and 53d40819bd to 9.0.x and 6b88222c6b to 8.9.x. Thanks!

Backported this docs fix to 8.9.x

  • alexpott committed 23d5106 on 9.1.x 
    Issue #3101214 by mrinalini9, Sutharsan, Kristen Pol: Document that Core...

  • alexpott committed 53d4081 on 9.0.x 
    Issue #3101214 by mrinalini9, Sutharsan, Kristen Pol: Document that Core...

  • alexpott committed 6b88222 on 8.9.x 
    Issue #3101214 by mrinalini9, Sutharsan, Kristen Pol: Document that Core...

Status: Fixed » Closed (fixed)

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