php-fig / container

MIT License
9.95k stars 53 forks source link

PHP version requirement was changed in a patch version (1.1.2) #41

Closed guvra closed 2 years ago

guvra commented 3 years ago

Hi,

In version 1.1.1, PHP version requirement was the following:

"php": ">=7.2.0"

In version 1.1.2, it was changed to the following:

"php": ">=7.4.0"

What happens if there is a critical security issue that is fixed in a future release (e.g. 1.1.3)? Projects based on PHP <=7.3 won't be able to upgrade.

If the reason is that PHP 7.3 will soon be EOL, debian buster uses it and is still maintained (EOL date 2022-08).

Thanks

Jean85 commented 3 years ago

The bump in the requirement was a bugfix in itself. We reintroduced extends \Throwable in the exception, which was problematic in PHP versions older than 7.4, see #31.

Normally your concern would be warranted, but I cannot imagine a situation where a further patch would be needed, since here we only have 3 interfaces and no working code.

skyselang commented 3 years ago

Bad upgrade, regardless of the consequences, can only lock the version

Jean85 commented 3 years ago

Bad upgrade, regardless of the consequences, can only lock the version

What do you mean? Composer will prevent the installation of the patch if you don't have an unsupported PHP version.

mtx-z commented 3 years ago

Same here: this "broke" (pipeline fails on composer commands) my Laravel v5.8.38 on composer update, as it requires (Laravel framework composer.json) "psr/container": "^1.0", and I'm running PHP 7.3. It updated the psr/container from 1.1.1 to 1.1.2. I'll need to lock the package version or update my PHP version.

As you can guess the error is the following:

Problem 1 - psr/container is locked to version 1.1.2 and an update of this package was not requested. - psr/container 1.1.2 requires php >=7.4.0 -> your php version (7.3.23) does not satisfy that requirement. 
Problem 2 - psr/container 1.1.2 requires php >=7.4.0 -> your php version (7.3.23) does not satisfy that requirement. - symfony/service-contracts v2.4.0 requires psr/container ^1.1 -> satisfiable by psr/container[1.1.2]. - symfony/service-contracts is locked to version v2.4.0 and an update of this package was not requested.

Unfortunately, Laravel 5.x support ended, I doubt we'll have an update on this side.

image source

drealecs commented 3 years ago

I can only guess that you run composer update using a PHP version >= PHP 7.4 and during composer install you used PHP 7.3. You need to run composer update using PHP 7.3 so that the package version stays at 1.1.1 and will not be upgraded to 1.1.2.

As a good practice and to ensure you don't bump into this kind of problems, you should make sure when running composer update, during development time, you use the same PHP version as the one used for running the actual application, where you will also run composer install. If this is not possible, for example because you run composer as a container and that uses the latest PHP version and just because you have locally a newer version that you use in other multiple projects, you can add to composer.json:

{
  "config": {
    "platform": {
      "php": "7.3"
    }
  }
}
Jean85 commented 3 years ago

Totally agree on @drealecs. You would've had the same issue even if I released this change as 1.2.0.

mtx-z commented 3 years ago

You are right, I indeed ran composer update on my local machine, with PHP 7.4. But my project composer.json contains "php": "^7.3.0", and I thought this was enough, as I never encountered this kind of issue.

had the same issue even if I released this change as 1.2.0

agreed as Laravel required ^1.0

EDIT: the "php": "^7.3.0" is a require and not a platform config, I'll try that way.

Jean85 commented 3 years ago

Also "php": "^7.3.0" allows 7.4 (or it wouldn't work on your local machine with 7.4).

skyselang commented 3 years ago

It violates the definition of version number and is not compatible with each other.

Jean85 commented 3 years ago

It violates the definition of version number and is not compatible with each other.

@skyselang can you explain you comment? It's not clear to me.

corex commented 2 years ago

I can answer on behalf of @skyselang

No matter what you suggests, it is only "symptom treatment". You are not following the semantic versioning strategy supported by composer.

"Composer uses Semantic Versioning (aka SemVer) 2.0.0" Source: https://getcomposer.org/doc/faqs/which-version-numbering-system-does-composer-itself-use.md#which-version-numbering-system-does-composer-itself-use-

Summary for Semantic Versioning 2.0.0 - Given a version number MAJOR.MINOR.PATCH, increment the:

  1. MAJOR version when you make incompatible API changes,
  2. MINOR version when you add functionality in a backwards compatible manner, and
  3. PATCH version when you make backwards compatible bug fixes.

This effectively means that you can not use i.e. "composer update symfony/console --with-dependencies" as this will force psr/container 1.1.2 will be added to the project.

We have multiple legacy sites, which I need to go through and make sure config.platform.php target is set. It takes time. Note the word "legacy".

I strongly recommend that you revert the PATCH and release a new version with old php version specified.

After that step, introduce the breaking change and make a MAJOR bump/release.

Jean85 commented 2 years ago

I'm sorry but I have to rely on Composer detecting the right PHP language version on your side. Bumping the required language version is not a requirement for a major bump, and this case is a perfect example, since the bumping was needed to fix a bug.

This argument has been hashed over and over, and I've explained my position at length in https://github.com/getsentry/sentry-laravel/issues/335#issuecomment-629132274 (where you'll find multiple links to multiple similar closed issues).

Yes, I could've release this as a minor, in case a patch for older PHP versions would be needed, but that's probably not going to happen here.

So, I'm closing this as wont-fix.

drealecs commented 2 years ago

"Composer uses Semantic Versioning (aka SemVer) 2.0.0" Source: https://getcomposer.org/doc/faqs/which-version-numbering-system-does-composer-itself-use.md#which-version-numbering-system-does-composer-itself-use-

This only mentioned what versions are used by composer itself when released. Literally the ones listed here https://github.com/composer/composer/releases .
There is no mention that all other projects that uses composer for distribution must use semantic versioning. You can read here: https://getcomposer.org/doc/articles/versions.md: "it is mostly useful for projects respecting semantic versioning".

We have multiple legacy sites, which I need to go through and make sure config.platform.php target is set. It takes time. Note the word "legacy".

If your php version for running composer when doing composer update is not the php version from production, you should make sure at least config.platform.php is up to date with the production version. Otherwise, you're going to hit other problems with other packages as well, not just this one. You just need to remember to do it once before running the next first composer update. I believe that might not be a big effort considering that you already touch/review the composer.json/composer.lock files.