remove pre-configured platform PHP version #236
Conversation
|
👍 |
| unset($contents['config']['platform']); | ||
| } | ||
|
|
||
| if (empty($contents['config'])) { |
There was a problem hiding this comment.
What about to make it nested to the previous if statement? The $contents['config'] could be empty only when we unset $contents['config']['platform'], otherwise it probably has at least one item.
if (empty($contents['config']['platform'])) {
unset($contents['config']['platform']);
if (empty($contents['config'])) {
unset($contents['config']);
}
}We should use cascade checks here, I think.
There was a problem hiding this comment.
I wanted to avoid nesting control structures too deeply.
There was a problem hiding this comment.
What purpose to do it? Only to reduce complexity?
There was a problem hiding this comment.
Yes, but I am happy to change it if others feel uncomfortable too with how it is right now.
There was a problem hiding this comment.
OK, let's wait for others a bit, but I think I'm fine to keep it as is
There was a problem hiding this comment.
I think this does not complexify the behavior, it just simplifies the outputted JSON
|
ping @symfony/deciders This seems to cause issues for people not familiar with the platform config in Composer (see composer/composer#4319 (comment)). |
|
Does this result in a warning that "composer.json is not in sync with lock file"? Or is this config option not considered for this sync check by composer? |
|
The lock file is synchronised after the |
|
LGTM 👍 |
|
👍 |
|
Just FYI composer/composer#4881 should solve the confusion side.. I'd argue having platform config is a good thing but obviously if people don't take care of setting it correctly it's a bad idea, so maybe not having one by default is better. |
|
@Seldaek but having it set to 5.3 in new Symfony 2.8 projects is not a good idea. symfony standard-edition needs this value because of its min supported version, but I hope new projects actually target higher PHP runtimes when deploying. So 👍 for this change |
|
@javiereguiluz What do you think? |
|
I think it can be merged now and released for newcomers to avoid this issue 👍 |
|
👍 |
|
@xabbuh I'm sorry it took me so long to merge your contribution. It's merged now. |
This PR was merged into the 1.0-dev branch. Discussion ---------- remove pre-configured platform PHP version This resolves #234. Commits ------- be7d2dd remove pre-configured platform PHP version
This resolves #234.