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
TestsUpdate scaffold plugin READMEChange record- Update drupal.org documentation as follow-on task (#3082601: Update Composer documentation to match Drupal 8.8.0+ capabilities)
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.
Comment | File | Size | Author |
---|---|---|---|
#32 | 3085697-32.patch | 30.93 KB | Mixologic |
#32 | 3085697-25-to-32-interdiff.txt | 703 bytes | Mixologic |
#25 | 3085697-17-to-25-interdiff.txt | 10.94 KB | greg.1.anderson |
#25 | 3085697-25.patch | 30.96 KB | greg.1.anderson |
#17 | 3085697-17.patch | 25.3 KB | mbaynton |
Comments
Comment #2
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedPatch that enables appending to non-scaffolded files.
Comment #3
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWRONG PATCH FILE.
Comment #4
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #5
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpload correct patch for the issue.
Comment #6
mbayntonIn 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 likefilterProcess()
. 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?Comment #7
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere'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.
Comment #9
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFix code style issues and spurious failure (test error)
Comment #10
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented... and run the tests.
Comment #11
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedMis-rolled patch. This is the intention of #9, see above for interdiff.
Comment #13
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedTurn on gitignore management explicitly for test that expects it.
Comment #14
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented... and run the tests.
Comment #15
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedPer discussion at BADCamp, we are going to rename 'apply-once' to 'force-append', and leave the other things named as they are.
Comment #16
mbayntonI'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.
Comment #17
mbayntonImprove wording on that message
Comment #18
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI am +1 on #17 and +1 on RTBC.
Comment #19
MixologicI'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...
Comment #20
MixologicComment #21
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #22
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #23
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #24
larowlanThere are no declared protected/private properties for these fields
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
could this be simplified to
return !empty($this->data[self::FORCE_APPEND])
?these don't seem to agree
this is 8.8 only right? can use null coalesce?
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
Comment #25
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented1. 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 theelse
; I would not want to make it the responsibility of managing the$scaffoldFiles
map fromScaffoldFileCollection
in an operation. I also hateelse
, but I think it's best to keep it in place here.Thanks for the review - fixes substantially improved the code.
Comment #26
MixologicI 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".
Comment #27
larowlanthis looks like a logic change, previously unset was defaulting to TRUE, now it defaults to FALSE
edit: nvm
❤️🦆 - 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 :)
Comment #28
larowlanCommitted c5ac07a and pushed to 8.8.x. Thanks!
Published change record
Comment #31
larowlanReverted this as it caused a fail in HEAD
Comment #32
MixologicThe rename that happened over in https://www.drupal.org/project/drupal/issues/3086081 caused a new fixture in this patch to be outdated.
Comment #34
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedTested #32 locally as well - that did the trick!
Comment #36
larowlanCommitted c1346b6 and pushed to 8.8.x. Thanks!