Problem/Motivation

The current Scaffold plugin allows append operations to happen only to files that were scaffolded by a previous stage. For example, drupal/core provides a scaffolded robots.txt file, and an individual Drupal site may provide additional scaffold operations to append more directives to the end of the robots.txt file. Every time that the scaffold operation runs, the pristine robots.txt file is copied from the drupal/core asset file, and then the append data is added to the fresh copy. The fact that we always start with a fresh file ensures that append data will not keep piling up in the target file.

In some cases, though, we might want to append to a file that is NOT scaffolded on every run. An example of this would be a Profile that needs code in the user's settings.php file. We can potentially use scaffolding to place this code. The best way to do this is to scaffold a settings.profile.php file, and include said file via include __DIR__ . '/settings.profile.php'; in settings.php. If we require that the Composer Project Template for the site scaffold the settings.php file, then the profile could use a scaffold append op to meet this use-case. If settings.php is not being scaffolded (probably common, since most people are currently used to owning their settings.php files), we would still like to be able to append our include statement to it in a way that works whether or not it is being scaffolded.

For files that are not being scaffolded, then the data will not be rewritten on subsequent scaffold runs. This means that the second and subsequent times that the scaffold operation runs, the append data will already exist in the target file. We must therefore take care to not append the additional content again, as otherwise it will build up and create a separate copy on every scaffold run.

Proposed resolution

We would like the AppendOp to be able to flexibly append to either scaffolded or non-scaffolded files.

  • By default, we will skip the append with an error message if appending to a non-scaffolded file.
  • If the file being appended specifically allows it, then the scaffold plugin will allow append content to be added to a non-scaffolded file, as long as the target file does not yet contain the contents.
  • If the target file does not exist at all, we will allow the AppendOp to contain default data for creating the base form of the file from scratch.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

An "append" operation now has two more attributes, force-append and default.

  • force-append: This attribute indicates that it is acceptable to append to a target file that was not scaffolded. Default is 'false'
  • default: This attribute specifies the relative path to a default file to use in instances where the target file was not scaffolded and does not initially exist.

Example:

      "file-mapping": {
        "[web-root]/sites/default/settings.php": {
          "append": "assets/include-local-settings.txt",
          "force-append": true,
          "default": "assets/initial-default-settings.txt"
        }
      }

Release notes snippet

Scaffold "append" operations may now append to files that were not themselves scaffolded.

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

FileSize
10.96 KB

Patch that enables appending to non-scaffolded files.

greg.1.anderson’s picture

Status: Active » Needs review
FileSize
1.41 KB

WRONG PATCH FILE.

greg.1.anderson’s picture

Issue summary: View changes
Issue tags: +Composer initiative
greg.1.anderson’s picture

Issue summary: View changes
FileSize
18.58 KB

Upload correct patch for the issue.

mbaynton’s picture

In approximate order of what I approximate is important:

* I think we should be more noisy, in terms of Composer output, when we modify existing, otherwise unmanaged files the user owns. This is not expected Composer behavior, so noting it might help reduce confusion / "where did that come from" moments.

This feels like we're building some of the basic capabilities of Symfony Flex here, which I think lends credibility to the concept. When Flex makes modifications to not-strictly-Composer files because it's almost surely the right modification to make, though, it points out that it's modified something but that you should review the modifications and that "these files are *yours*".

* I see our returned ScaffoldResult always asserts that the file is managed. Am I correctly assuming that this will gitignore the mostly unmanaged file?

* Naming things is hard. For apply-once I wonder if we can find something more self-documenting of "This attribute indicates that it is acceptable to append to a target file that was not scaffolded," while remaining succinct. I'll think on it.

* Naming things is hard. It took me a bit to work through that the effect of the new missingConjunctionTarget() method. Looks like it is to filter execution of the operation, but only when the file being operated on was not scaffolded already. Because of the "but", the name cannot be something simple like filterProcess(). Do you think it would be very disruptive to generalize the interface and method names a bit, and always invoke the method that returns a SkipOp for Operations that implement the interface?

greg.1.anderson’s picture

Here's a patch with the first two items addressed (or at least an attempt thereof).

Still trying to name things, though; no progress there yet.

Status: Needs review » Needs work

The last submitted patch, 7: 3085697-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

greg.1.anderson’s picture

Fix code style issues and spurious failure (test error)

greg.1.anderson’s picture

Status: Needs work » Needs review

... and run the tests.

greg.1.anderson’s picture

FileSize
25.28 KB

Mis-rolled patch. This is the intention of #9, see above for interdiff.

Status: Needs review » Needs work

The last submitted patch, 11: 3085697-11.patch, failed testing. View results

greg.1.anderson’s picture

Turn on gitignore management explicitly for test that expects it.

greg.1.anderson’s picture

Status: Needs work » Needs review

... and run the tests.

greg.1.anderson’s picture

Per discussion at BADCamp, we are going to rename 'apply-once' to 'force-append', and leave the other things named as they are.

mbaynton’s picture

I'm pretty good on this now. Just want to clean up one message the existing file exists at the target path already has the append/prepend data.

mbaynton’s picture

Improve wording on that message

greg.1.anderson’s picture

I am +1 on #17 and +1 on RTBC.

Mixologic’s picture

I've poked around in this and the functionality is valuable, and the implementation is solid.

I think the only thing we'd need to do is make sure that we update this when it gets in:
https://www.drupal.org/docs/develop/using-composer/using-drupals-compose...

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community
greg.1.anderson’s picture

Issue summary: View changes
greg.1.anderson’s picture

Issue summary: View changes
greg.1.anderson’s picture

Issue summary: View changes
larowlan’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/composer/Plugin/Scaffold/Operations/AppendOp.php
    @@ -37,10 +42,17 @@ class AppendOp implements OperationInterface, ConjoinableInterface {
    +    $this->forceAppend = $force_append;
    ...
    +    $this->default = $default_path;
    

    There are no declared protected/private properties for these fields

  2. +++ b/composer/Plugin/Scaffold/Operations/AppendOp.php
    @@ -67,18 +93,75 @@ public function process(ScaffoldFilePath $destination, IOInterface $io, Scaffold
    +    if ($this->existingFileHasData($existingData, $this->append) || $this->existingFileHasData($existingData, $this->prepend)) {
    +      $message = "  - Skip <info>[dest-rel-path]</info>: the file already has the append/prepend data.";
    +      return new SkipOp($message);
    

    what if it has one but not the other? think we're missing a test case for that too - should it skip prepend if only prepend is there? I'd guess no

  3. +++ b/composer/Plugin/Scaffold/Operations/OperationData.php
    @@ -88,6 +90,19 @@ public function overwrite() {
    +    return isset($this->data[self::FORCE_APPEND]) ? $this->data[self::FORCE_APPEND] : FALSE;
    

    could this be simplified to return !empty($this->data[self::FORCE_APPEND]) ?

  4. +++ b/composer/Plugin/Scaffold/Operations/OperationData.php
    @@ -128,6 +143,26 @@ public function append() {
    +   * Checks if default path exists.
    ...
    +   *   Returns true if prepend exists.
    ...
    +    return isset($this->data[self::DEFAULT]);
    

    these don't seem to agree

  5. +++ b/composer/Plugin/Scaffold/Operations/OperationData.php
    @@ -128,6 +143,26 @@ public function append() {
    +    return isset($this->data[self::DEFAULT]) ? $this->data[self::DEFAULT] : '';
    

    this is 8.8 only right? can use null coalesce?

  6. +++ b/composer/Plugin/Scaffold/Operations/ScaffoldFileCollection.php
    @@ -57,6 +58,14 @@ public function __construct(array $file_mappings, Interpolator $location_replace
    +        elseif ($op instanceof ConjoinableInterface) {
    +          $op = $op->missingConjunctionTarget($destination);
    +        }
    

    ick, elseif and instanceof in one line, both are red flags in my book. Is there anything else we can do here? Duck typing should help get rid of the instanceof, add a ::supportsConjunction method, but it feels like the collection shouldn't have this knowledge of the operations, and instead the operations should be asked what to do in case of an existing operation/missing file - that would do away with both instanceof checks in this class, and the need for ConjoinableInterface altogether

greg.1.anderson’s picture

Status: Needs work » Needs review
FileSize
30.96 KB
10.94 KB

1. Explicitly declared class fields.

2. This scenario is not supported. What is supported here is a project (e.g. installation profile or hosting provider adapter package) may include a bit of text to append / prepend to an existing file (e.g. settings.php). If this project later changes the text to be appended, then the new value will get appended again. There is no facility at this time to identify and remove the text that was appended in the past. Having append text but not prepend text is a more specific example of this; having the append text in place but not the prepend text implies that only the append text appeared in the past, and then prepend text was added to the project that provided append/prepend data.

We could consider providing refinements to this feature to do things like that in the future, but it seems like too much complexity at this time. It would be a little easier to detect the 'just prepend' / 'just append' data present scenario, but if we did that it would beg the question as to why we did not correctly handle alteration as well.

It will probably be rare to have prepend data with this feature anyway.

3. Simplified to !empty.

4. Fixed docblock comment cut-copy-pasta

5. Yes, could have used null coalesce here, but avoided even that by using default values for the metadata.

6. Quack quack - got rid of instanceof with Duck Typing. This did not get rid of the else; I would not want to make it the responsibility of managing the $scaffoldFiles map from ScaffoldFileCollection in an operation. I also hate else, but I think it's best to keep it in place here.

Thanks for the review - fixes substantially improved the code.

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community

I did some more review and my eyes aren't finding things to polish or question. Changes appear to have addressed everything in #24, so I'll go ahead and say "yes, and".

larowlan’s picture

  1. +++ b/composer/Plugin/Scaffold/Operations/OperationData.php
    @@ -87,7 +87,7 @@ public function path() {
    -    return isset($this->data[self::OVERWRITE]) ? $this->data[self::OVERWRITE] : TRUE;
    +    return !empty($this->data[self::OVERWRITE]);
    

    this looks like a logic change, previously unset was defaulting to TRUE, now it defaults to FALSE

  2. +++ b/composer/Plugin/Scaffold/Operations/OperationData.php
    @@ -176,9 +176,32 @@ public function default() {
    +      self::OVERWRITE => TRUE,
    

    edit: nvm

  3. +++ b/composer/Plugin/Scaffold/Operations/OperationInterface.php
    @@ -26,4 +26,31 @@ interface OperationInterface {
    +  public function combineWithConjunctionTarget(OperationInterface $conjunction_target);
    ...
    +  public function missingConjunctionTarget(ScaffoldFilePath $destination);
    

    ❤️🦆 - this is lovely cleanup. Instanceof as a 🚩again shows the path forward. Thank you Sandy Metz for teaching me this. The else is fine, its a step down from an elseif :)

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed c5ac07a and pushed to 8.8.x. Thanks!

Published change record

  • larowlan committed c5ac07a on 8.8.x
    Issue #3085697 by greg.1.anderson, mbaynton, Mixologic, larowlan: Allow...

  • larowlan committed f5040c6 on 8.8.x
    Revert "Issue #3085697 by greg.1.anderson, mbaynton, Mixologic, larowlan...
larowlan’s picture

Status: Fixed » Needs work

Reverted this as it caused a fail in HEAD

Mixologic’s picture

Status: Needs work » Needs review
FileSize
703 bytes
30.93 KB

The rename that happened over in https://www.drupal.org/project/drupal/issues/3086081 caused a new fixture in this patch to be outdated.

The last submitted patch, 25: 3085697-25.patch, failed testing. View results

greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

Tested #32 locally as well - that did the trick!

  • larowlan committed c1346b6 on 8.8.x
    Issue #3085697 by greg.1.anderson, Mixologic, mbaynton, larowlan: Allow...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed c1346b6 and pushed to 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

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