Skip to content

Conversation

@emembeka
Copy link

Symfony\Component\Console\Command\Command

Fixing the signature and returning Command's constant instead of literal number.

…ommand\Command

Return Command's constant instead of litaral number.
@colemanw
Copy link
Member

Thanks @emembeka - have you confirmed that the constant is available in all versions of Symfony Console that we support?

@emembeka
Copy link
Author

emembeka commented Oct 13, 2025

The constants where introduced in symfony/console:5.1.3
But I can't really figure out which console versions are supported, the composer.json does not define the dependency.
Neither to symfony/console nor to composer/composer which contains the BaseCommand.
The actual composer/composer requires "symfony/console": "^5.4.47 || ^6.4.25 || ^7.1.10",

@colemanw
Copy link
Member

Ok this sounds mergeable then.
@totten

@demeritcowboy
Copy link
Contributor

It's interesting it hasn't come up in unit tests (e.g. https://github.com/colemanw/webform_civicrm/actions/runs/18466516067/job/52609698960#step:19:75) and I can't reproduce locally. Having said that, this does seem like the Right Thing To Do™ and it runs ok with the change, and also in drupal 10.

@emembeka
Copy link
Author

Doesn't it make sense to define a dependency to composer/composer in the composer.json ?

@totten
Copy link
Member

totten commented Nov 1, 2025

The constants where introduced in symfony/console:5.1.3

Aah, good to know.

Doesn't it make sense to define a dependency to composer/composer in the composer.json ?

No and yes? 🙃

  • The docs for Composer Plugin API only specify that you require a version of composer-plugin-api. (IIRC, this is a special/semi-magic constraint -- each build of composer.phar will decide whether it meets it.)

  • The docs do suggest using a require-dev for composer/composer. This allows you to setup IDE and phpunit with a special/local copy of composer -- and makes it easier to develop. (Our composer.json here does pull in composer/composer, but it's a very old version.)

The actual composer/composer requires "symfony/console": "^5.4.47 || ^6.4.25 || ^7.1.10",

Would that it were so. 🥲 That sounds like the constraint on the current-stable of composer (circa 2.8.12).

Each developer/site-operator runs their own version of composer. (Depends how often they do composer self-update... IME, a lot of people update their toolchain infrequently...) For purposes of this plugin, our only influence is through that composer-plugin-api constraint.

  • The docs claim: "The current Composer plugin API version is 2.6.0."
  • Currently, we say: "composer-plugin-api": "^1.1 || ^2.0" (Which is a pretty wide range)
  • It would make sense if the composer-plugin-api version exactly matched the composer.phar version. But clearly, 2.6.0 != 2.8.12. (I have no evidence for what the correlation actually is....)

The good news is that (IIRC) we don't need every version of civicrm/composer-compile-plugin to support every version of composer. (If you're on an older version of composer, then you can get an old version of the plugin.) We just need to make sure that our lower-bound is really compatible.

Here are a couple paths:

  1. Raise our lower-bound.
    • AFAICS, the first version of composer to ensure symfony/console with 5.1.3+ would be composer 2.3.0.
    • In composer.json, we need to set the require for composer-plugin-api to something that meets or exceed composer 2.3.0. The exact correlation is unclear, but.... surely the latest composer-plugin-api: ^2.6.0 is newer than composer 2.3.0?
    • In composer.json, we should also raise require-dev. (And... hopefully the test are happy with that...)
  2. Or... keep our current/broad constraints, but update this PR to use guarded reference. (defined(...) ? Command::SUCCESS : 0)

@totten
Copy link
Member

totten commented Nov 1, 2025

It would make sense if the composer-plugin-api version exactly matched the composer.phar version. But clearly, 2.6.0 != 2.8.12. (I have no evidence for what the correlation actually is....)

OK, it looks like composer-plugin-api version (PluginInterface::PLUGIN_API_VERSION) loosely correlates to composer.git version (aka composer.phar version), as in these examples:

  • composer.git@2.3.0 raised PLUGIN_API_VERSION=2.3.0
  • composer.git@2.4.0 preserved PLUGIN_API_VERSION=2.3.0. (Presumably, no change in the nominal interfaces.)
  • composer.git@2.5.0 preserved PLUGIN_API_VERSION=2.3.0. (Presumably, no change in the nominal interfaces.)
  • composer.git@2.6.0 raised PLUGIN_API_VERSION=2.6.0

So this makes it awkward to express constraints on other bundled/transitive dependencies (symfony/console), but it's sufficient for the current purpose. Requiring composer-plugin-api of ^2.3.0 (or ^2.6.0) should ensure that our update only deploys on configurations with symfony/console@5.4 or higher (which should do the job).

@totten
Copy link
Member

totten commented Nov 1, 2025

But I can't really figure out which console versions are supported, the composer.json does not define the dependency.

Forked off a discussion at composer/composer#12587. (IMHO, the plugin docs should probably speak more to this kind of situation...)

@totten
Copy link
Member

totten commented Nov 1, 2025

Merged by way of #31. One more time - thanks @emembeka for reporting and submitting a fix!

@totten totten closed this Nov 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants