Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VendorExposeCommand execute is missing a return type. #82

Closed
2 tasks done
nlighteneddesign opened this issue Sep 30, 2024 · 11 comments
Closed
2 tasks done

VendorExposeCommand execute is missing a return type. #82

nlighteneddesign opened this issue Sep 30, 2024 · 11 comments

Comments

@nlighteneddesign
Copy link

nlighteneddesign commented Sep 30, 2024

Module version(s) affected

2.0.3

Description

When the build is running in CI I get this error:

Fatal error: Declaration of SilverStripe\VendorPlugin\Console\VendorExposeCommand::execute(Symfony\Component\Console\Input\InputInterface $input, Symfony\Component\Console\Output\OutputInterface $output) must be compatible with Symfony\Component\Console\Command\Command::execute(Symfony\Component\Console\Input\InputInterface $input, Symfony\Component\Console\Output\OutputInterface $output): int in /app/vendor/silverstripe/vendor-plugin/src/Console/VendorExposeCommand.php on line 36

W: PHP Fatal error:  Declaration of SilverStripe\VendorPlugin\Console\VendorExposeCommand::execute(Symfony\Component\Console\Input\InputInterface $input, Symfony\Component\Console\Output\OutputInterface $output) must be compatible with Symfony\Component\Console\Command\Command::execute(Symfony\Component\Console\Input\InputInterface $input, Symfony\Component\Console\Output\OutputInterface $output): int in /app/vendor/silverstripe/vendor-plugin/src/Console/VendorExposeCommand.php on line 36

How to reproduce

Build in platform.sh
I'm not sure why this isn't the same as running it locally.

Possible Solution

Add missing return type int

Additional Context

No response

Validations

  • Check that there isn't already an issue that reports the same bug
  • Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

Related issue

PR

@nlighteneddesign
Copy link
Author

I'll create a PR soon

@emteknetnz
Copy link
Member

emteknetnz commented Oct 22, 2024

Our CI is working fine for vendor-plugin - https://github.com/silverstripe/vendor-plugin/actions/runs/11432614774

CMS 6 uses symfony 7 and vendor-plugin 2, we're not seeing any issues with the method signature used in vendor-plugin - https://github.com/silverstripe/silverstripe-framework/blob/6/composer.json#L43

That CI build linked installed symfony 6.4 - is your CI doing the same or is it installing something more recent?

Based on silverstripe/recipe-plugin#46 I'm going to assume that you're installing symfony 7

@nlighteneddesign
Copy link
Author

The ci is installing Symfony console 6, however I think the conflict is actually with composer greater than 2.6.6 according to support. I admit it is strange, cause I see symfony console 7 being installed as a prebuild step for composer dependencies, then composer installs my dependencies which include symfony console 6.

The site then won't build due to missing signatures.

@emteknetnz
Copy link
Member

Seems like there's probably an issue with the composer dependencies in your project/ci then? As mentioned we are not seeing any issues with method signatures in the CI builds for either CMS 5 or CMS 6, both of which are using silverstripe/vendor-plugin:^2

As there doesn't appear to be an issue at this end I'll close this issue

@GuySartorelli
Copy link
Member

See silverstripe/recipe-plugin#46 (comment) for why I think we should make this change

@GuySartorelli
Copy link
Member

Closing as a duplicate of silverstripe/.github#325

@GuySartorelli GuySartorelli closed this as not planned Won't fix, can't repro, duplicate, stale Oct 24, 2024
@madmatt
Copy link
Member

madmatt commented Feb 10, 2025

@nlighteneddesign Just in case this helps, I am also running into this issue, also on Platform.sh, for what it's worth, and I managed to solve the issue. This is doing an upgrade from CMS 4 to CMS 5.

On Platform.sh, I can see that it's installing symfony/console version 7.2.1. However, it seems that locally I am also running composer with a similar version that includes the strict int return type, but this doesn't fail for some reason that I can't yet figure out.

Both versions of composer are the same (2.8.5, on php8.2.27).

Forcing Platform.sh to install symfony/console: ^6 resolves the issue. You can do that via the dependencies block in .platform.app.yaml:

build:
  flavor: none
dependencies:
  php:
    require:
      symfony/console: "^6"
hooks:
  build: |
    composer install --no-interaction --no-dev --prefer-dist --no-progress --optimize-autoloader
    composer vendor-expose

You can confirm the installed version during the git push - the first set of composer install commands is installing composer, and the second is building the app.

@GuySartorelli
Copy link
Member

Oh, so it's not the composer version that matters? Is that symfony/console a global dependency somehow? Or is it being included in the project's dependencies?

@madmatt
Copy link
Member

madmatt commented Feb 10, 2025

It's not composer exactly, but rather the version of symfony/console included within composer that is the problem.

Platform.sh installs Composer for you automatically during the build phase of the project. You can decide if you want Composer v1 or v2, we have explicitly requested v2. During a git push it automatically runs your build steps for you, after installing Composer.

Using Silverstripe CMS v4 and PHP 8.1, Platform.sh will install Composer, but with symfony/console version 6. I'm not sure why or how this happens, but it's same actual version of composer that is installed. I presume this is something related to how symfony/console handles versions - perhaps v6 is the best for PHP < 8.2 and v7 is flagged as PHP 8.2+ compatible (I haven't verified that sorry).

Upgrading to CMS 5 and PHP 8.2, the initial install of composer looks like this:

Output from `git push platform test` without dependency fix
$> git push platform test
Enumerating objects: 7, done.
Counting objects: 100% (7/7), done.
Delta compression using up to 10 threads
Compressing objects: 100% (4/4), done.
Writing objects: 100% (4/4), 1.11 KiB | 189.00 KiB/s, done.
Total 4 (delta 3), reused 0 (delta 0), pack-reused 0

Validating submodules

Validating configuration files

Processing activity: Matt Peel pushed to test
    Found 1 new commit

    Building application 'app' (runtime type: php:8.2, tree: 8736a52)
      Generating runtime configuration.
      
      Installing build dependencies...
        Installing php build dependencies: composer/composer
        W: Changed current directory to /app/.global/composer/composer
        W: No composer.lock file present. Updating dependencies to latest instead of installing from lock file. See https://getcomposer.org/install for more information.
        W: Loading composer repositories with package information
        W: Updating dependencies
        W: Lock file operations: 29 installs, 0 updates, 0 removals
        W:   - Locking composer/ca-bundle (1.5.5)
        W:   - Locking composer/class-map-generator (1.6.0)
        W:   - Locking composer/composer (2.8.5)
        W:   - Locking composer/metadata-minifier (1.0.0)
        W:   - Locking composer/pcre (3.3.2)
        W:   - Locking composer/semver (3.4.3)
        W:   - Locking composer/spdx-licenses (1.5.8)
        W:   - Locking composer/xdebug-handler (3.0.5)
        W:   - Locking justinrainbow/json-schema (5.3.0)
        W:   - Locking psr/container (2.0.2)
        W:   - Locking psr/log (3.0.2)
        W:   - Locking react/promise (v3.2.0)
        W:   - Locking seld/jsonlint (1.11.0)
        W:   - Locking seld/phar-utils (1.2.1)
        W:   - Locking seld/signal-handler (2.0.2)
        W:   - Locking symfony/console (v7.2.1)
        W:   - Locking symfony/deprecation-contracts (v3.5.1)
        W:   - Locking symfony/filesystem (v7.2.0)
        W:   - Locking symfony/finder (v7.2.2)
        W:   - Locking symfony/polyfill-ctype (v1.31.0)
        W:   - Locking symfony/polyfill-intl-grapheme (v1.31.0)
        W:   - Locking symfony/polyfill-intl-normalizer (v1.31.0)
        W:   - Locking symfony/polyfill-mbstring (v1.31.0)
        W:   - Locking symfony/polyfill-php73 (v1.31.0)
        W:   - Locking symfony/polyfill-php80 (v1.31.0)
        W:   - Locking symfony/polyfill-php81 (v1.31.0)
        W:   - Locking symfony/process (v7.2.0)
        W:   - Locking symfony/service-contracts (v3.5.1)
        W:   - Locking symfony/string (v7.2.0)
        W: Writing lock file
        W: Installing dependencies from lock file (including require-dev)
        W: Package operations: 29 installs, 0 updates, 0 removals
        W:   - Downloading composer/class-map-generator (1.6.0)
        W:   - Installing symfony/process (v7.2.0): Extracting archive
        W:   - Installing symfony/polyfill-php81 (v1.31.0): Extracting archive
        W:   - Installing symfony/polyfill-php80 (v1.31.0): Extracting archive
        W:   - Installing symfony/polyfill-php73 (v1.31.0): Extracting archive
        W:   - Installing symfony/finder (v7.2.2): Extracting archive
        W:   - Installing symfony/polyfill-mbstring (v1.31.0): Extracting archive
        W:   - Installing symfony/polyfill-ctype (v1.31.0): Extracting archive
        W:   - Installing symfony/filesystem (v7.2.0): Extracting archive
        W:   - Installing symfony/polyfill-intl-normalizer (v1.31.0): Extracting archive
        W:   - Installing symfony/polyfill-intl-grapheme (v1.31.0): Extracting archive
        W:   - Installing symfony/string (v7.2.0): Extracting archive
        W:   - Installing symfony/deprecation-contracts (v3.5.1): Extracting archive
        W:   - Installing psr/container (2.0.2): Extracting archive
        W:   - Installing symfony/service-contracts (v3.5.1): Extracting archive
        W:   - Installing symfony/console (v7.2.1): Extracting archive
        W:   - Installing seld/signal-handler (2.0.2): Extracting archive
        W:   - Installing seld/phar-utils (1.2.1): Extracting archive
        W:   - Installing seld/jsonlint (1.11.0): Extracting archive
        W:   - Installing react/promise (v3.2.0): Extracting archive
        W:   - Installing psr/log (3.0.2): Extracting archive
        W:   - Installing justinrainbow/json-schema (5.3.0): Extracting archive
        W:   - Installing composer/pcre (3.3.2): Extracting archive
        W:   - Installing composer/xdebug-handler (3.0.5): Extracting archive
        W:   - Installing composer/spdx-licenses (1.5.8): Extracting archive
        W:   - Installing composer/semver (3.4.3): Extracting archive
        W:   - Installing composer/metadata-minifier (1.0.0): Extracting archive
        W:   - Installing composer/class-map-generator (1.6.0): Extracting archive
        W:   - Installing composer/ca-bundle (1.5.5): Extracting archive
        W:   - Installing composer/composer (2.8.5): Extracting archive
        W: Generating optimized autoload files
        W: 24 packages you are using are looking for funding.
        W: Use the `composer fund` command to find out more!
      Executing build hook...
        ===== Run composer install explicitly:
<snip most of the composer install from the project's composer.json>
<note that this `composer install` DOES run vendor-expose but does not seem to actually expose everything - unsure why this is>
        ===== Run composer vendor-expose explicitly:
        W: Composer could not detect the root package (akqa-nz/unimed) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
        W: PHP Fatal error:  Declaration of SilverStripe\VendorPlugin\Console\VendorExposeCommand::execute(Symfony\Component\Console\Input\InputInterface $input, Symfony\Component\Console\Output\OutputInterface $output) must be compatible with Symfony\Component\Console\Command\Command::execute(Symfony\Component\Console\Input\InputInterface $input, Symfony\Component\Console\Output\OutputInterface $output): int in /app/vendor/silverstripe/vendor-plugin/src/Console/VendorExposeCommand.php on line 36

After making the above YML change, I get the following install plan for composer (and the vendor-expose command works):

Output from `git push platform test` after dependency fix
$> git push platform test
git push platform test
Enumerating objects: 5, done.
Counting objects: 100% (5/5), done.
Delta compression using up to 10 threads
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 1.29 KiB | 439.00 KiB/s, done.
Total 3 (delta 2), reused 0 (delta 0), pack-reused 0

Validating submodules

Validating configuration files

Processing activity: Matt Peel pushed to test
    Found 1 new commit

    Building application 'app' (runtime type: php:8.2, tree: 8736a52)
      Generating runtime configuration.
      
      Installing build dependencies...
        Installing php build dependencies from composer_require
        W: Changed current directory to /app/.global
        W: No composer.lock file present. Updating dependencies to latest instead of installing from lock file. See https://getcomposer.org/install for more information.
        W: Loading composer repositories with package information
        W: Updating dependencies
        W: Lock file operations: 9 installs, 0 updates, 0 removals
        W:   - Locking psr/container (2.0.2)
        W:   - Locking symfony/console (v6.4.17)
        W:   - Locking symfony/deprecation-contracts (v3.5.1)
        W:   - Locking symfony/polyfill-ctype (v1.31.0)
        W:   - Locking symfony/polyfill-intl-grapheme (v1.31.0)
        W:   - Locking symfony/polyfill-intl-normalizer (v1.31.0)
        W:   - Locking symfony/polyfill-mbstring (v1.31.0)
        W:   - Locking symfony/service-contracts (v3.5.1)
        W:   - Locking symfony/string (v7.2.0)
        W: Writing lock file
        W: Installing dependencies from lock file (including require-dev)
        W: Package operations: 9 installs, 0 updates, 0 removals
        W:   - Installing symfony/polyfill-mbstring (v1.31.0): Extracting archive
        W:   - Installing symfony/polyfill-intl-normalizer (v1.31.0): Extracting archive
        W:   - Installing symfony/polyfill-intl-grapheme (v1.31.0): Extracting archive
        W:   - Installing symfony/polyfill-ctype (v1.31.0): Extracting archive
        W:   - Installing symfony/string (v7.2.0): Extracting archive
        W:   - Installing symfony/deprecation-contracts (v3.5.1): Extracting archive
        W:   - Installing psr/container (2.0.2): Extracting archive
        W:   - Installing symfony/service-contracts (v3.5.1): Extracting archive
        W:   - Installing symfony/console (v6.4.17): Extracting archive
        W: Generating optimized autoload files
        W: 8 packages you are using are looking for funding.
        W: Use the `composer fund` command to find out more!

One other thing to note: During the investigation of this, I noticed that running composer install alone does actually seem to internally run composer vendor-expose as well, albeit via a method that doesn't seem to break with the strict return type warning (perhaps a different way of calling it).

However this is being called, it doesn't appear to actually fully expose everything. In particular, the root-level resources aren't exposed properly, despite being listed as being exposed in the console output. I'll try to debug that one later, as it might be a Platform.sh specific issue (it uses a read-only filesystem to install the app to, which may impact the symlinking in odd ways).

@emteknetnz
Copy link
Member

With the yml fix I noticed it's installing quite a lot less dependencies, for example Installing php build dependencies: composer/composer is missing - You said Platform.sh will install Composer, - though it seems like it's perhaps just using a version that was already there? Is that second test "fresh", or is it an update on the same environment after running the first deployment?

@madmatt
Copy link
Member

madmatt commented Feb 10, 2025

It's as fresh as I can make it, although I don't know what it's doing internally. I also noticed the different dependencies as well and couldn't make heads nor tail of it. v6 does require an additional module symfony/deprecation-contracts which v7 doesn't, but that itself has no further dependencies.

It's possible that Platform.sh has some kind of cache that only applies for the latest version or something and as soon as you break the cache key by forcing a specific version, it installs it all from scratch or something similar. At that point we're getting into platform-specific nuances that I don't think are useful to Silverstripe.

At this point I don't know what the solution is - this seems to at least be a way to resolve the specific issue with Silverstripe CMS 5, PHP 8.2 (haven't tried 8.3 yet, that's my next step) Composer v2 and Platform.sh.

Obviously updating this check will be required in future, so I think just planning to upgrade with CMS v6 to target symfony/console v7 makes sense. I don't think we can do much about composer itself... the only other thing I can suggest is to make the change to add the int return type. That's obviously an API break, but it's only a break for anyone who has sub-classed the VendorExposeCommand, right? It wouldn't break backwards (e.g. where symfony/console doesn't have the return type) as the E_FATAL only occurs in one direction (base class has a return type that a sub-class doesn't). But yeah - API break means major version increase, which seems unwarranted here unless it was wider spread than Platform.sh (which it may be, but seems unlikely given the relative maturity of PHP 8.2, Composer and Silverstripe CMS 5 at this point).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants