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

Fix composer.json platform.php setting. #704

Closed
wants to merge 1 commit into from
Closed

Conversation

jwage
Copy link
Member

@jwage jwage commented May 30, 2018

Q A
Type improvement
BC Break no
Fixed issues #701

Summary

This was something that @theofidry suggested we add to composer.json for the box build process. I admit I don't fully understand why. Maybe someone can chime in here on why it is needed and what the value should be.

@jwage jwage added this to the 2.0 milestone May 30, 2018
@jwage jwage self-assigned this May 30, 2018
@jwage jwage requested a review from Majkl578 May 30, 2018 13:58
@theofidry
Copy link
Contributor

I'll try to give a clear explanation :)

If you are on PHP 7.2+ and run a composer update, because your composer.json constraint is ^7.1, it might install code that is compatible only with PHP 7.2+. If that's the case, when you will build the PHAR, you will ship a PHAR working only for PHP 7.2+ instead of ^7.1.

Putting this configuration enforce Composer to behave as if you were running on a specific version of PHP, so here 7.1.0, regardless of your actual PHP version on your machine.

Note that this config should be unset for the build not building the PHAR with for example with composer config --unset platform.php

@Majkl578
Copy link
Contributor

I'd argue you should explicitly set the platform before the packaging, not making it by default present in composer.json.
This in the end affects testing of lowest/latest dependencies too and will always i.e. reject Symfony 4 completely on CI.

@theofidry
Copy link
Contributor

This in the end affects testing of lowest/latest dependencies too and will always i.e. reject Symfony 4 completely on CI.

Not if you remove it:

Note that this config should be unset for the build not building the PHAR with for example with composer config --unset platform.php

If you don't set it by default you must not forget to add it when building the PHAR locally, so maybe it can be added to the build script instead.

@Majkl578
Copy link
Contributor

Ok, then anyone who clones the repo will... Telling everyone to unset the platform just to accomodate a release process is a bit overkill imho.

If you don't set it by default you must not forget to add it when building the PHAR locally

Can be done by the build-phar.sh script.

@Majkl578
Copy link
Contributor

See alternative apporach in #705.

@jwage
Copy link
Member Author

jwage commented May 30, 2018

@Majkl578 I like that approach better 👍 closing this PR.

@jwage jwage closed this May 30, 2018
@jwage jwage removed this from the 2.0 milestone May 30, 2018
@jwage jwage removed the Improvement label May 30, 2018
@jwage jwage deleted the fix-composer-platform branch May 31, 2018 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants