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

Do not depend on phpspec/prophecy #5033

Closed
staabm opened this issue Aug 22, 2022 · 20 comments
Closed

Do not depend on phpspec/prophecy #5033

staabm opened this issue Aug 22, 2022 · 20 comments
Labels
type/change-in-dependency-requires-adaptation A change in a dependency requires a change in PHPUnit

Comments

@staabm
Copy link
Contributor

staabm commented Aug 22, 2022

we are using phpunit and don't use mocks. therefore we don't need phpspec/prophecy, which is a requirement for phpunit atm.

we would like to use phpunit on php8.2 but composer let us not install it because phpspec/prophecy seems to not yet support php 8.2.

Error: Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - phpunit/phpunit[9.4.1, ..., 9.5.22] require phpspec/prophecy ^1.12.1 -> satisfiable by phpspec/prophecy[1.12.1, ..., v1.15.0].
    - phpunit/phpunit[9.3.0, ..., 9.4.0] require phpspec/prophecy ^1.11.1 -> satisfiable by phpspec/prophecy[1.11.1, ..., v1.15.0].
    - phpunit/phpunit[9.0.0, ..., 9.2.6] require php ^7.3 -> your php version (8.2.0-dev) does not satisfy that requirement.
    - phpspec/prophecy[1.11.0, ..., 1.11.1] require php ^7.2 -> your php version (8.2.0-dev) does not satisfy that requirement.
    - phpspec/prophecy[1.12.0, ..., 1.13.0] require php ^7.2 || ~8.0, <8.1 -> your php version (8.2.0-dev) does not satisfy that requirement.
    - phpspec/prophecy[1.14.0, ..., v1.15.0] require php ^7.2 || ~8.0, <8.2 -> your php version (8.2.0-dev) does not satisfy that requirement.
    - Root composer.json requires phpunit/phpunit ^9 -> satisfiable by phpunit/phpunit[9.0.0, ..., 9.5.22].

atm we can work around the problem using --ignore-platform-req=php+ composer cli switch.

I guess our situation is similar for other phpunit uses which also don't use mocks...

do you think its feasible to drop this dependency - or at least consider it optional in future phpunit versions?

thanks for the great tool.

@staabm staabm added the type/enhancement A new idea that should be implemented label Aug 22, 2022
@sebastianbergmann
Copy link
Owner

This has been discussed in #4141 and #4142.

TL;DR: The dependency on Prophecy has been removed for PHPUnit 10. Anything related to out-of-the-box support for Prophecy is deprecated since PHPUnit 9.1.

If we remove the dependency on phpspec/prophecy in the composer.json of PHPUnit 8.5 and PHPUnit 9.5, then PHPUnit would continue to work as-is for any developer who does not use Prophecy (through TestCase::prophesize()).

For developers who use TestCase::prophesize() in their tests, PHPUnit will stop to work unexpectedly. If they add a dependency on phpspec/prophecy in the composer.json of their project then their tests should work again.

Of course, we should be able to detect in TestCase::prophesize() whether Prophecy is available and trigger an error with a descriptive message ("Please add a dependency on phpspec/prophecy to your composer.json").

However, I do not know what to do here. If we do not do anything, PHPUnit 8.5 and PHPUnit 9.5 cannot be installed without hassle until Prophecy allows PHP 8.2 in their composer.json. And PHP 8.3 next year, and so on. If we drop the dependency on Prophecy for PHPUnit 8.5 and PHPUnit 9.5, even with a nice error message, then we break the tests for everyone who uses TestCase::prophesize(), and in a bugfix release, no less.

@sebastianbergmann sebastianbergmann added type/change-in-dependency-requires-adaptation A change in a dependency requires a change in PHPUnit and removed type/enhancement A new idea that should be implemented labels Aug 22, 2022
@sebastianbergmann sebastianbergmann changed the title decouple phpspec/prophecy from phpunit Do not depend on phpspec/prophecy Aug 22, 2022
@staabm
Copy link
Contributor Author

staabm commented Aug 22, 2022

thanks for the in-detail anwser. great to see this is a solved problem for phpunit 10+.

after reading your reply I get the feeling the easierst way forward, without BC breaks etc, would be to make people aware of the workaround, namely using the --ignore-platform-req=php+ composer cli switch on php 8.2.

@sebastianbergmann
Copy link
Owner

According to phpspec/prophecy#556 (comment), the following also works:

"replace": { "phpspec/prophecy": "*" }

@sebastianbergmann
Copy link
Owner

Also note that you do not suffer from this problem if you use PHPUnit from a PHP Archive (PHAR) instead of installing it using Composer.

@sebastianbergmann
Copy link
Owner

"replace": { "phpspec/prophecy": "*" }

Never used "replace" before today, but I can confirm that the above works (as in: it installs PHPUnit without Prophecy and Prophecy's dependencies):

$ composer init
Welcome to the Composer config generator  

This command will guide you through creating your composer.json config.

Package name (<vendor>/<name>) [sebastianbergmann/project]: example/test
Description []: 
Author [Sebastian Bergmann <sb@sebastian-bergmann.de>, n to skip]: n
Minimum Stability []: 
Package Type (e.g. library, project, metapackage, composer-plugin) []: project
License []: 

Define your dependencies.

Would you like to define your dependencies (require) interactively [yes]? no
Would you like to define your dev dependencies (require-dev) interactively [yes]? 
Search for a package: phpunit/phpunit
Enter the version constraint to require (or leave blank to use the latest version): ^9.5
Search for a package: 
Add PSR-4 autoload mapping? Maps namespace "Example\Test" to the entered relative path. [src/, n to skip]: n

{
    "name": "example/test",
    "type": "project",
    "require-dev": {
        "phpunit/phpunit": "^9.5"
    },
    "require": {}
}

Do you confirm generation [yes]? 
Would you like to install dependencies now [yes]? 
Loading composer repositories with package information
Updating dependencies
Lock file operations: 33 installs, 0 updates, 0 removals
  - Locking doctrine/instantiator (1.4.1)
  - Locking myclabs/deep-copy (1.11.0)
  - Locking nikic/php-parser (v4.14.0)
  - Locking phar-io/manifest (2.0.3)
  - Locking phar-io/version (3.2.1)
  - Locking phpdocumentor/reflection-common (2.2.0)
  - Locking phpdocumentor/reflection-docblock (5.3.0)
  - Locking phpdocumentor/type-resolver (1.6.1)
  - Locking phpspec/prophecy (v1.15.0)
  - Locking phpunit/php-code-coverage (9.2.16)
  - Locking phpunit/php-file-iterator (3.0.6)
  - Locking phpunit/php-invoker (3.1.1)
  - Locking phpunit/php-text-template (2.0.4)
  - Locking phpunit/php-timer (5.0.3)
  - Locking phpunit/phpunit (9.5.22)
  - Locking sebastian/cli-parser (1.0.1)
  - Locking sebastian/code-unit (1.0.8)
  - Locking sebastian/code-unit-reverse-lookup (2.0.3)
  - Locking sebastian/comparator (4.0.6)
  - Locking sebastian/complexity (2.0.2)
  - Locking sebastian/diff (4.0.4)
  - Locking sebastian/environment (5.1.4)
  - Locking sebastian/exporter (4.0.4)
  - Locking sebastian/global-state (5.0.5)
  - Locking sebastian/lines-of-code (1.0.3)
  - Locking sebastian/object-enumerator (4.0.4)
  - Locking sebastian/object-reflector (2.0.4)
  - Locking sebastian/recursion-context (4.0.4)
  - Locking sebastian/resource-operations (3.0.3)
  - Locking sebastian/type (3.0.0)
  - Locking sebastian/version (3.0.2)
  - Locking theseer/tokenizer (1.2.1)
  - Locking webmozart/assert (1.11.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 33 installs, 0 updates, 0 removals
  - Downloading phpunit/phpunit (9.5.22)
  - Installing webmozart/assert (1.11.0): Extracting archive
  - Installing phpdocumentor/reflection-common (2.2.0): Extracting archive
  - Installing phpdocumentor/type-resolver (1.6.1): Extracting archive
  - Installing phpdocumentor/reflection-docblock (5.3.0): Extracting archive
  - Installing sebastian/version (3.0.2): Extracting archive
  - Installing sebastian/type (3.0.0): Extracting archive
  - Installing sebastian/resource-operations (3.0.3): Extracting archive
  - Installing sebastian/recursion-context (4.0.4): Extracting archive
  - Installing sebastian/object-reflector (2.0.4): Extracting archive
  - Installing sebastian/object-enumerator (4.0.4): Extracting archive
  - Installing sebastian/global-state (5.0.5): Extracting archive
  - Installing sebastian/exporter (4.0.4): Extracting archive
  - Installing sebastian/environment (5.1.4): Extracting archive
  - Installing sebastian/diff (4.0.4): Extracting archive
  - Installing sebastian/comparator (4.0.6): Extracting archive
  - Installing sebastian/code-unit (1.0.8): Extracting archive
  - Installing sebastian/cli-parser (1.0.1): Extracting archive
  - Installing phpunit/php-timer (5.0.3): Extracting archive
  - Installing phpunit/php-text-template (2.0.4): Extracting archive
  - Installing phpunit/php-invoker (3.1.1): Extracting archive
  - Installing phpunit/php-file-iterator (3.0.6): Extracting archive
  - Installing theseer/tokenizer (1.2.1): Extracting archive
  - Installing nikic/php-parser (v4.14.0): Extracting archive
  - Installing sebastian/lines-of-code (1.0.3): Extracting archive
  - Installing sebastian/complexity (2.0.2): Extracting archive
  - Installing sebastian/code-unit-reverse-lookup (2.0.3): Extracting archive
  - Installing phpunit/php-code-coverage (9.2.16): Extracting archive
  - Installing doctrine/instantiator (1.4.1): Extracting archive
  - Installing phpspec/prophecy (v1.15.0): Extracting archive
  - Installing phar-io/version (3.2.1): Extracting archive
  - Installing phar-io/manifest (2.0.3): Extracting archive
  - Installing myclabs/deep-copy (1.11.0): Extracting archive
  - Installing phpunit/phpunit (9.5.22): Extracting archive
3 package suggestions were added by new dependencies, use `composer suggest` to see details.
Generating autoload files
25 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
No security vulnerability advisories found
$ cat composer.json
{
    "name": "example/test",
    "type": "project",
    "require-dev": {
        "phpunit/phpunit": "^9.5"
    },
    "require": {},
    "replace": { "phpspec/prophecy": "*" }
}
$ composer update  
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 0 updates, 5 removals
  - Removing phpdocumentor/reflection-common (2.2.0)
  - Removing phpdocumentor/reflection-docblock (5.3.0)
  - Removing phpdocumentor/type-resolver (1.6.1)
  - Removing phpspec/prophecy (v1.15.0)
  - Removing webmozart/assert (1.11.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 0 updates, 5 removals
  - Removing webmozart/assert (1.11.0)
  - Removing phpspec/prophecy (v1.15.0)
  - Removing phpdocumentor/type-resolver (1.6.1)
  - Removing phpdocumentor/reflection-docblock (5.3.0)
  - Removing phpdocumentor/reflection-common (2.2.0)
Generating autoload files
25 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
No security vulnerability advisories found

sebastianbergmann added a commit to sebastianbergmann/phpunit-documentation-english that referenced this issue Aug 22, 2022
@sebastianbergmann
Copy link
Owner

make people aware of the workaround, namely using the --ignore-platform-req=php+ composer cli switch on php 8.2.

I have added a section on this to the documentation for PHPUnit 8.5 and PHPUnit 9.5.

@sebastianbergmann
Copy link
Owner

sebastianbergmann commented Aug 22, 2022

This would be the patch for the 9.5 branch to remove the dependency on Prophecy:

If we released a PHPUnit 9.5.23, for instance, with these changes then

  • PHPUnit would not pull in Prophecy and can therefore be installed without workaround on PHP 8.2
  • nothing would change for developers who do not use TestCase::prophesize()
  • errors would be triggered for tests that use TestCase::prophesize()

Developers that use TestCase::prophesize() can restore the current behaviour by adding "phpspec/prophecy": "^1.12.1" to their project's composer.json file.

This would not result in an API BC break, but it would make a composer.json change necessary for developers that use Prophecy. I have no idea how many developers would be affected by this change.

Thoughts?

PS: If we do this, then these documentation changes need to be reverted:

@oliverklee
Copy link
Sponsor Contributor

I would be fine with this approach (dropping the dependency).

@localheinz
Copy link
Collaborator

@sebastianbergmann

To clarify, I would not recommend to replace dependencies in packages: when a package A requires a package B, and package B replaces package C, then package C will not be available for package A, and of course, neither for dependents of package A.

It's totally fine to replace packages in applications, though!

@RageZBla
Copy link

Same dropping the dependencies is fine by me even if means I need to declare it explicitly (in my composer.json) to use Prophecy.

@sebastianbergmann as always thanks for for your work.

@Pierstoval
Copy link

Throwing an error saying something like You are using Prophecy in the "%s" test class. Out-of-the-box support for Prophecy stopped at phpunit X.Y, please run "composer install ..." To fix this issue seems like the best solution to me.

As said above by other peers, it would have zero impact on devs that don't use prophecy (like me), and will be part of the natural upgrade process for devs using it and migrating to newer versions of phpunit

@sebastianbergmann
Copy link
Owner

Do you think that

This test uses TestCase::prophesize(), but phpspec/prophecy is not installed. Please run "composer require --dev phpspec/prophecy ^1.12.1".

good enough as an error message?

@acelaya
Copy link
Sponsor

acelaya commented Aug 22, 2022

As a user that's still using prophecy for mocks, I would say it's fine to remove the dependency from PHPUnit.

  • It's a dev dependency, which means the error will show up for everyone early enough in the development process.
  • Solving it is super straightforward (as long as the error message is clear), so even if caught during CI just before a deployment or similar, the change to fix it is quick to do and push.
  • The usage of TestCase::prophesize, was already throwing a warning since a couple of versions ago, which means most people using it probably migrated to something like phpspec/prophecy-phpunit, which already depends on phpspec/prophecy itself.

@sebastianbergmann
Copy link
Owner

Thank you all for your valuable input. These changes will be implemented for PHPUnit 8.5.29 and PHPUnit 9.5.23.

sebastianbergmann added a commit to sebastianbergmann/phpunit-documentation-english that referenced this issue Aug 22, 2022
@sebastianbergmann
Copy link
Owner

These changes will be implemented for PHPUnit 8.5.29 and PHPUnit 9.5.23.

You do not have to wait for these new releases to add phpspec/prophecy to your project's dependencies (in case you actually use TestCase::prophesize()).

@staabm
Copy link
Contributor Author

staabm commented Aug 22, 2022

thanks to everyone involved. great to see a fast fix for this problem. great job!

@spresnac
Copy link

This would be the patch for the 9.5 branch to remove the dependency on Prophecy:

If we released a PHPUnit 9.5.23, for instance, with these changes then

* PHPUnit would not pull in Prophecy and can therefore be installed without workaround on PHP 8.2

* nothing would change for developers who do not use `TestCase::prophesize()`

* errors would be triggered for tests that use `TestCase::prophesize()`

Developers that use TestCase::prophesize() can restore the current behaviour by adding "phpspec/prophecy": "^1.12.1" to their project's composer.json file.

This would not result in an API BC break, but it would make a composer.json change necessary for developers that use Prophecy. I have no idea how many developers would be affected by this change.

Thoughts?

PS: If we do this, then these documentation changes need to be reverted:

* [sebastianbergmann/phpunit-documentation-english@744a796](https://github.com/sebastianbergmann/phpunit-documentation-english/commit/744a796800aad6434665db85cd2e4dc7e306866d)

* [sebastianbergmann/phpunit-documentation-english@e0df579](https://github.com/sebastianbergmann/phpunit-documentation-english/commit/e0df5790d336218531a12ae63c936c23ee24c723)

This is okay for me as long as the docs refelct this in a clear and visible way.
I mean, even some popular frameworks do things like that and as long as it is well communicated and well documentated, I am fine with it

@sebastianbergmann
Copy link
Owner

This is okay for me as long as the docs refelct this in a clear and visible way.

Updating the documentation to reflect this change is the next thing on my TODO :)

sebastianbergmann added a commit to sebastianbergmann/phpunit-documentation-english that referenced this issue Aug 22, 2022
@lulco
Copy link

lulco commented Aug 22, 2022

According to phpspec/prophecy#556 (comment), the following also works:

"replace": { "phpspec/prophecy": "*" }

This works for me only if I have only one composer dependency with this “hack” installed.

@Levivb
Copy link

Levivb commented Aug 22, 2022

Do you think that

This test uses TestCase::prophesize(), but phpspec/prophecy is not installed. Please run "composer require --dev phpspec/prophecy ^1.12.1".

good enough as an error message?

Perhaps add a link to this very issue in the error message for developers wanting more (background) information?

Something like:
This test uses TestCase::prophesize(), but phpspec/prophecy is not installed. Please run "composer require --dev phpspec/prophecy ^1.12.1". See https://github.com/sebastianbergmann/phpunit/issues/5033 for more information

Not impacted by this change myself, but it could give those impacted a bit more to go on :)

sanmai added a commit to sanmai/php-markdown that referenced this issue Sep 5, 2022
athos-ribeiro added a commit to athos-ribeiro/DeepCopy that referenced this issue Sep 5, 2022
sebastianbergmann/phpunit#5033 changed phpunit
to no longer depend on phpspec/prophecy. This results in the need to add
a development dependency for phpspec/prophecy. Doing so results in a
warning, since PHPUnit\Framework\TestCase::prophesize() is deprecated
and will be removed in PHPUnit 10. Let's use the trait provided by
phpspec/prophecy-phpunit instead.

* Fixes myclabs#177
athos-ribeiro added a commit to athos-ribeiro/php-mock-phpunit that referenced this issue Sep 6, 2022
sebastianbergmann/phpunit#5033 changed phpunit
to no longer depend on phpspec/prophecy. This results in the need to add
a development dependency for phpspec/prophecy. Doing so results in a
warning, since PHPUnit\Framework\TestCase::prophesize() is deprecated
and will be removed in PHPUnit 10. While we could use the trait provided
by phpspec/prophecy-phpunit instead, it would break PHP < 7.3 support.

* Closes php-mock#50
michalbundyra pushed a commit to php-mock/php-mock-phpunit that referenced this issue Sep 7, 2022
* Require phpspec/prophecy as dev dependency

sebastianbergmann/phpunit#5033 changed phpunit
to no longer depend on phpspec/prophecy. This results in the need to add
a development dependency for phpspec/prophecy. Doing so results in a
warning, since PHPUnit\Framework\TestCase::prophesize() is deprecated
and will be removed in PHPUnit 10. While we could use the trait provided
by phpspec/prophecy-phpunit instead, it would break PHP < 7.3 support.

* Closes #50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/change-in-dependency-requires-adaptation A change in a dependency requires a change in PHPUnit
Projects
None yet
Development

No branches or pull requests

10 participants