php-tuf / composer-stager Goto Github PK

We need to define a governance policy the project. All that has been certainly determined thus far is that it will fall under Drupal core governance.

• Define the policy.
• #299
• Further document it as appropriate, e.g., in README.md or the Wiki.

@tedbow our ActiveDirIsWritable and StagingDirIsWritable preconditions currently only test that the top level directories themselves are writable. Non-writable children and descendants therefore would not be caught until they actually cause failures during file syncing, potentially corrupting the codebase. I think we should make the checks recursive, even though that will make them slower. What do you think?

cc @phenaproxima @wimleers

Consider the following code:

$this->t(
    'The source and destination files cannot be the same at %path',
    $this->p(['%path' => '/some/path']),
)

When Drupal's translator is used, then when this message is displayed in the Drupal admin UI to an administrator whose preferred language is English, the message will be:

The source and destination files cannot be the same at <em class="placeholder">/some/path</em>

In other words, since Drupal is a web CMS, Drupal's translator returns HTML, with placeholder values formatted with <em> tags.

Currently, Composer Stager's Translator just inserts placeholder values with no formatting. In other words, it outputs:

The source and destination files cannot be the same at /some/path

Composer Stager should not assume an HTML display of its messages, so it should not follow Drupal's example of wrapping placeholder values with <em> tags, but... perhaps it should wrap them with quotes, so as to output:

The source and destination files cannot be the same at "/some/path"

That would mean prior to Infrastructure\Service\Translation\Translator calling return $this->symfonyTranslatorProxy->trans(...), it would modify $parameters to add quotes around all of them. More importantly, it would mean Composer Stager's Translator having some opinionated implementation, and not just be an un-opinionated wrapper/proxy around Symfony\Contracts\Translation\TranslatorTrait.

Is it appropriate and beneficial for Composer Stager's Translator to add the opinionated decision of adding quotes around placeholder values? Personally, I think yes, and that it would both improve the corresponding exception messages, and clarify that the scope of a translator includes not only translating to the desired language but also deciding how to format placeholder values.

Begin by familiarizing yourself with https://github.com/php-tuf/composer-stager/wiki.

Further details TBD.

• Part of #78.

Dependency tree

$ composer info --tree php-parallel-lint/php-parallel-lint
php-parallel-lint/php-parallel-lint v1.3.2 This tool check syntax of PHP files about 20x faster than serial check.
|--ext-json *
`--php >=5.3.0

Hi,

First of all, super exciting project and I already like the design and code quality in it. I would like to promote this as learning material for others.
Although, I have some ideas how to further improve it, how to adapt Onion/Hexagonal architecture deeper, like:

• introducing new value objects (e.g.: for paths, those should be clearly local references only if I am not mistaken, but currently they are just strings)
• decoupling from the Symfony Command library too, like the code is already decoupled from Filesystem, and supporting other presentation layers (like other CLI libs)
• narrowing down the public API of this library, like FileCopierInterface is the generic API that could be extended, RsyncFileCopierInterface and SymfonyFileCopierInterface are empty currently and they are infrastructure specific, so they are probably unnecessary.
• ...

What do you think? Are you open to PR-s with changes like these and having discussions about them?

Resolved by 27b94da.

• Part of #78.

Dependency tree

$ composer info --tree infection/infection
infection/infection 0.27.7 Infection is a Mutation Testing framework for PHP. The mutation adequacy score can be used to measure the effectiveness of a test set in terms of its ability to detect faults.
|--colinodell/json5 ^2.2
|  |--ext-json *
|  |--ext-mbstring *
|  |  `--php >=7.1
|  `--php ^7.1.3|^8.0
|--composer-runtime-api ^2.0
|--composer/xdebug-handler ^2.0 || ^3.0
|  |--composer/pcre ^1 || ^2 || ^3
|  |  `--php ^7.4 || ^8.0
|  |--php ^7.2.5 || ^8.0
|  `--psr/log ^1 || ^2 || ^3
|     `--php >=8.0.0
|--ext-dom *
|--ext-json *
|--ext-libxml *
|--ext-mbstring *
|  `--php >=7.1
|--fidry/cpu-core-counter ^0.4.0 || ^0.5.0 || ^1.0
|  `--php ^7.2 || ^8.0
|--infection/abstract-testframework-adapter ^0.5.0
|  `--php ^7.4 || ^8.0
|--infection/extension-installer ^0.1.0
|  `--composer-plugin-api ^1.1 || ^2.0
|--infection/include-interceptor ^0.2.5
|--justinrainbow/json-schema ^5.2.10
|  `--php >=5.3.3
|--nikic/php-parser ^4.15.1
|  |--ext-tokenizer *
|  `--php >=7.0
|--ondram/ci-detector ^4.1.0
|  `--php ^7.1 || ^8.0
|--php ^8.1
|--sanmai/later ^0.1.1
|  `--php >=7.4
|--sanmai/pipeline ^5.1 || ^6
|  `--php ^7.4 || ^8.0
|--sebastian/diff ^3.0.2 || ^4.0 || ^5.0
|  `--php >=7.3
|--symfony/console ^5.4 || ^6.0 || ^7.0
|  |--php >=8.1
|  |--symfony/deprecation-contracts ^2.5|^3
|  |  `--php >=8.1
|  |--symfony/polyfill-mbstring ~1.0
|  |  `--php >=7.1
|  |--symfony/service-contracts ^2.5|^3
|  |  |--php >=8.1
|  |  `--psr/container ^2.0
|  |     `--php >=7.4.0
|  `--symfony/string ^5.4|^6.0
|     |--php >=8.1
|     |--symfony/polyfill-ctype ~1.8
|     |  `--php >=7.1
|     |--symfony/polyfill-intl-grapheme ~1.0
|     |  `--php >=7.1
|     |--symfony/polyfill-intl-normalizer ~1.0
|     |  `--php >=7.1
|     `--symfony/polyfill-mbstring ~1.0
|        `--php >=7.1
|--symfony/filesystem ^5.4 || ^6.0 || ^7.0
|  |--php >=8.1
|  |--symfony/polyfill-ctype ~1.8
|  |  `--php >=7.1
|  `--symfony/polyfill-mbstring ~1.8
|     `--php >=7.1
|--symfony/finder ^5.4 || ^6.0 || ^7.0
|  `--php >=8.1
|--symfony/process ^5.4 || ^6.0 || ^7.0
|  `--php >=8.1
|--thecodingmachine/safe ^2.1.2
|  `--php ^8.0
`--webmozart/assert ^1.11
   |--ext-ctype *
   |  `--php >=7.1
   `--php ^7.2 || ^8.0

In several places like \PhpTuf\ComposerStager\Internal\FileSyncer\Service\PhpFileSyncer::sync set_time_limit() is called. But this might be disabled on some hosting. On drupal/automatic_updates we want to make sure we will work in this type of hosting. So I was trying to make build test in https://www.drupal.org/project/automatic_updates/issues/3381294#comment-15193112 that disables this function before it starts up the server and sets max_execution_time low.

Could composer-stager check if set_time_limit is available before calling it. Thanks

• Part of #78.

Dependency tree

$ composer info --tree phpmd/phpmd
phpmd/phpmd 2.13.0 PHPMD is a spin-off project of PHP Depend and aims to be a PHP equivalent of the well known Java tool PMD.
├──composer/xdebug-handler ^1.0 || ^2.0 || ^3.0
│  ├──composer/pcre ^1 || ^2 || ^3
│  │  └──php ^7.4 || ^8.0
│  ├──php ^7.2.5 || ^8.0
│  └──psr/log ^1 || ^2 || ^3
│     └──php >=5.3.0
├──ext-xml *
├──pdepend/pdepend ^2.12.1
│  ├──php >=5.3.7
│  ├──symfony/config ^2.3.0|^3|^4|^5|^6.0
│  │  ├──php >=7.2.5
│  │  ├──symfony/deprecation-contracts ^2.1|^3
│  │  │  └──php >=7.1
│  │  ├──symfony/filesystem ^4.4|^5.0|^6.0
│  │  │  ├──php >=7.2.5
│  │  │  ├──symfony/polyfill-ctype ~1.8
│  │  │  │  └──php >=7.1
│  │  │  ├──symfony/polyfill-mbstring ~1.8
│  │  │  │  └──php >=7.1
│  │  │  └──symfony/polyfill-php80 ^1.16
│  │  │     └──php >=7.1
│  │  ├──symfony/polyfill-ctype ~1.8
│  │  │  └──php >=7.1
│  │  ├──symfony/polyfill-php80 ^1.16
│  │  │  └──php >=7.1
│  │  └──symfony/polyfill-php81 ^1.22
│  │     └──php >=7.1
│  ├──symfony/dependency-injection ^2.3.0|^3|^4|^5|^6.0
│  │  ├──php >=7.2.5
│  │  ├──psr/container ^1.1.1
│  │  │  └──php >=7.4.0
│  │  ├──symfony/deprecation-contracts ^2.1|^3
│  │  │  └──php >=7.1
│  │  ├──symfony/polyfill-php80 ^1.16
│  │  │  └──php >=7.1
│  │  ├──symfony/polyfill-php81 ^1.22
│  │  │  └──php >=7.1
│  │  └──symfony/service-contracts ^1.1.6|^2
│  │     ├──php >=7.2.5
│  │     ├──psr/container ^1.1
│  │     │  └──php >=7.4.0
│  │     └──symfony/deprecation-contracts ^2.1|^3
│  │        └──php >=7.1
│  └──symfony/filesystem ^2.3.0|^3|^4|^5|^6.0
│     ├──php >=7.2.5
│     ├──symfony/polyfill-ctype ~1.8
│     │  └──php >=7.1
│     ├──symfony/polyfill-mbstring ~1.8
│     │  └──php >=7.1
│     └──symfony/polyfill-php80 ^1.16
│        └──php >=7.1
└──php >=5.3.9

• Part of #78.

Dependency tree

$ composer info --tree slevomat/coding-standard
slevomat/coding-standard 8.8.0 Slevomat Coding Standard for PHP_CodeSniffer complements Consistence Coding Standard by providing sniffs with additional checks.
|--dealerdirect/phpcodesniffer-composer-installer ^0.6.2 || ^0.7 || ^1.0
|  |--composer-plugin-api ^1.0 || ^2.0
|  |--php >=5.4
|  `--squizlabs/php_codesniffer ^2.0 || ^3.1.0 || ^4.0
|     |--ext-simplexml *
|     |--ext-tokenizer *
|     |--ext-xmlwriter *
|     `--php >=5.4.0
|--php ^7.2 || ^8.0
|--phpstan/phpdoc-parser >=1.15.2 <1.16.0
|  `--php ^7.2 || ^8.0
`--squizlabs/php_codesniffer ^3.7.1
   |--ext-simplexml *
   |--ext-tokenizer *
   |--ext-xmlwriter *
   `--php >=5.4.0

Should we remove Composer Stager's dependency on symfony/filesystem? Keeping it would result in a new production dependency for Drupal core. We could replace it with custom code. Or we could keep it and replace our custom path-handling code with functionality added in the latest versions of the Symfony library. This is the trade-off, in short:

• Part of #78.

Dependency tree

$ composer info --tree rector/rector
rector/rector 0.18.6 Instant Upgrade and Automated Refactoring of any PHP code
├──php ^7.2|^8.0
└──phpstan/phpstan ^1.10.35
   └──php ^7.2|^8.0

• Part of #79.

Create an issue template or similar as a "backdrop" to prevent public disclosure of security issues, similar to Drupal's alerts on its "Create issue" pages, e.g., https://www.drupal.org/node/add/project-issue/drupal:

Security issues should not be reported here. Follow the procedure for reporting security issues.

It is currently impossible to use Composer Stager without depending on the Internal layer in some way. Eliminate this requirement.

• #268
• #261

Related: Eliminate dependencies on Composer Stager internals [#3382942] | Drupal.org

• Part of #78.

Dependency tree

$ composer info --tree thecodingmachine/phpstan-strict-rules
thecodingmachine/phpstan-strict-rules v1.0.0 A set of additional rules for PHPStan based on best practices followed at TheCodingMachine
|--php ^7.1|^8.0
`--phpstan/phpstan ^1.0
   `--php ^7.2|^8.0

Discovered here https://www.drupal.org/project/automatic_updates/issues/3362143#comment-15083985

but will copy relevant comment:

I traced into this, and what I found was straightforward: when the stage directory is created, sites/default (the directory) has different permissions depending on whether the file syncer is rsync, or the PHP syncer.

In the active directory, the permissions are 644. That's because Drupal actively tries to write-protect the directory (it does this at install time, and in system_requirements()) for security hardening. Makes sense.

Now, if you're using the rsync file syncer, those permissions are preserved when the stage directory is created. That's because Composer Stager calls rsync with the --archive option, which implies the --perms option, which preserves permissions. Because of that, the scaffold files cannot be updated in sites/default, and the update fails due to ScaffoldFilePermissionsValidator. Makes sense.

The PHP file syncer, as it turns out, is less reliable. It delegates to Symfony's Filesystem component (specifically the \Symfony\Component\Filesystem\Filesystem::copy() method), which does try to preserve permissions on copied files...but it does not preserve permissions on directories. When creating directories, it does this:

$this->mkdir(\dirname($targetFile));

\Symfony\Component\Filesystem\Filesystem::mkdir() takes a second argument -- the permissions, which default to 777. But copy() doesn't pass that argument, so the directory is created with 777 permissions.

This causes our build tests, which use the PHP file syncer by default, to pass by coincidence. As it turns out, we do try to ensure that we're handling permissions of sites/default properly -- if you look at \Drupal\Tests\automatic_updates\Build\CoreUpdateTest::createTestProject(), this is the last bit of the method:

// Ensure that Drupal has write-protected the site directory.
$this->assertDirectoryIsNotWritable($this->getWebRoot() . '/sites/default');

What we didn't realize, though, is that the PHP file syncer is disregarding those permissions, and creating the staged copy of sites/default with world-writable permissions. So this test is wrongfully, but understandably, assuming that the permissions from the active sites/default directory are reflected in the staged sites/default.

Exceptions don't currently follow any particular rule for their exception codes. The default values are used for new exceptions, and wrapping exceptions just use the code of the exception they wrap. As such, they provide no real value, whereas they could be useful in cases such as Handle "Composer not found" error better in ComposerExecutableValidator [#3321966] | Drupal.org, not to mention automated tests.

Settle on an exception code scheme and implement it. Codes should be defined as public constants on exception classes or interfaces, or maybe as enums in PHP 8.

Currently all symlinks are forbidden. But some kinds are essentially safe and can be permitted, as discussed at length here: https://www.drupal.org/project/automatic_updates/issues/3319507

This issue will allow symlinks according to the following rules:

• Absolute links are disallowed. - An absolute link in the active directory will point to the same path on disk when copied to the staging directory, creating the possibility of staged operations changing production assets.
• Hard links are disallowed. - Hard links point to inodes as opposed to mere paths, i.e., the actual data blocks on the disk. Therefore, a hard link to a file essentially is that file. So anything done to one is done to the other, with the same net effect as absolute links.
• No symlinks of any kind are allowed on Windows. - Most Linux flavors will let you create a symlink pointing to a path that does not exist, but Windows will fail. This creates a temporal coupling that cannot be guaranteed. Given a symlink link.txt with a target target.txt, we would have to make sure that target.txt was always written first (which is infeasible) or the operation would fail, leaving the codebase in some state of corruption.
• No links that point outside the codebase root are allowed. - Active and staging directories adjacent to one another on the filesystem have identical ancestors by definition. Hence, a relative link pointing out of one would target the same file as said link when copied "next door" and, like previous scenarios, create the possibility of corrupting accidentally shared assets.

All other symlinks are allowed.

While working on https://www.drupal.org/project/automatic_updates/issues/3319507, I created a testing layout looking like this:

.
├── composer.json
├── composer.lock
├── custom
│   └── MYMODULE
│       ├── .git
│       ├── MYMODULE.info.yml
│       └── node_modules
├── index.php
├── modules
│   └── MYMODULE -> ../custom/MYMODULE
└── vendor

Composer Stager choked on the modules/MYMODULE symlink with this error from Symfony's Filesystem component:

Failed to copy "/private/tmp/package_manager_testing_roottest18634147/active/custom/example" because file does not exist

When I told @TravisCarden about this, he said he doesn't have test coverage for symlinks to directories in Composer Stager. Since we're likely to see this kind of thing in the wild, we should probably at least get test coverage for that, and hopefully find (and bust!) this bug.

• Part of #78.

Dependency tree

$ composer info --tree ergebnis/composer-normalize
ergebnis/composer-normalize 2.30.2 Provides a composer plugin for normalizing composer.json.
|--composer-plugin-api ^2.0.0
|--ergebnis/json ^1.0.1
|  `--php ^8.0
|--ergebnis/json-normalizer ^4.0.2
|  |--ergebnis/json ^1.0.1
|  |  `--php ^8.0
|  |--ergebnis/json-pointer ^3.2.0
|  |  `--php ^8.0
|  |--ergebnis/json-printer ^3.3.0
|  |  |--ext-json *
|  |  |--ext-mbstring *
|  |  |  `--php >=7.1
|  |  `--php ^8.0
|  |--ergebnis/json-schema-validator ^4.0.0
|  |  |--ergebnis/json ^1.0.0
|  |  |  `--php ^8.0
|  |  |--ergebnis/json-pointer ^3.2.0
|  |  |  `--php ^8.0
|  |  |--ext-json *
|  |  |--justinrainbow/json-schema ^5.2.12
|  |  |  `--php >=5.3.3
|  |  `--php ^8.0
|  |--ext-json *
|  |--justinrainbow/json-schema ^5.2.12
|  |  `--php >=5.3.3
|  `--php ~8.0.0 || ~8.1.0 || ~8.2.0
|--ergebnis/json-printer ^3.3.0
|  |--ext-json *
|  |--ext-mbstring *
|  |  `--php >=7.1
|  `--php ^8.0
|--ext-json *
|--justinrainbow/json-schema ^5.2.12
|  `--php >=5.3.3
|--localheinz/diff ^1.1.1
|  `--php ^7.1 || ^8.0
`--php ~8.0.0 || ~8.1.0 || ~8.2.0

The current behavior of TranslatableInterface::__toString() conflicts with Drupal:

This requirement of TranslatableInterface::__toString() is incompatible with Drupal's TranslatableMarkup, because Drupal's TranslatableMarkup::__toString() returns the translated string (it basically does the same as trans()).

Can we change this Composer Stager interface so that instead of defining __toString() we define a different method (e.g., noTrans()) to return the string in its original language? This can be deferred to a followup issue instead of blocking an initial merge of this PR.

Originally posted by @effulgentsia in #141 (comment)

A solution is not yet specified. Consider alternatives.

Infrastructure\Value\PathList\PathList::add() (and, by extension, ::__construct() takes an arbitrary array, which requires a whole ::assertValidInput() method to validate. Make it variadic (i.e., ::add(string ...$paths) to eliminate the need for ad hoc validation. c.f. Creating Strictly Typed Arrays and Collections in PHP — SitePoint.

I would like to make this change for v2.x and then adopt the pattern as a standard for any API additions going forward.

@phenaproxima I believe this would require a small change to the Automatic Updates module, i.e., changing instances of ::add($paths) to ::add(...$paths). Does that bother you?

• Part of #78.

Dependency tree

$ composer info --tree psalm/phar
psalm/phar 5.6.0 Composer-based Psalm Phar
└──php ^7.1 || ^8.0

The two known offenders at the time of this writing are these:

• Infrastructure\Service\Filesystem\Filesystem::remove()
• Infrastructure\Service\FileSyncer\PhpFileSyncer::sync()

I was looking through the dev tooling and I was interested in the use of mutation testing. However looking at the automated testing it doesn't look like infection is running as part of the automated testing. Am I missing something? Or should we either make it run or remove it as a dev dependency.

This is what happens when you roll a natural 1 on arcana (D&D nerds, rejoice).

Let's say we have a directory tree like this:

src/
tests/
  src/

If we want to exclude the top-level src directory from a Composer Stager operation, we would pass src as an excluded path. This is in accordance with the documentation of the file syncer interface, which states that exclusions should be passed as relative paths.

However, the rsync file syncer will pass that to rsync as --exclude=src. Which, it turns out, will cause ALL the src directories in the tree to be excluded! It seems that --exclude follows (essentially) the same rules as .gitignore. In a gitignore file, if you wanted to exclude the top-level src directory, you'd refer to it as /src. Indeed, if you pass --exclude=/src to rsync, it behaves correctly.

So I'm thinking that the rsync file syncer should probably always prefix excluded paths with /, so as to make it behave as the interface documentation says it will.

But, you're asking, what if you pass a fully absolute path? My guess -- which I have not tested -- is that it treats it as relative to the source directory. So, if you pass /path/to/my/stuff, and your source directory happens to contain a path like path/to/my/stuff, it'll be excluded. But if you pass an absolute path that does NOT map neatly into the source directory tree like that, I'm assuming it'll just be ignored, which is totally fine.

If I open this repository in PHPStorm immediately after cloning I don't see any folders - i.e. src/ which makes the project unusable.

https://github.com/php-tuf/composer-stager/blob/develop/src/Infrastructure/Process/FileCopier.php#L32

If a new set of composer files is syncs to a new location old files are not deleted. This will mean the resulting files will be polluted with files no longer needed.

Follow-up to #60.

Now that we depend on Symfony Filesystem ^6.2 we have access to the new path manipulation utilities. Much of our custom path-handling should be replaceable with it.

This can be deferred to https://github.com/php-tuf/composer-stager/milestone/2 if necessary, but if so, double check that sure our current implementation is sufficiently insulated to allow for such a change without breaking BC.

NoUnsupportedLinksExist injects its inner preconditions in this order:

    NoAbsoluteLinksExistInterface
    NoHardLinksExistInterface
    NoLinksExistOnWindowsInterface
    NoLinksPointOutsideTheCodebaseInterface

This is a problem because, if any hard links exist, NoAbsoluteLinksExist will fail outright with an exception coming from the Filesystem service, because it will enter readLink() and die because it finds a thing that's not a symlink: https://github.com/php-tuf/composer-stager/blob/feature/symlink-support/src/Infrastructure/Service/Filesystem/Filesystem.php#L143

So, this needs to be fixed. The easiest(?) solution is to move NoHardLinksExistInterface to the first position of NoUnsupportedLinksExist::__construct().

With the addition of all the new preconditions in #58, we'll be scanning the filesystem a lot. But we don't know what the performance impact will be, especially on large codebases, because we haven't done any testing.

Proposal

I think we need do some initial benchmarking and create a CI job that lets us watch the numbers over time. Depending on what we find, we can determine whether we need to optimize.

Follow-up

For posterity, here are some ideas for optimization if our testing shows we need it:

• #75 so cheaper preconditions can be evaluated first.
• Add Unix find-based file finders (with a fallback strategy like we did with rsync).
• Cache filesystem scan results for reuse between preconditions within the same PHP process.
• Cache whole precondition check results beyond single PHP processes, with cache expiration/invalidation.
• Run precondition checks concurrently, i.e., multithreaded.

Resolved by 998fae5.

• Part of #78.

Dependency tree

$ composer info --tree phpro/grumphp-shim
phpro/grumphp-shim v1.15.0 GrumPHP Phar distribution
├──composer-plugin-api ~2.0
├──ext-json *
└──php ^8.0

Example use

$ composer all
> grumphp run
GrumPHP is sniffing your code!

Running tasks with priority 0!
==============================

Running task 1/11: composer... ✔
Running task 2/11: composer_normalize... ✔
Running task 3/11: deptrac... ✔
Running task 4/11: phpcs... ✔
Running task 5/11: phplint... ✔
Running task 6/11: phpmd... ✔
Running task 7/11: phpstan... ✔
Running task 8/11: phpunit... ✔
Running task 9/11: psalm... ✔
Running task 10/11: xmllint... ✔
Running task 11/11: yamllint... ✔

Running tasks with priority -100!
=================================

Running task 1/1: clover_coverage... ✔

# time: 32.25s user 12.16s system 96% cpu 45.807 total

I can run that dozens of times per day with basically no break in my flow. I would be very sad to lose it and the associated productivity.

It is currently possible to change preconditions at design time, but it requires overriding service definitions, which is neither obvious nor easy. This will add the following facilities for changing them at runtime:

• PreconditionsAwareTrait->addPrecondition(PreconditionInterface $precondition): void;
• PreconditionsAwareTrait->getPreconditions(): array;
• PreconditionsAwareTrait->setPreconditions(?PreconditionInterface $preconditions = null): void;
• PreconditionsAwareTrait->hasPrecondition(string $precondition): void;
• PreconditionsAwareTrait->removePrecondition(string $precondition): void;

• Part of #78.

Dependency tree

$ composer info --tree qossmic/deptrac-shim
qossmic/deptrac-shim 0.24.0 deptrac phar distribution
├──ext-json *
├──ext-tokenizer *
├──ext-zlib *
└──php ^7.4 || 8.0.* || 8.1.*

Setting up the problem

Used https://github.com/php-tuf/composer-stager-console to make sure this not something specific to how we are using Compose Stager in Drupal.

I changed \PhpTuf\ComposerStager\Infrastructure\Factory\FileSyncer\FileSyncerFactory::create to either return $this->rsyncFileSyncer; or return $this->phpFileSyncer; to determine if the problem happens with both copiers. It only happens with phpFileSyncer. With rsyncFileSyncer the commit succeeds and I can see an updated file.

composer create-project drupal/recommended-project:10.0.8 cs-console-test
cd cs-console-test 
composer config minimum-stability dev
composer require drupal/automatic_updates:dev-3.0.x # this brings in Composer package that will have git repo

Then use the composer-stager-console

bin/composer-stage begin  --active-dir=/Users/ted.bowman/sites/cs-console-test  
bin/composer-stage commit --active-dir=/Users/ted.bowman/sites/cs-console-test

I get the error

Failed to copy "/Users/ted.bowman/projects/composer-stager-console/.composer_staging/web/modules/contrib/automatic_updates/.git/objects/pack/pack-4803e1c5c08f0b2573ae9399d594c5a57d268af0.idx" to "/Users/ted.bowman/sites/cs-console-test/web/modules/contrib/automatic_updates/.git/objects/pack/pack-4803e1c5c08f0b2573ae9399d594c5a57d268af0.idx"

I have seen the same error when trying to use package manager.

I was using composer-stager v2.0.0-alpha1

As surfaced in #118, Drupal requires strings that will appear in user interfaces to be translatable for multilingual support. But it depends on them being defined using its Localization API, which obviously, Composer Stager knows nothing about. (Nor would it matter, as far as I know, since the code still wouldn't be in the Drupal codebase for discovery.) We need to solve this problem for dependent projects in a way that accounts for the complexity of substitutions, for example.

Background

@larowlan, #118 - How would downstream users of this go about translating the precondition names/descriptions/unfulfilled/fulfilled strings. For use with e.g. Drupal this would be a requirement I'd think

@TravisCarden #118 (comment) - This is the hard one, isn't it? I'm glad you bring it up. We discussed it early on, but there wasn't consensus on whether it was actually an issue. I seems to me that it would be a requirement. But as you imply, there are obvious difficulties, and I don't have an easy answer. A few possibilities come readily to my mind:

1. Composer Stager could provide very specific exception codes (c.f. #71), and Drupal could declare its own translatable strings based on them instead of just taking whatever it gets back. The major drawback to this, of course, is that details will be lost, such as error messages that state what went wrong and specify a file it occurred in.

2. We could add the ability to pass some sort of translation callback into Composer Stager API calls.

3. Instead of returning strings, we could have Composer Stager return some sort of object that provides placeholder substitution similar to Drupal's TranslatableMarkup.

Related issue: Support scanning Composer Stager library for translatable strings [#3354011] with the Translation template extractor (potx).

Note: Adding this to https://github.com/php-tuf/composer-stager/milestone/1 as a requirement for a stable release.

• Part of #78.

Dependency tree

$ composer info --tree symfony/config
symfony/config v6.2.7 Helps you find, load, combine, autofill and validate configuration values of any kind
|--php >=8.1
|--symfony/deprecation-contracts ^2.1|^3
|  `--php >=8.1
|--symfony/filesystem ^5.4|^6.0
|  |--php >=8.1
|  |--symfony/polyfill-ctype ~1.8
|  |  `--php >=7.1
|  `--symfony/polyfill-mbstring ~1.8
|     `--php >=7.1
`--symfony/polyfill-ctype ~1.8
   `--php >=7.1

It is currently necessary for a client's autoloader configuration (services.yml) to know about Composer Stager's Internal layer. See https://github.com/php-tuf/composer-stager-console/blob/7bd5363f8c920886e855a48106163ac8afdedc14/config/services.yml#L22-L29, for example:

services:
    PhpTuf\ComposerStager\API\FileSyncer\Service\FileSyncerInterface:
        factory: [ '@PhpTuf\ComposerStager\Internal\FileSyncer\Factory\FileSyncerFactory', 'create' ]
    PhpTuf\ComposerStager\Internal\FileSyncer\Factory\FileSyncerFactory:
        arguments:
            $phpFileSyncer: '@PhpTuf\ComposerStager\Internal\FileSyncer\Service\PhpFileSyncer'
            $rsyncFileSyncer: '@PhpTuf\ComposerStager\Internal\FileSyncer\Service\RsyncFileSyncer'
    PhpTuf\ComposerStager\Internal\FileSyncer\Service\PhpFileSyncer: ~
    PhpTuf\ComposerStager\Internal\FileSyncer\Service\RsyncFileSyncer: ~

Without these, Symfony explodes:

Fatal error: Uncaught Symfony\Component\DependencyInjection\Exception\RuntimeException: Cannot autowire service
"PhpTuf\ComposerStager\Internal\Core\Beginner": argument "$fileSyncer" of method "__construct()" references interface
"PhpTuf\ComposerStager\API\FileSyncer\Service\FileSyncerInterface" but no such service exists. You should maybe alias
this interface to one of these existing services: "PhpTuf\ComposerStager\Internal\FileSyncer\Service\PhpFileSyncer",
"PhpTuf\ComposerStager\Internal\FileSyncer\Service\RsyncFileSyncer". in
/var/www/composer-stager-console/vendor/symfony/dependency-injection/Compiler/DefinitionErrorExceptionPass.php:49

This dependency needs to be eliminated or the Internal layer isn't truly internal, i.e., changes to it require clients to make corresponding change.

@phenaproxima, you said you might be able to help with this.

Note to self: Remember to re-enable the corresponding new PHPStan rule to prevent regressions:

• Part of #78.

Dependency tree

$ composer info --tree phpstan/phpstan-deprecation-rules
phpstan/phpstan-deprecation-rules 1.1.4 PHPStan rules for detecting usage of deprecated classes, methods, properties, constants and traits.
├──php ^7.2 || ^8.0
└──phpstan/phpstan ^1.10.3
   └──php ^7.2|^8.0

See Add php-tuf/composer-stager to core dependencies and governance — for experimental Automatic Updates & Project Browser modules [#3331078] | Drupal.org

Pursuant to a release candidate (RC), Composer Stager needs to go through the Drupal core review process for addition as a production dependency:

• #282
• #79
• #300

Additional tasks TBD.

• Part of #78.

Dependency tree

$ composer info --tree phpstan/phpstan-strict-rules
phpstan/phpstan-strict-rules 1.5.0 Extra strict and opinionated rules for PHPStan
|--php ^7.2 || ^8.0
`--phpstan/phpstan ^1.10
   `--php ^7.2|^8.0

figure out here https://www.drupal.org/project/automatic_updates/issues/3363937

What our case was

1. we staged an operation
2. We put a file in the active directory
3. we called commit() with an excluded absolute path of the file from 2). The purpose of this is we don't want the file from 2) deleted on commit()

Result
PHP file syncer: this file was not deleted on commit
Rsync file syncer: this file was deleted on commit

We solved this by making the path relative but it seems like they should act the same way.

I chatted with @TravisCarden about this and the absolute path should only be used if it is in the source(stage in this case) directory.

Some preconditions are more fundamental or less expensive than others, making it desirable to evaluate them first. Add a system of weighting to control the order they get evaluated in.

Evaluate dev Composer dependencies according to the Core dependency release cycles, security information, and evaluation criteria for Drupal. This only includes dependencies that are not already a part of drupal/core, drupal/core-dev, or drupal/coder.

• #81
• #80
• #297
• #88
• #84
• #296
• #77 (kept)
• #86 (removed)
• #82 (removed)
• #87 (removed)
• #89 (removed)
• #90 (removed)
• #83 (removed)

PHPUnit and PHPBench are taken for granted as obvious and indispensable, respectively.

The current list of dev dependencies (that are not already a part of drupal/core, drupal/core-dev, or drupal/coder):

1. infection/infection
2. phpbench/phpbench
3. phpro/grumphp-shim
4. phpspec/prophecy
5. phpstan/phpstan-strict-rules
6. rector/rector
7. symfony/config
8. thecodingmachine/phpstan-strict-rules

c.f. https://github.com/php-tuf/composer-stager/wiki/Coding-standards-&-style-guide

According to the Core dependency release cycles, security information, and evaluation criteria, dependencies are evaluated for their security policies before being added to Drupal core. To facilitate adding this library (see Add php-tuf/composer-stager to core dependencies on Drupal.org), we need to define a security policy.

• Define the policy.
• #85
• #298
• Further document it as appropriate, e.g., in README.md or the Wiki.