-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Remove pinned platform #4824
Remove pinned platform #4824
Conversation
Signed-off-by: Alexander M. Turek <me@derrabus.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should indeed have been dropped when removing composer.lock
from version control. My bad.
@@ -54,9 +54,6 @@ | |||
}, | |||
"bin": ["bin/doctrine-dbal"], | |||
"config": { | |||
"platform": { | |||
"php": "7.3.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was introduced to prevent locking the versions that are incompatible with the lowest supported PHP version. While the lock file is gone, it's less of an issue but without pinning the platform, it becomes possible to upgrade a dependency to such a version. Do we rely on CI for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without pinning the platform, it becomes possible to upgrade a dependency to such a version.
Yes. And I would argue that this is not a problem and might even be desirable.
Do we rely on CI for that?
Well currently, the CI installs dependencies compatible with PHP 7.3.0, even on the PHP 8 jobs. For instance, psr/log
is installed as version 1.1:
https://github.com/doctrine/dbal/runs/3733999925#step:4:59
If you look at the same job in this PR, you'll see that version 2.0 is installed instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if I may add: We do have a PHP 7.3 job that makes sure that the package version constraints we set in composer.json can be fulfilled on PHP 7.3. So we don't need the platform pinning for that matter either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. While this change makes the maintenance a bit more challenging, it's not the reason to disallow newer package versions.
Summary
I'd like to remove the
platform.php
setting from our composer.json file.Without a lock file under version control, all that setting does is preventing the installation of newer packages on PHP 7.4 and higher (e.g. psr/log 2). I would argue that testing with different sets of dependencies for different PHP versions is somewhat desirable.