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

Prefix all code bundled in PHAR distribution with random/unique namespace #2015

Open
sebastianbergmann opened this Issue Dec 23, 2015 · 18 comments

Comments

8 participants
@sebastianbergmann
Owner

sebastianbergmann commented Dec 23, 2015

Issues such as #2014 happen when the code under test shares dependencies with PHPUnit but requires different versions than the ones bundled in the PHAR.

Depends on https://github.com/webmozart/php-scoper being ready for real world usage.

@Soullivaneuh

This comment has been minimized.

Show comment
Hide comment
@Soullivaneuh

Soullivaneuh Dec 23, 2015

I was working on it but php-scoper is not on packagist yet. See humbug/php-scoper#12.

I'm not sure of the theseer/autoload work here, can you please elaborate? :-)

I was working on it but php-scoper is not on packagist yet. See humbug/php-scoper#12.

I'm not sure of the theseer/autoload work here, can you please elaborate? :-)

@Soullivaneuh

This comment has been minimized.

Show comment
Hide comment
@Soullivaneuh

Soullivaneuh Dec 23, 2015

AFAIK, this will partially resolve #2014.

The .phar will be fixed, but not global composer install, right?

AFAIK, this will partially resolve #2014.

The .phar will be fixed, but not global composer install, right?

@lastzero

This comment has been minimized.

Show comment
Hide comment
@lastzero

lastzero Jan 12, 2016

Contributor

Excellent idea! We just had trouble testing our Symfony 2.8 project. PHPUnit's phar (tested with 5.1.4) comes with the Yaml component of Symfony 3.0. Since Yaml::parse() does not support file names anymore, important dependencies such as Doctrine ORM are broken with this version (YAML config files can not be read anymore and you see misleading error messages). I can imagine, many developers are facing similar problems without understanding the reason. They might stop writing unit tests right in this moment. You should really fix this.

Contributor

lastzero commented Jan 12, 2016

Excellent idea! We just had trouble testing our Symfony 2.8 project. PHPUnit's phar (tested with 5.1.4) comes with the Yaml component of Symfony 3.0. Since Yaml::parse() does not support file names anymore, important dependencies such as Doctrine ORM are broken with this version (YAML config files can not be read anymore and you see misleading error messages). I can imagine, many developers are facing similar problems without understanding the reason. They might stop writing unit tests right in this moment. You should really fix this.

@sebastianbergmann

This comment has been minimized.

Show comment
Hide comment
@sebastianbergmann

sebastianbergmann Jan 12, 2016

Owner

@lastzero In a scenario such as the one you describe you have to install PHPUnit via Composer for the time being.

@lastzero In a scenario such as the one you describe you have to install PHPUnit via Composer for the time being.

@lastzero

This comment has been minimized.

Show comment
Hide comment
@lastzero

lastzero Jan 12, 2016

Contributor

@sebastianbergmann Sure, but my developers didn't figure this out on their own. It's just plain dangerous to bundle a common standard component such as the Symfony YAML parser in a version with all backwards compatibility removed. Actually, I think there was a previous release of phpunit(.phar), that checked if a vendor directory exists and only used the bundled libraries if not.

Hint for other developers: If you're facing issues like this, you can use http://php.net/manual/en/reflectionclass.getfilename.php to get the filename of a class to see if it's coming from vendor or from a phar file.

Contributor

lastzero commented Jan 12, 2016

@sebastianbergmann Sure, but my developers didn't figure this out on their own. It's just plain dangerous to bundle a common standard component such as the Symfony YAML parser in a version with all backwards compatibility removed. Actually, I think there was a previous release of phpunit(.phar), that checked if a vendor directory exists and only used the bundled libraries if not.

Hint for other developers: If you're facing issues like this, you can use http://php.net/manual/en/reflectionclass.getfilename.php to get the filename of a class to see if it's coming from vendor or from a phar file.

@ssnikiforov

This comment has been minimized.

Show comment
Hide comment
@ssnikiforov

ssnikiforov Oct 20, 2016

We have stucked with the same problem. "symfony/dependency-injection" v3.2 (master) has a dependency "use Symfony\Component\Yaml\Yaml;" in "/Loader/YamlFileLoader.php". And, also, phpunit has an implementation (hard-coded) of "Yaml" class in phpunit's phploc.php file.

Solution is to set up in your composer.json requirement of exactly "3.0" version of symphony/dependency-injection. You can type smth like this:
"symfony/dependency-injection": "3.0".

Thanks @iplus for found solution to the problem.

ssnikiforov commented Oct 20, 2016

We have stucked with the same problem. "symfony/dependency-injection" v3.2 (master) has a dependency "use Symfony\Component\Yaml\Yaml;" in "/Loader/YamlFileLoader.php". And, also, phpunit has an implementation (hard-coded) of "Yaml" class in phpunit's phploc.php file.

Solution is to set up in your composer.json requirement of exactly "3.0" version of symphony/dependency-injection. You can type smth like this:
"symfony/dependency-injection": "3.0".

Thanks @iplus for found solution to the problem.

@COil

This comment has been minimized.

Show comment
Hide comment
@COil

COil Oct 27, 2016

@ssnikiforov I tried adding symfony/dependency-injection": "3.0" but it doesn't seem to work. (with Symfony 3.2 beta1) (and php 5.5)

[17:41:43] $ composer update symfony/symfony --with-dependencies
Loading composer repositories with package information
Updating dependencies (including require-dev)                                                             
Your requirements could not be resolved to an installable set of packages.
 Problem 1
    - don't install symfony/dependency-injection v3.0.0|don't install symfony/symfony 3.2.x-dev
    - don't install symfony/dependency-injection v3.0.0|remove symfony/symfony 3.2.x-dev
    - don't install symfony/dependency-injection v3.0.0|don't install symfony/symfony 3.2.x-dev
    - Installation request for symfony/dependency-injection 3.0 -> satisfiable by symfony/dependency-injection[v3.0.0].
    - Installation request for symfony/symfony 3.2.x-dev -> satisfiable by symfony/symfony[3.2.x-dev].

COil commented Oct 27, 2016

@ssnikiforov I tried adding symfony/dependency-injection": "3.0" but it doesn't seem to work. (with Symfony 3.2 beta1) (and php 5.5)

[17:41:43] $ composer update symfony/symfony --with-dependencies
Loading composer repositories with package information
Updating dependencies (including require-dev)                                                             
Your requirements could not be resolved to an installable set of packages.
 Problem 1
    - don't install symfony/dependency-injection v3.0.0|don't install symfony/symfony 3.2.x-dev
    - don't install symfony/dependency-injection v3.0.0|remove symfony/symfony 3.2.x-dev
    - don't install symfony/dependency-injection v3.0.0|don't install symfony/symfony 3.2.x-dev
    - Installation request for symfony/dependency-injection 3.0 -> satisfiable by symfony/dependency-injection[v3.0.0].
    - Installation request for symfony/symfony 3.2.x-dev -> satisfiable by symfony/symfony[3.2.x-dev].
@YetiCGN

This comment has been minimized.

Show comment
Hide comment
@YetiCGN

YetiCGN May 22, 2017

+1

This came up during my test of Symfony 3.3.0-RC1 today. The app runs normally but PhpUnit crashes on the first test, because some new constants in the Yaml class are not defined. Turns out, the Yaml classes are all loaded from the phpunit phar and not from the vendor folder.

YetiCGN commented May 22, 2017

+1

This came up during my test of Symfony 3.3.0-RC1 today. The app runs normally but PhpUnit crashes on the first test, because some new constants in the Yaml class are not defined. Turns out, the Yaml classes are all loaded from the phpunit phar and not from the vendor folder.

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Dec 23, 2017

Contributor

👍 nice to see this happened ;)

hm... doesn't look like a bc breaker, why pushed to 7.0 instead of 6.x ?

Contributor

keradus commented Dec 23, 2017

👍 nice to see this happened ;)

hm... doesn't look like a bc breaker, why pushed to 7.0 instead of 6.x ?

sebastianbergmann added a commit that referenced this issue Dec 23, 2017

Revert "Closes #2015"
This reverts commit defa9b9.
@sebastianbergmann

This comment has been minimized.

Show comment
Hide comment
@sebastianbergmann

sebastianbergmann Dec 23, 2017

Owner

defa9b9 had to be reverted because of regressions. After the holidays I will have another go at it.

defa9b9 had to be reverted because of regressions. After the holidays I will have another go at it.

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Dec 23, 2017

Contributor

Could you please share some insights @sebastianbergmann please? What was the issue?
It would be interesting for anyone thinking about prefixing the phar build.

Contributor

keradus commented Dec 23, 2017

Could you please share some insights @sebastianbergmann please? What was the issue?
It would be interesting for anyone thinking about prefixing the phar build.

@sebastianbergmann

This comment has been minimized.

Show comment
Hide comment
@sebastianbergmann

sebastianbergmann Dec 24, 2017

Owner

"After the holidays I will have another go at it." means that I will investigate next week and share my findings here. One of the issues found is/was documented in #2939.

"After the holidays I will have another go at it." means that I will investigate next week and share my findings here. One of the issues found is/was documented in #2939.

sebastianbergmann added a commit that referenced this issue Dec 24, 2017

Revert "Revert "Closes #2015""
This reverts commit a19b93d.
@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus Dec 26, 2017

Contributor

yeah, I was not wondering about solution, but about what was the issue :D thanks ;)

Contributor

keradus commented Dec 26, 2017

yeah, I was not wondering about solution, but about what was the issue :D thanks ;)

@Soullivaneuh

This comment has been minimized.

Show comment
Hide comment
@Soullivaneuh

Soullivaneuh Jan 31, 2018

Just a side note: Only vendor should be prefixed, not PHPUnit classes to avoid confusion on IDE completion for example. 👍

Just a side note: Only vendor should be prefixed, not PHPUnit classes to avoid confusion on IDE completion for example. 👍

@kambo-1st

This comment has been minimized.

Show comment
Hide comment
@kambo-1st

kambo-1st Apr 8, 2018

Contributor

I worked on this issue on the PHPUnit Code Sprint in Hamburg, specifically on the regression described in #2939. Reason of this regression was an issue in the php-scoper. Fortunately this issue has been fixed in latest release of the php-scoper (for reference it was the pull request humbug/php-scoper#167).

But the new version of php-scoper introduce another problems. For example, if the test depended on any of prefixed classed in the phar file eg.: it use class PHPUnit_Framework_MockObject_MockObject in a return type definition, it will break. For these cases there is a configuration option in the php-scoper, which allows to "whitelist" particular classes. By "whitelisting" it's meant, that the for the prefixed class an alias with original name will be created. Whitelisted classes can be specified only by their fully qualified class names and nothing else. Usage of this feature thus mean creation of the class list by hand or in automatic manner, for example by using composer classmap. Which is not very robust. To solve this, an issue for the php-scoper has been created (humbug/php-scoper#192) with propose of whitelisting according namespace names or (FQCN) class name prefixes.

In the meantime I prepare a solution, which is by no means meant as a final one (its actualy a proof of concept). There is a pull request #3086 for it. At this moment i am omitting files in following folders:

  • src/Framework
  • phpunit/phpunit-mock-objects,
  • phpunit/php-code-coverage and
  • phpunit/php-token-stream - This must be also whitelisted, because of the dynamic object instantiation in Stream.php
Contributor

kambo-1st commented Apr 8, 2018

I worked on this issue on the PHPUnit Code Sprint in Hamburg, specifically on the regression described in #2939. Reason of this regression was an issue in the php-scoper. Fortunately this issue has been fixed in latest release of the php-scoper (for reference it was the pull request humbug/php-scoper#167).

But the new version of php-scoper introduce another problems. For example, if the test depended on any of prefixed classed in the phar file eg.: it use class PHPUnit_Framework_MockObject_MockObject in a return type definition, it will break. For these cases there is a configuration option in the php-scoper, which allows to "whitelist" particular classes. By "whitelisting" it's meant, that the for the prefixed class an alias with original name will be created. Whitelisted classes can be specified only by their fully qualified class names and nothing else. Usage of this feature thus mean creation of the class list by hand or in automatic manner, for example by using composer classmap. Which is not very robust. To solve this, an issue for the php-scoper has been created (humbug/php-scoper#192) with propose of whitelisting according namespace names or (FQCN) class name prefixes.

In the meantime I prepare a solution, which is by no means meant as a final one (its actualy a proof of concept). There is a pull request #3086 for it. At this moment i am omitting files in following folders:

  • src/Framework
  • phpunit/phpunit-mock-objects,
  • phpunit/php-code-coverage and
  • phpunit/php-token-stream - This must be also whitelisted, because of the dynamic object instantiation in Stream.php
@lastzero

This comment has been minimized.

Show comment
Hide comment
@lastzero

lastzero Apr 22, 2018

Contributor

Wow, this all seems to be a lot of work. Did you check how many classes outside the PHPUnit scope are actually used? Would it be an option to simply "fork" them, let's say if it's just the YAML parser? Or use a simpler YAML parser that can be bundled with PHPUnit?

Contributor

lastzero commented Apr 22, 2018

Wow, this all seems to be a lot of work. Did you check how many classes outside the PHPUnit scope are actually used? Would it be an option to simply "fork" them, let's say if it's just the YAML parser? Or use a simpler YAML parser that can be bundled with PHPUnit?

@keradus

This comment has been minimized.

Show comment
Hide comment
@keradus

keradus May 14, 2018

Contributor

@sebastianbergmann , sounds like perfect candidate to whitelist all classes except those marked as @internal.
maybe it's good occasion to add that @internal for classes that were not meant to be public, not meant to be under BC promise? I can prepare a PR which would mark all classes as @internal, and then we would publish those which shall be public ?

Contributor

keradus commented May 14, 2018

@sebastianbergmann , sounds like perfect candidate to whitelist all classes except those marked as @internal.
maybe it's good occasion to add that @internal for classes that were not meant to be public, not meant to be under BC promise? I can prepare a PR which would mark all classes as @internal, and then we would publish those which shall be public ?

@sebastianbergmann

This comment has been minimized.

Show comment
Hide comment
@sebastianbergmann

sebastianbergmann May 14, 2018

Owner

I would rather not mix these two topics.

I would rather not mix these two topics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment