Issue #3103090 by greg.1.anderson, alexpott, jungle, ravi.shankar, dww, Mile23, larowlan: Avoid re-scaffolding unchanged files (and printing scaffold file information over and over)

merge-requests/2419/head
Alex Pott 2020-05-03 11:42:30 +01:00
parent c285e2b22c
commit c1548fce81
No known key found for this signature in database
GPG Key ID: 31905460D4A69276
12 changed files with 252 additions and 138 deletions

View File

@ -154,11 +154,17 @@ class Handler {
$scaffold_options = $this->manageOptions->getOptions();
// Create a collection of scaffolded files to process. This determines which
// take priority and which are conjoined.
// take priority and which are combined.
$scaffold_files = new ScaffoldFileCollection($file_mappings, $location_replacements);
// Get the scaffold files whose contents on disk match what we are about to
// write. We can remove these from consideration, as rewriting would be a
// no-op.
$unchanged = $scaffold_files->checkUnchanged();
$scaffold_files->filterFiles($unchanged);
// Process the list of scaffolded files.
$scaffold_results = ScaffoldFileCollection::process($scaffold_files, $this->io, $scaffold_options);
$scaffold_results = $scaffold_files->process($this->io, $scaffold_options);
// Generate an autoload file in the document root that includes the
// autoload.php file in the vendor directory, wherever that is. Drupal
@ -195,7 +201,7 @@ class Handler {
* A multidimensional array of file mappings, as returned by
* self::getAllowedPackages().
*
* @return \Drupal\Composer\Plugin\Scaffold\Operations\OperationInterface[]
* @return \Drupal\Composer\Plugin\Scaffold\Operations\OperationInterface[][]
* An array of destination paths => scaffold operation objects.
*/
protected function getFileMappingsFromPackages(array $allowed_packages) {

View File

@ -11,17 +11,42 @@ use Drupal\Composer\Plugin\Scaffold\ScaffoldFilePath;
*/
abstract class AbstractOperation implements OperationInterface {
/**
* Cached contents of scaffold file to be written to disk.
*
* @var string
*/
protected $contents;
/**
* {@inheritdoc}
*/
public function combineWithConjunctionTarget(OperationInterface $conjunction_target) {
final public function contents() {
if (!isset($this->contents)) {
$this->contents = $this->generateContents();
}
return $this->contents;
}
/**
* Load the scaffold contents or otherwise generate what is needed.
*
* @return string
* The contents of the scaffold file.
*/
abstract protected function generateContents();
/**
* {@inheritdoc}
*/
public function scaffoldOverExistingTarget(OperationInterface $existing_target) {
return $this;
}
/**
* {@inheritdoc}
*/
public function missingConjunctionTarget(ScaffoldFilePath $destination) {
public function scaffoldAtNewLocation(ScaffoldFilePath $destination) {
return $this;
}

View File

@ -49,6 +49,13 @@ class AppendOp extends AbstractOperation {
*/
protected $forceAppend;
/**
* The contents from the file that we are prepending / appending to.
*
* @var string
*/
protected $originalContents;
/**
* Constructs an AppendOp.
*
@ -69,16 +76,36 @@ class AppendOp extends AbstractOperation {
$this->managed = TRUE;
}
/**
* {@inheritdoc}
*/
protected function generateContents() {
// Fetch the prepend contents, if provided.
$prepend_contents = '';
if (!empty($this->prepend)) {
$prepend_contents = file_get_contents($this->prepend->fullPath()) . "\n";
}
// Fetch the append contents, if provided.
$append_contents = '';
if (!empty($this->append)) {
$append_contents = "\n" . file_get_contents($this->append->fullPath());
}
// Get the original contents, or the default data if the original is empty.
$original_contents = $this->originalContents;
if (empty($original_contents) && !empty($this->default)) {
$original_contents = file_get_contents($this->default->fullPath());
}
// Attach it all together.
return $prepend_contents . $original_contents . $append_contents;
}
/**
* {@inheritdoc}
*/
public function process(ScaffoldFilePath $destination, IOInterface $io, ScaffoldOptions $options) {
$destination_path = $destination->fullPath();
// This is just a sanity check; the OperationFactory has in theory already
// accounted for this, and will return a SkipOp with a warning message.
if (!file_exists($destination_path) && empty($this->default)) {
throw new \RuntimeException($destination->getInterpolator()->interpolate("Cannot append/prepend because no prior package provided a scaffold file at [dest-rel-path]."));
}
$interpolator = $destination->getInterpolator();
// Be extra-noisy of creating a new file or appending to a non-scaffold
@ -93,33 +120,20 @@ class AppendOp extends AbstractOperation {
$io->write($interpolator->interpolate($message));
}
// Fetch the prepend contents, if provided.
$prepend_contents = '';
// Notify that we are prepending, if there is prepend data.
if (!empty($this->prepend)) {
$this->prepend->addInterpolationData($interpolator, 'prepend');
$prepend_contents = file_get_contents($this->prepend->fullPath()) . "\n";
$io->write($interpolator->interpolate(" - Prepend to <info>[dest-rel-path]</info> from <info>[prepend-rel-path]</info>"));
}
// Fetch the append contents, if provided.
// Notify that we are appending, if there is append data.
$append_contents = '';
if (!empty($this->append)) {
$this->append->addInterpolationData($interpolator, 'append');
$append_contents = "\n" . file_get_contents($this->append->fullPath());
$io->write($interpolator->interpolate(" - Append to <info>[dest-rel-path]</info> from <info>[append-rel-path]</info>"));
}
// We typically should always have content if we get here; the
// OperationFactory should create a SkipOp instead of an AppendOp if there
// is no append / prepend content. The edge case is if there is content
// that is all 'trim'ed away. Then we get a message that we are appending,
// although nothing will in fact actually happen.
if (!empty(trim($prepend_contents)) || !empty(trim($append_contents))) {
// None of our asset files are very large, so we will load each one into
// memory for processing.
$original_contents = file_get_contents(file_exists($destination_path) ? $destination_path : $this->default->fullPath());
// Write the appended and prepended contents back to the file.
$altered_contents = $prepend_contents . $original_contents . $append_contents;
file_put_contents($destination_path, $altered_contents);
}
// Write the resulting data
file_put_contents($destination_path, $this->contents());
// Return a ScaffoldResult with knowledge of whether this file is managed.
return new ScaffoldResult($destination, $this->managed);
@ -128,16 +142,17 @@ class AppendOp extends AbstractOperation {
/**
* {@inheritdoc}
*/
public function combineWithConjunctionTarget(OperationInterface $conjunction_target) {
return new ConjunctionOp($conjunction_target, $this);
public function scaffoldOverExistingTarget(OperationInterface $existing_target) {
$this->originalContents = $existing_target->contents();
return $this;
}
/**
* {@inheritdoc}
*/
public function missingConjunctionTarget(ScaffoldFilePath $destination) {
// If there is no conjunction target (the destination is not scaffolded),
// then any append we do will be to an unmanaged file.
public function scaffoldAtNewLocation(ScaffoldFilePath $destination) {
// If there is no existing scaffold file at the target location, then any
// append we do will be to an unmanaged file.
$this->managed = FALSE;
// Default: do not allow an append over a file that was not scaffolded.
@ -164,6 +179,9 @@ class AppendOp extends AbstractOperation {
return new SkipOp($message);
}
// Cache the original data to use during append.
$this->originalContents = $existingData;
return $this;
}

View File

@ -1,53 +0,0 @@
<?php
namespace Drupal\Composer\Plugin\Scaffold\Operations;
use Composer\IO\IOInterface;
use Drupal\Composer\Plugin\Scaffold\ScaffoldFilePath;
use Drupal\Composer\Plugin\Scaffold\ScaffoldOptions;
/**
* Joins two operations on the same file into a single operation.
*
* @internal
*/
class ConjunctionOp extends AbstractOperation {
/**
* The first operation.
*
* @var \Drupal\Composer\Plugin\Scaffold\Operations\OperationInterface
*/
protected $firstOperation;
/**
* The second operation.
*
* @var \Drupal\Composer\Plugin\Scaffold\Operations\OperationInterface
*/
protected $secondOperation;
/**
* ConjunctionOp constructor.
*
* @param \Drupal\Composer\Plugin\Scaffold\Operations\OperationInterface $first_operation
* @param \Drupal\Composer\Plugin\Scaffold\Operations\OperationInterface $second_operation
*/
public function __construct(OperationInterface $first_operation, OperationInterface $second_operation) {
$this->firstOperation = $first_operation;
$this->secondOperation = $second_operation;
}
/**
* {@inheritdoc}
*/
public function process(ScaffoldFilePath $destination, IOInterface $io, ScaffoldOptions $options) {
$destination_path = $destination->fullPath();
// First, scaffold the original file. Disable symlinking, because we
// need a copy of the file if we're going to append / prepend to it.
@unlink($destination_path);
$this->firstOperation->process($destination, $io, $options->overrideSymlink(FALSE));
return $this->secondOperation->process($destination, $io, $options);
}
}

View File

@ -13,6 +13,14 @@ use Drupal\Composer\Plugin\Scaffold\ScaffoldOptions;
*/
interface OperationInterface {
/**
* Returns the exact data that will be written to the scaffold files.
*
* @return string
* Data to be written to the scaffold location.
*/
public function contents();
/**
* Process this scaffold operation.
*
@ -29,18 +37,18 @@ interface OperationInterface {
public function process(ScaffoldFilePath $destination, IOInterface $io, ScaffoldOptions $options);
/**
* Determines what to do if operation is used with a previous operation.
* Determines what to do if operation is used at same path as a previous op.
*
* Default behavior is to scaffold this operation at the specified
* destination, ignoring whatever was there before.
*
* @param OperationInterface $conjunction_target
* @param OperationInterface $existing_target
* Existing file at the destination path that we should combine with.
*
* @return OperationInterface
* The op to use at this destination.
*/
public function combineWithConjunctionTarget(OperationInterface $conjunction_target);
public function scaffoldOverExistingTarget(OperationInterface $existing_target);
/**
* Determines what to do if operation is used without a previous operation.
@ -50,9 +58,12 @@ interface OperationInterface {
* and therefore do not need to do anything special when there is no existing
* file.
*
* @param \Drupal\Composer\Plugin\Scaffold\ScaffoldFilePath $destination
* Scaffold file's destination path.
*
* @return OperationInterface
* The op to use at this destination.
*/
public function missingConjunctionTarget(ScaffoldFilePath $destination);
public function scaffoldAtNewLocation(ScaffoldFilePath $destination);
}

View File

@ -47,6 +47,13 @@ class ReplaceOp extends AbstractOperation {
$this->overwrite = $overwrite;
}
/**
* {@inheritdoc}
*/
protected function generateContents() {
return file_get_contents($this->source->fullPath());
}
/**
* {@inheritdoc}
*/
@ -85,8 +92,7 @@ class ReplaceOp extends AbstractOperation {
protected function copyScaffold(ScaffoldFilePath $destination, IOInterface $io) {
$interpolator = $destination->getInterpolator();
$this->source->addInterpolationData($interpolator);
$fs = new Filesystem();
$success = $fs->copy($this->source->fullPath(), $destination->fullPath());
$success = file_put_contents($destination->fullPath(), $this->contents());
if (!$success) {
throw new \RuntimeException($interpolator->interpolate("Could not copy source file <info>[src-rel-path]</info> to <info>[dest-rel-path]</info>!"));
}

View File

@ -29,15 +29,16 @@ class ScaffoldFileCollection implements \IteratorAggregate {
/**
* ScaffoldFileCollection constructor.
*
* @param array $file_mappings
* @param \Drupal\Composer\Plugin\Scaffold\Operations\OperationInterface[][] $file_mappings
* A multidimensional array of file mappings.
* @param \Drupal\Composer\Plugin\Scaffold\Interpolator $location_replacements
* An object with the location mappings (e.g. [web-root]).
*/
public function __construct(array $file_mappings, Interpolator $location_replacements) {
// Collection of all destination paths to be scaffolded. Used to determine
// when two project scaffold the same file and we have to skip or use a
// ConjunctionOp.
// when two projects scaffold the same file and we have to either replace or
// combine them together.
// @see OperationInterface::scaffoldOverExistingTarget().
$scaffoldFiles = [];
// Build the list of ScaffoldFileInfo objects by project.
@ -46,24 +47,20 @@ class ScaffoldFileCollection implements \IteratorAggregate {
$destination = ScaffoldFilePath::destinationPath($package_name, $destination_rel_path, $location_replacements);
// If there was already a scaffolding operation happening at this path,
// and the new operation is Conjoinable, then use a ConjunctionOp to
// join together both operations. This will cause both operations to
// run, one after the other. At the moment, only AppendOp is
// conjoinable; all other operations simply replace anything at the same
// path.
// allow the new operation to decide how to handle the override.
// Usually, the new operation will replace whatever was there before.
if (isset($scaffoldFiles[$destination_rel_path])) {
$previous_scaffold_file = $scaffoldFiles[$destination_rel_path];
$op = $op->combineWithConjunctionTarget($previous_scaffold_file->op());
$op = $op->scaffoldOverExistingTarget($previous_scaffold_file->op());
// Remove the previous op so we only touch the destination once.
$message = " - Skip <info>[dest-rel-path]</info>: overridden in <comment>{$package_name}</comment>";
$this->scaffoldFilesByProject[$previous_scaffold_file->packageName()][$destination_rel_path] = new ScaffoldFileInfo($destination, new SkipOp($message));
}
// If there is NOT already a scaffolding operation happening at this
// path, but the operation is a ConjunctionOp, then we need to check
// to see if there is a strategy for non-conjunction use.
// path, notify the scaffold operation of this fact.
else {
$op = $op->missingConjunctionTarget($destination);
$op = $op->scaffoldAtNewLocation($destination);
}
// Combine the scaffold operation with the destination and record it.
@ -75,17 +72,56 @@ class ScaffoldFileCollection implements \IteratorAggregate {
}
/**
* {@inheritdoc}
* Removes any item that has a path matching any path in the provided list.
*
* Matching is done via destination path.
*
* @param string[] $files_to_filter
* List of destination paths
*/
public function getIterator() {
return new \RecursiveArrayIterator($this->scaffoldFilesByProject, \RecursiveArrayIterator::CHILD_ARRAYS_ONLY);
public function filterFiles(array $files_to_filter) {
foreach ($this->scaffoldFilesByProject as $project_name => $scaffold_files) {
foreach ($scaffold_files as $destination_rel_path => $scaffold_file) {
if (in_array($destination_rel_path, $files_to_filter, TRUE)) {
unset($scaffold_files[$destination_rel_path]);
}
}
$this->scaffoldFilesByProject[$project_name] = $scaffold_files;
if (!$this->checkListHasItemWithContent($scaffold_files)) {
unset($this->scaffoldFilesByProject[$project_name]);
}
}
}
/**
* Processes the iterator created by ScaffoldFileCollection::create().
* Scans through a list of scaffold files and determines if any has contents.
*
* @param Drupal\Composer\Plugin\Scaffold\ScaffoldFileInfo[] $scaffold_files
* List of scaffold files, path: ScaffoldFileInfo
*
* @return bool
* TRUE if at least one item in the list has content
*/
protected function checkListHasItemWithContent(array $scaffold_files) {
foreach ($scaffold_files as $destination_rel_path => $scaffold_file) {
$contents = $scaffold_file->op()->contents();
if (!empty($contents)) {
return TRUE;
}
}
return FALSE;
}
/**
* {@inheritdoc}
*/
public function getIterator() {
return new \ArrayIterator($this->scaffoldFilesByProject);
}
/**
* Processes the files in our collection.
*
* @param \Drupal\Composer\Plugin\Scaffold\Operations\ScaffoldFileCollection $collection
* The iterator to process.
* @param \Composer\IO\IOInterface $io
* The Composer IO object.
* @param \Drupal\Composer\Plugin\Scaffold\ScaffoldOptions $scaffold_options
@ -94,9 +130,9 @@ class ScaffoldFileCollection implements \IteratorAggregate {
* @return \Drupal\Composer\Plugin\Scaffold\Operations\ScaffoldResult[]
* The results array.
*/
public static function process(ScaffoldFileCollection $collection, IOInterface $io, ScaffoldOptions $scaffold_options) {
public function process(IOInterface $io, ScaffoldOptions $scaffold_options) {
$results = [];
foreach ($collection as $project_name => $scaffold_files) {
foreach ($this as $project_name => $scaffold_files) {
$io->write("Scaffolding files for <comment>{$project_name}</comment>:");
foreach ($scaffold_files as $scaffold_file) {
$results[$scaffold_file->destination()->relativePath()] = $scaffold_file->process($io, $scaffold_options);
@ -105,4 +141,30 @@ class ScaffoldFileCollection implements \IteratorAggregate {
return $results;
}
/**
* Returns the list of files that have not changed since they were scaffolded.
*
* Note that there are two reasons a file may have changed:
* - The user modified it after it was scaffolded.
* - The package the file came to was updated, and the file is different in
* the new version.
*
* With the current scaffold code, we cannot tell the difference between the
* two. @see https://www.drupal.org/project/drupal/issues/3092563
*
* @return string[]
* List of relative paths to unchanged files on disk.
*/
public function checkUnchanged() {
$results = [];
foreach ($this as $project_name => $scaffold_files) {
foreach ($scaffold_files as $scaffold_file) {
if (!$scaffold_file->hasChanged()) {
$results[] = $scaffold_file->destination()->relativePath();
}
}
}
return $results;
}
}

View File

@ -35,6 +35,13 @@ class SkipOp extends AbstractOperation {
$this->message = $message;
}
/**
* {@inheritdoc}
*/
protected function generateContents() {
return '';
}
/**
* {@inheritdoc}
*/

View File

@ -123,4 +123,17 @@ class ScaffoldFileInfo {
return $this->op()->process($this->destination, $io, $options);
}
/**
* Returns TRUE if the target does not exist or has changed.
*
* @return bool
*/
final public function hasChanged() {
$path = $this->destination()->fullPath();
if (!file_exists($path)) {
return TRUE;
}
return $this->op()->contents() !== file_get_contents($path);
}
}

View File

@ -26,16 +26,6 @@ class ComposerHookTest extends TestCase {
use ExecTrait;
use AssertUtilsTrait;
/**
* The root of this project.
*
* Used to substitute this project's base directory into composer.json files
* so Composer can find it.
*
* @var string
*/
protected $projectRoot;
/**
* Directory to perform the tests in.
*
@ -64,7 +54,9 @@ class ComposerHookTest extends TestCase {
$this->fileSystem = new Filesystem();
$this->fixtures = new Fixtures();
$this->fixtures->createIsolatedComposerCacheDir();
$this->projectRoot = $this->fixtures->projectRoot();
$this->fixturesDir = $this->fixtures->tmpDir($this->getName());
$replacements = ['SYMLINK' => 'false', 'PROJECT_ROOT' => $this->fixtures->projectRoot()];
$this->fixtures->cloneFixtureProjects($this->fixturesDir, $replacements);
}
/**
@ -79,9 +71,6 @@ class ComposerHookTest extends TestCase {
* Test to see if scaffold operation runs at the correct times.
*/
public function testComposerHooks() {
$this->fixturesDir = $this->fixtures->tmpDir($this->getName());
$replacements = ['SYMLINK' => 'false', 'PROJECT_ROOT' => $this->projectRoot];
$this->fixtures->cloneFixtureProjects($this->fixturesDir, $replacements);
$topLevelProjectDir = 'composer-hooks-fixture';
$sut = $this->fixturesDir . '/' . $topLevelProjectDir;
// First test: run composer install. This is the same as composer update
@ -136,4 +125,29 @@ class ComposerHookTest extends TestCase {
$this->assertStringContainsString("Not scaffolding files for fixtures/scaffold-override-fixture, because it is not listed in the element 'extra.drupal-scaffold.allowed-packages' in the root-level composer.json file.", $stdout);
}
/**
* Test to see if scaffold messages are omitted when running scaffold twice.
*/
public function testScaffoldMessagesDoNotPrintTwice() {
$topLevelProjectDir = 'drupal-drupal';
$sut = $this->fixturesDir . '/' . $topLevelProjectDir;
// First test: run composer install. This is the same as composer update
// since there is no lock file. Ensure that scaffold operation ran.
$stdout = $this->mustExec("composer install --no-ansi", $sut);
$this->assertStringContainsString('- Copy [web-root]/index.php from assets/index.php', $stdout);
$this->assertStringContainsString('- Copy [web-root]/update.php from assets/update.php', $stdout);
// Run scaffold operation again. It should not print anything.
$stdout = $this->mustExec("composer scaffold --no-ansi", $sut);
$this->assertEquals('', $stdout);
// Delete a file and run it again. It should re-scaffold the removed file.
unlink("$sut/index.php");
$stdout = $this->mustExec("composer scaffold --no-ansi", $sut);
$this->assertStringContainsString('- Copy [web-root]/index.php from assets/index.php', $stdout);
$this->assertStringNotContainsString('- Copy [web-root]/update.php from assets/update.php', $stdout);
}
}

View File

@ -31,14 +31,9 @@ class AppendOpTest extends TestCase {
$prepend = $fixtures->sourcePath('drupal-drupal-test-append', 'prepend-to-robots.txt');
$append = $fixtures->sourcePath('drupal-drupal-test-append', 'append-to-robots.txt');
$sut = new AppendOp($prepend, $append);
$sut = new AppendOp($prepend, $append, TRUE);
$sut->scaffoldAtNewLocation($destination);
// Test the system under test.
$sut->process($destination, $fixtures->io(), $options);
// Assert that the target file was created.
$this->assertFileExists($destination->fullPath());
// Assert the target contained the contents from the correct scaffold files.
$contents = trim(file_get_contents($destination->fullPath()));
$expected = <<<EOT
# robots.txt fixture scaffolded from "file-mappings" in drupal-drupal-test-append composer.json fixture.
# This content is prepended to the top of the existing robots.txt fixture.
@ -50,6 +45,16 @@ class AppendOpTest extends TestCase {
# This content is appended to the bottom of the existing robots.txt fixture.
# robots.txt fixture scaffolded from "file-mappings" in drupal-drupal-test-append composer.json fixture.
EOT;
$pre_calculated_contents = $sut->contents();
$this->assertEquals(trim($expected), trim($pre_calculated_contents));
// Test the system under test.
$sut->process($destination, $fixtures->io(), $options);
// Assert that the target file was created.
$this->assertFileExists($destination->fullPath());
// Assert the target contained the contents from the correct scaffold files.
$contents = trim(file_get_contents($destination->fullPath()));
$this->assertEquals(trim($expected), $contents);
// Confirm that expected output was written to our io fixture.
$output = $fixtures->getOutput();

View File

@ -4,7 +4,7 @@ namespace Drupal\Tests\Composer\Plugin\Scaffold\Integration;
use PHPUnit\Framework\TestCase;
use Drupal\Tests\Composer\Plugin\Scaffold\Fixtures;
use Drupal\Composer\Plugin\Scaffold\Operations\ConjunctionOp;
use Drupal\Composer\Plugin\Scaffold\Operations\AppendOp;
use Drupal\Composer\Plugin\Scaffold\Operations\SkipOp;
use Drupal\Composer\Plugin\Scaffold\Operations\ScaffoldFileCollection;
@ -41,8 +41,8 @@ class ScaffoldFileCollectionTest extends TestCase {
// Confirm that the keys of the output are the same as the keys of the
// input.
$this->assertEquals(array_keys($scaffold_file_fixtures), array_keys($resolved_file_mappings));
// '[web-root]/robots.txt' is now a SkipOp, as it is now part of a
// conjunction operation.
// '[web-root]/robots.txt' is now a SkipOp, as it is now part of an
// append operation.
$this->assertEquals([
'[web-root]/index.php',
'[web-root]/.htaccess',
@ -62,8 +62,8 @@ class ScaffoldFileCollectionTest extends TestCase {
// Test that .htaccess is skipped.
$this->assertInstanceOf(SkipOp::class, $resolved_file_mappings['fixtures/drupal-assets-fixture']['[web-root]/.htaccess']->op());
// Test that the expected conjunction operation exists.
$this->assertInstanceOf(ConjunctionOp::class, $resolved_file_mappings['fixtures/drupal-drupal']['[web-root]/robots.txt']->op());
// Test that the expected append operation exists.
$this->assertInstanceOf(AppendOp::class, $resolved_file_mappings['fixtures/drupal-drupal']['[web-root]/robots.txt']->op());
}
}