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

Bump php to 8.2 #6506

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Bump php to 8.2 #6506

wants to merge 11 commits into from

Conversation

jeffwidman
Copy link
Member

@jeffwidman jeffwidman commented Jan 24, 2023

We are currently on 7.4 which has been EOL'd by upstream in Nov 2022:

There is no newer version of the 7.x series, so we have to bump to the 8.x series.

Fix: #6527
Fix: #6958

@jeffwidman
Copy link
Member Author

While I wasn't in a hurry to bump this for fear of accidentally breaking
compatibility with php 7.x code, we are starting to see test failures
that indicate we will need to bump soon:

[dependabot-core-dev] ~/dependabot-core/composer $ DEBUG_HELPERS=true rspec ./spec/dependabot/composer/update_checker/version_resolver_spec.rb:225
Run options: include {:locations=>{"./spec/dependabot/composer/update_checker/version_resolver_spec.rb"=>[225]}}

Randomized with seed 12699
php -d memory_limit=-1 /opt/composer/v2/bin/run
{"error":"Your requirements could not be resolved to an installable set of packages.\n  Problem 1\n    - monolog\/monolog dev-main requires php >=8.1 -> your php version (7.4.33) does not satisfy that requirement.\n    - monolog\/monolog 3.x-dev is an alias of monolog\/monolog dev-main and thus requires it to be installed too.\n    - Root composer.json requires monolog\/monolog ^3.0|4.1.x-dev as 3.0.0 -> satisfiable by monolog\/monolog[3.0.0-RC1, ..., 3.x-dev (alias of dev-main)].\n"}

So opening this draft PR to see what CI thinks.

@jeffwidman
Copy link
Member Author

I don't know enough about PHP and also our usage of composer to know if us bumping to 8.x will break Dependabot on anyone running with 7.4, as that's still reasonably popular.

There's a chance that since we're not installing but only looking for updates that it will not matter the version of PHP that Dependabot uses... I don't really know though.

@jeffwidman jeffwidman marked this pull request as ready for review January 24, 2023 08:10
@jeffwidman jeffwidman requested a review from a team as a code owner January 24, 2023 08:10
@jurre
Copy link
Member

jurre commented Jan 24, 2023

I don't know enough about PHP and also our usage of composer to know if us bumping to 8.x will break Dependabot on anyone running with 7.4, as that's still reasonably popular.

Yeah, same 🤔 can we ask someone that PHP's? I see a fair number of failing tests also, which seems to happen in the build step? Is that related?

@jeffwidman
Copy link
Member Author

jeffwidman commented Jan 24, 2023

Let me work through the build failures, and then I'll tag some PHP folks... but the build failures are because of various things that we'll have to fix whenever this actually happens.

@jeffwidman
Copy link
Member Author

@stof @Seldaek 👋 from Dependabot.

We are currently running php 7.4 when we composer against users' composer manifest files... if we bump that to 8.2 in our native helpers (which is what this PR does), then will that still allow our composer instance to work fine against users' manifests that are still using 7.4 / 8.0 / 8.1?

I am unclear if composer only looks at the php version specified in the manifest file that it's updating, or also consider its own version of PHP that it's running on.

I can test it out myself, but wanted to also check with the composer maintainers in case there's some gotcha that won't be immediately apparent to us.

@Seldaek
Copy link

Seldaek commented Jan 25, 2023

It might cause some issues for projects not defining a platform config. But right now you are also having issues for people relying on more modern PHP versions, so I'd say running latest is probably a better trade-off.

@jeffwidman
Copy link
Member Author

jeffwidman commented Jan 25, 2023

Thanks @Seldaek, that makes sense.

While fixing UT failures, I also noticed this failure:

{"error":"Your requirements could not be resolved to an installable set of packages.
Problem 1
 - doctrine\/instantiator 1.0.5 requires php >=5.3,<8.0-DEV -> your php version (8.2.1) does not satisfy that requirement.
 - phpunit\/phpunit 5.1.3 requires phpunit\/phpunit-mock-objects >=3.0.5 -> satisfiable by phpunit\/phpunit-mock-objects[3.0.6].
 - phpunit\/phpunit-mock-objects 3.0.6 requires doctrine\/instantiator ^1.0.2 -> satisfiable by doctrine\/instantiator[1.0.5].
 - phpunit\/phpunit is locked to version 5.1.3 and an update of this package was not requested.
 "}

doctrine/instantiator is an indirect / transient dep, so that's why it's not getting updated. So if we bump to PHP 8, then any transient deps that explicitly do not support PHP 8 will blow up.

However, I don't think that should block us upgrading php, as most packages will either not pin an upper limit on the PHP version, or they'll have released a newer version that added support... for example, doctrine/instantiator removed the blocking pin way back in 2017.

And as @Seldaek mentioned above, we're already in the situation that we're preventing new PR's from being opened where the package requires PHP 8. For example, the aforementioned doctrine/instantiator just released 2.0 which dropped support for php < 8... so Dependabot won't open PR's to bump that package to 2.0 until this PR is merged.

Comment on lines 85 to 93
// TODO surprisingly the returned result of getPrettyVersion depends on the PHP version:
// - PHP 7 returns: "2.4.1"
// - PHP 8 returns: "2.4.1@stable"
// file_put_contents('php://stdout', $updatedPackage->getPrettyVersion());
//
// return preg_replace('/^([v])/', '', $updatedPackage->getPrettyVersion());

$pretty = $updatedPackage->getPrettyVersion();

return rtrim(ltrim($pretty, 'v'), '@stable');
Copy link
Member Author

@jeffwidman jeffwidman Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Seldaek any idea why the returned value of getPrettyVersion() version depends on the underlying PHP version? I found this very surprising given that the composer code didn't change, only the php version.

This is a unit test running under composer 1, accessing a PEAR registry. I do realize that both PEAR and composer v1 are deprecated, but we're not quite ready to drop support for them, so I'd like to fix our code to buy us compatibility for a little longer.

And really what I care about is:
Is this @stable suffix always going to be the same returned value such that it's safe for me to simply strip it off? Or could it sometimes return @beta, -dev, etc?

IE, is @stable something PHP itself is adding in some manner in which case I'd always expect it to be there and I can safely strip it (I don't know if PHP has a concept of stable/unstable objects aka immutable?), or is it something composer is adding, so it may vary depending on the package/registry?

Note to myself - encountered while running:

[dependabot-core-dev] ~/dependabot-core/composer $ DEBUG_HELPERS=true rspec ./spec/dependabot/composer/update_checker_spec.rb:451

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nudge @Seldaek, any chance of quick input on ☝️ ?

To be clear, not asking for you do to additional research, just if you know off top of your head.

This is the only remaining unresolved question for this PR to get Dependabot to support PHP 8.

@jeffwidman jeffwidman force-pushed the bump-php-to-8.2 branch 4 times, most recently from f9616e9 to 8a0fa28 Compare March 3, 2023 06:16
@jeffwidman jeffwidman marked this pull request as ready for review March 3, 2023 06:17
@jeffwidman
Copy link
Member Author

jeffwidman commented Mar 3, 2023

This is now ready for review, with the only remaining item from a code standpoint being #6506 (comment).

From a user impact standpoint, I still need to pull metrics on what versions of PHP our users are using, and also double-check my current understanding that:

  1. w/o this, manifests that require PHP 8 fail
  2. with this, manifests that require PHP 7 fail

@jeffwidman jeffwidman mentioned this pull request Apr 3, 2023
1 task
@github-actions github-actions bot added the L: php:composer Issues and code for Composer label Aug 21, 2023
`php-cs-fixer` < `3.15` doesn't support `8.2` and instead throw ts the
following error/warning:

```
 > php-cs-fixer fix --diff --verbose '--dry-run'
  PHP needs to be a minimum version of PHP 7.4.0 and maximum version of PHP 8.1.*.
  Current PHP version: 8.2.1.
  To ignore this requirement please set `PHP_CS_FIXER_IGNORE_ENV`.
  If you use PHP version higher than supported, you may experience code modified in a wrong way.
  Please report such cases at https://github.com/PHP-CS-Fixer/PHP-CS-Fixer .
```

However, when I tried to upgrade it, I ran into a problem that the newer
version of `php-cs-fixer` has a dependency on a `composer` library
version that is incompatible with `composer1`.

So this silences the error so that the build doesn't fail. Note that
we're only doing this for `composer 1`, as our `composer 2`-based
helper/environment works fine. So the long term solution here will be
deprecating/dropping support for `composer 1`.
```
[dependabot-core-dev] ~/dependabot-core/composer $ DEBUG_HELPERS=true rspec ./spec/dependabot/composer/update_checker_spec.rb:717
Run options: include {:locations=>{"./spec/dependabot/composer/update_checker_spec.rb"=>[717]}}

Randomized with seed 14137
php -d memory_limit=-1 /opt/composer/v2/bin/run
{"error":"Your requirements could not be resolved to an installable set of packages.\n  Problem 1\n    - doctrine\/instantiator 1.0.5 requires php >=5.3,<8.0-DEV -> your php version (8.2.1) does not satisfy that requirement.\n    - phpunit\/phpunit 5.1.3 requires phpunit\/phpunit-mock-objects >=3.0.5 -> satisfiable by phpunit\/phpunit-mock-objects[3.0.6].\n    - phpunit\/phpunit-mock-objects 3.0.6 requires doctrine\/instantiator ^1.0.2 -> satisfiable by doctrine\/instantiator[1.0.5].\n    - phpunit\/phpunit is locked to version 5.1.3 and an update of this package was not requested.\n"}
```

`doctrine/instantiator` is an indirect dep, so fine to bump w/o affecting
the unit test. So I ran `composer update doctrine/instantiator` to only
bump that dep (and any pins it required bumping) and then committing the results.

The test now passes.
This test was failing with:
```
[dependabot-core-dev] ~/dependabot-core/composer $ DEBUG_HELPERS=true rspec ./spec/dependabot/composer/file_updater/lockfile_updater_spec.rb:191
Run options: include {:locations=>{"./spec/dependabot/composer/file_updater/lockfile_updater_spec.rb"=>[191]}}

Randomized with seed 9856
{}
php -d memory_limit=-1 /opt/composer/v1/bin/run
update
{"error":"Your requirements could not be resolved to an installable set of packages.\n  Problem 1\n    - dealerdirect\/phpcodesniffer-composer-installer v0.4.4 requires php ^5.3|^7 -> your PHP version (8.2.3) does not satisfy that requirement.\n    - dealerdirect\/phpcodesniffer-composer-installer v0.4.4 requires php ^5.3|^7 -> your PHP version (8.2.3) does not satisfy that requirement.\n    - Installation request for dealerdirect\/phpcodesniffer-composer-installer 0.4.4 -> satisfiable by dealerdirect\/phpcodesniffer-composer-installer[v0.4.4].\n"}
```

So I manually bumped it up to `1.0.0` in `composer.json`, then ran
`composer update dealerdirect/phpcodesniffer-composer-installer` to only
bump that package and its deps.

The test now passes. This test was asserting that a package that was
incompatible with `composer` v1 would throw an error. And the newer
version is still incompatible so the behavior under test didn't change.
Initially the test was failing because some of the indirect deps in
`composer.lock` enforced a PHP constraint < `8.0.0`. I was able to
workaround that by a simple `composer1 update`, which pruned the
lockfile heavily.

Then I noticed the test was failing because when it tried to extract the
version from the return value of the native helper that was failing.
Which I eventually tracked down to a difference in behavior of the
`getPrettyVersion` depending on the PHP version:
 - PHP 7 returns: "2.4.1"
 - PHP 8 returns: "2.4.1@stable"

Fixing this test may be as simple as tweaking the anchoring regex. Or we
could drop support for PEAR, but I'm a little hesitant to do that until
we drop support for `composer` `1`, as that's the easy time to drop it.
This test also didn't have a target version that was PHP 8 compatible.

Interestingly, it failed when I set the test expectation to `v4.22.1`,
but passes at `4.22.1` because upstream dropped the `v` prefix:
https://nova.laravel.com/releases/4.22.1
This one was a doozy.

```
DEBUG_HELPERS=true rspec ./spec/dependabot/composer/file_updater/lockfile_updater_spec.rb:404
```

results in:
```
Dependabot::DependencyFileNotResolvable:
    Your requirements could not be resolved to an installable set of packages.
        Problem 1
        - doctrine/inflector v1.3.0 requires php ^7.1 -> your php version (8.2.3) does not satisfy that requirement.
        - illuminate/support v5.2.0 requires doctrine/inflector ~1.0 -> satisfiable by doctrine/inflector[v1.3.0].
        - illuminate/support is locked to version v5.2.0 and an update of this package was not requested.
```

So we need a version of `doctrine/inflector` that is php 8, which landed in `1.4.2` via doctrine/inflector#159. So then we need to figure out what version of `illuminate/support` allows a new enough `doctrine/inflector` version. illuminate/support@7ac4794 bumped it up which says the 6.x series.

`composer update illuminate/support doctrine/inflector` also required unlocking bumping `illuminate/contracts` to allow `composer` to generate a lockfile that satisfied all constraints.

I was a little concerned because the point of this unit test is to ensure that `illuminate/contracts` bumps to the expected version, so I found the oldest version of `illuminate/contracts` on their `6.x` branch is `6.20.0` which landed in illuminate/contracts@55a3d76. And they're now up to `6.20.44` so this test is still workable.

So I manually downgraded `illuminate/contracts` to `6.20.0` in `composer.lock` and then ran `composer update --lock` to regenerate all the accompanying metadata.

I realize the actual versions probably don't matter as these are all stubs, but still I feel better when the stubs match reality.

However, once I updated the `composer.json`/`composer.lock`, then two
other tests were failing.

So I had to bump their desired version to new versions that matched
their tests expected behavior, as well as the `previous_version` to
match the new lockfile content.
The test was failing because the dependencies used didn't support PHP 8.

However, when I updated them to the oldest version that still supported
PHP 8, then they worked fine against the newest version of composer.

When I dug into it, I realized that because composer 1 was deprecated a
while back, any plugins that don't support the newest version of
composer v1 won't support PHP 8.2, and so the emitted error message is
about deps not working with PHP 8. While still technically possible, the
reality is that the error of allowing a modern PHP version but not a new
version of a deprecated composer major version just isn't going to
happen. And in the worst case that I'm somehow wrong, this isn't that
big of a deal, as the job will fail either way, it just now fails with
a subprocess unknown failure and prints the warning message for a human
to interpret.

So I removed the custom error handling and the associated test.
@Didel
Copy link

Didel commented Aug 29, 2023

I would love to see this merged. I did some testing locally (from the user perspective), and this branch did resolve my problems (PR's not being created by dependabot).

Of course, things will be different for each package of php-code, but most of the larger PHP ecosystems (e.g. I am working a lot with Drupal and Symfony packages) have built in the expectation that their tools should work on a large range of PHP environments, ranging from older 7.4 environments to the newest 8.2 . Most will have some versions that provide some overlap, e.g. working on 7.4 and 8.2, and then introducing a newer major version that drops support for the old 7.4 version. In that case, the user is actively updating their package to a new version.

As I see it, it might be the case that this could break update-checks for packages that rely on php 7.4 and don't support newer versions. But as already stated, 7.4 is EOL and should not be supported any more. Current versions of tools/packages should be expected to work fine with PHP 8.2 (as it is current). Worst case, it could break version-checks for some older versions of packages, which should be resolved if the package is updated outside of dependabot to a version that does support 8.2 .

More importantly, without this change, many updates that should be performed are not mentioned by dependabot, because composer is not able to resolve a set of versions that matches the constraints (because the versions in the constraints require php >= 8.0 or php >= 8.1 , not provided on the dependabot environment without this change). As more packages are updated and dropping support for older PHP versions, dependabot will only list less and less PHP upgrades without this change. I would be in favor of merging this ASAP, and perhaps eventually making the used PHP version more configurable in the future (e.g. it looks like npm_yarn allows using an argument containing the NODEJS_VERSION to override if needed).

@jeffwidman
Copy link
Member Author

cc @abdulapopoola ☝️ in case you didn't see this user feedback.

@Didel
Copy link

Didel commented Feb 7, 2024

What is the status of this ticket? At this point PHP support in Dependabot is non-functional for projects using any of the supported PHP versions (PHP >= 8.1). As a reminder, this is the current state of the supported PHP versions:

image
(Taken from php.net/supported-versions ).

What is needed to merge this PR? How can others contribute? I personally would be in favor of making PHP versions user-configurable (as mentioned above), but not before this PR has been merged to make Dependabot usable again.

@vintagesucks
Copy link

we're already in the situation that we're preventing new PR's from being opened where the package requires PHP 8. For example, the aforementioned doctrine/instantiator just released 2.0 which dropped support for php < 8... so Dependabot won't open PR's to bump that package to 2.0 until this PR is merged.

More importantly, without this change, many updates that should be performed are not mentioned by dependabot, because composer is not able to resolve a set of versions that matches the constraints (because the versions in the constraints require php >= 8.0 or php >= 8.1, not provided on the dependabot environment without this change).

At this point PHP support in Dependabot is non-functional for projects using any of the supported PHP versions (PHP >= 8.1).

@Didel Not sure if this applies to your use case, but just in case this helps anyone: // cc @jeffwidman

We use Dependabot in many PHP 8.1/8.2 projects without any problems, and also get Dependabot updates for packages that only support PHP 8.1+. If I remember correctly, we just define a target PHP version in our composer.json require section for this to work, e.g.:

"require": {
    "php": "^8.2.9",

@Didel
Copy link

Didel commented Feb 8, 2024

@Didel Not sure if this applies to your use case, but just in case this helps anyone: // cc @jeffwidman

We use Dependabot in many PHP 8.1/8.2 projects without any problems, and also get Dependabot updates for packages that only support PHP 8.1+. If I remember correctly, we just define a target PHP version in our composer.json require section for this to work, e.g.:

"require": {
    "php": "^8.2.9",

That does seem to help, yes! Thank you for the suggestion! I still think that's not a 'fix' but more of a work-around for the underlaying problem (depending on EOL PHP versions), but at least that allows Dependabot to create updates again. That will probably also explain why not every PHP project is impacted, as some (most?) might have the PHP dependency specified in their composer.json file.

@abdulapopoola
Copy link
Member

tagging @jonjanego as part of the release standardization effort

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: php:composer Issues and code for Composer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch Dependabot from running composer using php 7.4 to php 8.2 Dependabot does not send pr
6 participants