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

Minimum PHP version change in minor is breaking dependents #33

Closed
PeeHaa opened this issue Aug 1, 2017 · 21 comments
Closed

Minimum PHP version change in minor is breaking dependents #33

PeeHaa opened this issue Aug 1, 2017 · 21 comments
Assignees
Labels

Comments

@PeeHaa
Copy link

PeeHaa commented Aug 1, 2017

I am using phpunit on PHP 7.0.n. The last PHP version bump breaks this sebastianbergmann/phpunit-mock-objects#371

If we could not bump minimum PHP versions in minors that would be great. ❤️

@mikeSimonson
Copy link
Contributor

http://doctrine-project.org/2017/07/25/php-7.1-requirement-and-composer.html

@PeeHaa
Copy link
Author

PeeHaa commented Aug 1, 2017

k. But the fact of the matter is that it actually breaks stuff.

I very much strongly disagree with the Why dropping PHP support in a minor version is not a BC break statement.

I had code that works. And not it stops working. That's a breaking change.

From the page you linked:

For example, changing a method signature in a minor version is a no-go, since the composer version constraints mentioned above assume any minor upgrade can safely be used.

Bumping the PHP version resulted in exactly the same problem. It cannot be updated.

@Ocramius
Copy link
Member

Ocramius commented Aug 1, 2017

@PeeHaa dependency version bumps are not BC breaks. Also see https://github.com/mojombo/semver/blob/df7bd79bda7d7fe6da20d0724fe0111678cbaa8f/semver.md#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api

@Ocramius
Copy link
Member

Ocramius commented Aug 1, 2017

I had code that works. And not it stops working. That's a breaking change.

The code still works - you are just on the previous version of this particular dependency, as the SAT dependency solver of composer doesn't upgrade to a next minor. You will still get critical fixes in 1.0.x, should they occur.

@PeeHaa
Copy link
Author

PeeHaa commented Aug 1, 2017

But they are. For my reasoning see the exact same discussion I had 2 weeks ago igorw/evenement#39.

The TL;DR is igorw/evenement#39 (comment)

The dependents do the right thing by targeting updates to at most minor which should not break things. Yet it breaks things.

@Ocramius
Copy link
Member

Ocramius commented Aug 1, 2017

@PeeHaa PHP's API is not our API - we are just depending on it as with any of the many dependencies or dev-dependencies of this package.

If you run 5.x, you will just get 1.0.x installed, according to SAT semantics.

@Ocramius
Copy link
Member

Ocramius commented Aug 1, 2017

The failure in question is caused by a composer.lock change that is incompatible with the travis environment. This kind of failure is expected and correct, as travis is emulating your "deployment environment", while a change was performed in a completely incompatible local environment.

This kind of error can happen in many scenarios:

  • local environment older than production environment
  • local environment newer than production environment
  • local environment with different extensions than production environment

That's exactly what the composer.lock is designed for: preventing this sort of SNAFUs :-)

@PeeHaa
Copy link
Author

PeeHaa commented Aug 1, 2017

FWIW I see what you people are doing (after a lengthy conversation with @Ocramius). I just don't agree with it and it will fail for more people :-)

Last thing you will ever hear me about this in this issue:

I cannot ship a .lock file anymore with my project so that:

  • to ensure that the package versions are consistent for everyone working on your project

  • it ensures that your project does not break because of unexpected changes in dependencies.

composer docs

Sorry for me keep going at it. I just can see the problems that come from this decision.

Thanks for your patience o/

@Majkl578
Copy link
Contributor

Majkl578 commented Aug 1, 2017

I just can see the problems that come from this decision.

The problem here is that people use Composer in a wrong way, not that PHP version requirement was bumped. We can just hope this will educate more people and they'll learn that using different versions on dev/ci/staging/production a dangerous idea...

@PeeHaa
Copy link
Author

PeeHaa commented Aug 1, 2017

How can I make sure people downloading my project from github are installing the known and tested dependencies?

Everybody keep telling me I am composering wrong, but nobody can give me a sane solution for how to composer right when I want people to install the tested deps.

Now I am really done trying to convince people :-) I have heard and understood your side and I have said what I wanted.

@Ocramius
Copy link
Member

Ocramius commented Aug 1, 2017 via email

@lcobucci
Copy link
Member

lcobucci commented Aug 1, 2017

@PeeHaa as mentioned on the blog post you can define the platform on composer.json, this will make the resolver work properly.

@mikefrancis
Copy link

mikefrancis commented Aug 2, 2017

For anyone here having the same issue, I had developed a package on a PHP 7.1 box and our infrastructure is still running CI on 7.0, so Travis was failing.

To avoid a lengthy downgrade process and get around this, I did composer require doctrine/instantiator:~1.0.0.

Then you can either remove the dependency from your composer.json but only if you commit your lock file.

Edit: see @alcaeus comment below.

@alcaeus
Copy link
Member

alcaeus commented Aug 2, 2017

@mikefrancis or, alternatively, you don't commit a lock file that was generated on 7.1 if you're going to run code on 7.0. As suggested in the blog post, the correct way to handle these platform differences is by using the config.platform setting in composer.json, not arbitrarily restricting package versions.

@mikefrancis
Copy link

@alcaeus that's a much better solution

@Ocramius
Copy link
Member

Ocramius commented Aug 2, 2017

Locking single dependencies is also not the correct approach: if you know your deployment environment, lock onto that. Otherwise don't lock at all ( = you don't know the deployment system specifics)

@PeeHaa
Copy link
Author

PeeHaa commented Aug 2, 2017

@alcaeus that only works sanely if there aren't any other packages pulling this same thing.

@alcaeus
Copy link
Member

alcaeus commented Aug 2, 2017

@PeeHaa platform.config is relevant on the root level. If somebody else pulls in your library, its their job to make sure the platform settings are correct, not yours.

@PeeHaa
Copy link
Author

PeeHaa commented Aug 2, 2017

I don't have a library. I have a project.

@Ocramius
Copy link
Member

Ocramius commented Aug 2, 2017 via email

@PeeHaa
Copy link
Author

PeeHaa commented Aug 2, 2017

I know. We already discussed. Just noting that said solution may not be a solution, because people refuse to see it for some reason. Anyway unsubbed from thread. Sorry for bothering you all :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants