Minor observations from review

[larowlan]

I did a review of this for possible future inclusion in Drupal core.

Here's some things I saw that were easier to fix than describe.

• Some grammar issues
• Argument mismatch in Readme (I think)
• Unneeded use of func_get_args()
• Alternate approach to Windows detection - although see below regarding this.

Here's some observations I have from a reviewing the code

• ExceptionInterface is a fairly generic name, Guzzle uses GuzzleException, so perhaps ComposerStagerException (without the Interface suffix) - but this is merely an observation, feel free to ignore
• 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
• There doesn't seem to be any ability for the sync operations to provide feedback on progress - e.g PhpFileSyncer doesn't use the $callback argument
• PhpFileSyncer and AbstractPath::normalize reference making use of new features in symfony/filesystem 5.4 'once symfony 4 support is no longer needed'. As far as I can tell, the minimum version is now symfony/filesystem:6 so is there more file-path handling code that can be deferred to the filesystem component now?
• Filesystem::remove doesn't seem to use the $callback argument, and is final, so if someone wanted to make use of that, how would they without re-implementing the whole class - should it do something with $callback?
• In my opinion Host::isWindows should be static, and the interface can be removed as well as any dependency injection. This could then be used in PathFactory too.

[tedbow]

re

How would a 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

Don't we get other errors from other 3 party libraries that show through to our UI that aren't translatable? Or is standard practice in most of the dependencies to translatable errors?

[larowlan]

Not when they show in the UI (that I know of). For exceptions, log messages - yes.

[TravisCarden]

Thanks for your review, @larowlan! 🙂️

Some grammar issues

1. Thanks.

Argument mismatch in Readme (I think)

2. Ooh, you're right! Good catch.

Unneeded use of func_get_args()

3. My rationale for that approach is that it eliminates duplication: a child precondition can be added or removed with a single line change. If we individually pass children to the parent constructor, such a behavioral change would require two changes. And it would be easy for a future maintainer to forget the second one--especially since it wouldn't cause a PHP error. Perhaps you find the boilerplate ugly, and I would agree. But I was able to minimize it in 777a886.

Alternate approach to Windows detection... In my opinion Host::isWindows() should be static, and the interface can be removed as well as any dependency injection. This could then be used in PathFactory too.

3. I agree. In fact, it's been on my to-do list for some time. I've addressed it in Make Domain\Service\Host\HostInterface::isWindows() static #119. I'm leaving the interface and continuing to inject it as a dependency, though, because it still needs to be override-able. For example, unit tests that exercise OS-dependent behavior need to simulate it.

ExceptionInterface is a fairly generic name, Guzzle uses GuzzleException, so perhaps ComposerStagerException (without the Interface suffix)

4. True. I took the pattern straight from Symfony, which uses it in all of its components. I think I'll leave it as-is--especially since I know the Automatic Updates module is already relying on it.

Filesystem::remove() and PhpFileSyncer::sync() don't seem to use the $callback argument... should they do something with $callback?

5. Quite so, and yes they should. I was just sure I had documented this in the Limitations and known issues wiki page, but I must just have it in my private notes. I've created Make sure every class that takes an output callback actually uses it #120 for it. I've also created Make sure every method that takes a timeout argument actually enforces it #121.

PhpFileSyncer and AbstractPath::normalize() reference making use of new features in symfony/filesystem 5.4... is there more file-path handling code that can be deferred to the filesystem component now?

6. There would be, but we're getting rid of symfony/filesystem in Eliminate dependency on Symfony Filesystem? #60.

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.

7. 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. Provide meaningful exception codes #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.

[larowlan]

One other approach for translation is to have a TranslationInterface or similar that gets passed around and you call $translator->translate($the_message) with a default pass through implementation, that way something downstream could provide its own. Could be a factory for generating it with e.g. a configurable class name.

[effulgentsia]

I suggest opening a separate issue in which to further discuss message translation.

[TravisCarden]

Let's do, @effulgentsia. I've created #123 accordingly.

Now we can finish discussing the smaller matters here.

[TravisCarden]

Thanks @larowlan! Merging.