Prefix all code bundled in PHAR distribution with unique namesspaces

[kambo-1st]

This is a proof of concept for the #2015 it's by no means a final solution. For further detail please look at comments for #2015.

[theofidry]

Thanks for working on this @kambo-1st. I'll try to get back to PHP-Scoper this week and push a bit forward things there. Meanwhile don't hesitate to open issues on the repo for the problems encountered.

I also highly recommend to have some e2e tests using the scoped PHAR if that's not the case trying out some cases relying on shared constants or interfaces. You have some examples on how I did it in PHP-Scoper itself.

If you need to ping me directly the easiest way is Twitter/Slack

[sebastianbergmann]

@kambo-1st @theofidry Has there been progress? Thanks!

[theofidry]

I'm hacking on the ongoing issues on PHP-Scoper. However I don't think any of them are blockers to get a working isolated PHAR; just for the merge as you don't want something too ugly/brittle merged.

For the rest sorry I'm not aware of any progress, maybe @kambo-1st was a bit busy last week :)

[kambo-1st]

@theofidry is right, there aren’t any blocking issues in PHP-Scoper for building working phar file (at least at this moment), but I indeed not happy about “proof of concept” solution proposed in this pull request. From my point of view humbug/php-scoper#192 will be need for the final version. In the meantime, I will prepare some e2e tests for the phar file, there is still a huge probability of some other issues.

[codecov-io]

Codecov Report

Merging #3086 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #3086   +/-   ##
========================================
  Coverage      81.5%   81.5%           
  Complexity     3412    3412           
========================================
  Files           137     137           
  Lines          9014    9014           
========================================
  Hits           7347    7347           
  Misses         1667    1667

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef4c347...429d533. Read the comment docs.

[sebastianbergmann]

@kambo-1st Thank you. I agree that we should wait for a release that includes humbug/php-scoper#192.

[theofidry]

Whilst humbug/php-scoper#192 is required for the final version, @kambo-1st found an acceptable workaround for now. So I would suggest to keep going on the progress as I suspect there is a lot more to do still

[theofidry]

are those classes or constants?

[kambo-1st]

They are classes without any namespace.

[sebastianbergmann]

php-token-stream unfortunately does not use namespaces.

@theofidry Would it be possible to add support for whitelisting classes using PHP_Token_* or something along those lines?

[theofidry]

But do they need to be whitelisted?

[theofidry]

@sebastianbergmann that could be done. I opened humbug/php-scoper#227 for it

[kambo-1st]

@theofidry PHP_Token_* classes must be whitelisted, because they are instantiated dynamically (https://github.com/sebastianbergmann/php-token-stream/blob/711ca0c13c66f6b66c2ecb586e56415815034330/src/Token/Stream.php#L182). Prefixing break this.

[theofidry]

Maybe this could be fixed by having a patcher for this line instead?

[sebastianbergmann]

@theofidry Is php-scoper available as a PHAR? If so, @kambo-1st, I would prefer to have that PHAR in build/tools and used here.

[theofidry]

It should. There was a problem with the last version so it didn't get a PHAR but that shouldn't happen anymore.

However I would recommend to switch to a PHAR at the last moment since it makes it much harder to debug/fix in the meantime.

[sebastianbergmann]

Sure, using Composer for the moment is fine until we are sure that 1) php-scoper can handle PHPUnit and 2) PHPUnit uses php-scoper correctly.

[kambo-1st]

I also prefer PHAR. But as @theofidry mentioned it wasn't available for latest version. When it will be available, i will use it.

[theofidry]

I'm trying to prepare 0.9 which should provide some nice features:

• Smarter function arguments scoping (Smarter function arguments scoping humbug/php-scoper#228)
• Whitelist global functions by default (Whitelist global functions by default humbug/php-scoper#231)
• Support whitelisting of functions
• Whitelist classes based on a pattern (Whitelist classes based on a pattern humbug/php-scoper#227) (depends on the previous point)
• Whitelist files (Whitelist files humbug/php-scoper#196)

Is there anything else that is blocking here or that could help?

[sebastianbergmann]

@kambo-1st What is the status here? Can you answer @theofidry's question? Thanks!

[sebastianbergmann]

@kambo-1st What is the status here? Can you answer @theofidry's question? Thanks!

[theofidry]

Quick heads up: PHP-Scoper 0.9.1 has been tagged, it provides:

• whitelisting of namespaces
• wildcard whitelisting
• does not scope global user-defined symbols by default

which should make all of your work easier :)

[sebastianbergmann]

Thank you, @theofidry. I hope to hear from @kambo-1st soon. Otherwise I'll have to look into this myself again.

[kambo-1st]

Sorry for keep you waiting. I'll look into it this week.

[kambo-1st]

Quick update, new features in scooper look promising, especially wildcards. At this moment I preparing some tests.

[sebastianfeldmann]

I tried to follow up on this pull request and encountered an issue with php-scoper.
I opened an issue for it humbug/php-scoper#261

The rest seems to work.

[kambo-1st]

@sebastianfeldmann Interesting issue, it seems, that this does not have any effect on the builded phar file, at least on the first sight. This is because there is still a proper alias defininition on the end of the file.

\class_alias('PHPUnit\\PHPUnit_Framework_MockObject_MockObject', 'PHPUnit_Framework_MockObject_MockObject', \false);

[sebastianfeldmann]

For me executing the PHAR fails instantly because the Mock class can not be found

[theofidry]

Should be better with https://github.com/humbug/php-scoper/releases/tag/0.11.1

[sebastianfeldmann]

I can't see any difference :(

$whitelistClasses = [
    'PHPUnit\*',
    'SebastianBergmann\CodeCoverage\*',
    'PharIo\*',
    'PHP_Token*',
];
return [
    'whitelist' => $whitelistClasses,
];

and php-scoper still scopes PHP_Token and following.

<?php
namespace PHPUnit;
/**
 * A PHP token.
 */
abstract class PHP_Token
{
...
}
\class_alias('PHPUnit\\PHP_Token', 'PHP_Token', \false);

I'm not sure we are looking for the same issue. Can we agree, that with the given whitelist this class should not be prefixed with the PHPUnit namespace?

[theofidry]

Arg forgot to check if the whildcard works on global classes like that. That said I would argue you should be able to do without: the class aliasing should do the trick and doing without is really tricky

[theofidry]

I've had a very quick look and will try to dig into it more, but from what I see so far:

• The whitelisting works including for the PHP_Token classes: I see the class alias statements and aliasing, however you were missing the "load scoper-autoload.php
• I'm really not sure to understand the current whitelisting configuration: with 'PHPUnit\*' you are whitelisting the whole PHPUnit namespace including the runner and several utility classes. Shouldn't they be scoped as well and expose only the API used by the user?

[theseer]

@theofidry Just out of curiosity: Why would, for the phar which uses a static require list, the scoper-autoload.php be needed? And even if it wouldn't be a static require list but an actual autoload being based on a classmap?

[theofidry]

For classes if you were to require all the files it wouldn’t be an issue, but it would still be for functions (cf the PHPScoper doc I can’t link it atm). That said for now I’m trying to make it work outside the PHAR first once it’s done it will be easy to tell where the proble comes from.

…

On Sun 11 Nov 2018 at 14:38, Arne Blankerts ***@***.***> wrote: @theofidry <https://github.com/theofidry> Just out of curiosity: Why would, for the phar which uses a static require list, the scoper-autoload.php be needed? And even if it wouldn't be a static require list but an actual autoload being based on a classmap? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3086 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE76gcOxGkmfBXby4zZW0fgf_uMjlhoBks5uuChUgaJpZM4TLqmm> .

[theofidry]

@sebastianfeldmann I did a PR with some fixes. There is some stuff to not merge but I would keep them for now and remove them later when everything is working. My process:

• scope everything in a temporary directory (see the compile)
• go in that directory, dump the autoloader again and run the tests

I released a new version of PHP-Scoper to fix a bug related to the whitelisting feature. From what I see most of the remaining failures are due to class strings incorrectly being scoped or not scoped, if you check the diff of my PR you will see what I mean. There might be one or two failures which requires a big more of digging than that but that should already be a solid start.

Once this step is done, what is left is bundling it in the PHAR and ensuring the PHAR works as expected.

[sebastianfeldmann]

@theofidry I'm still a bit confused.
Could you explain one more time why it is the case that if I put something on the whitelist it gets prefixed anyway?
I know there are class aliases and it should work, but why bother?
The whitelisted namespace PHPUnit doesn't get prefixed into PHPUnit\PHPUnit\* and aliased.
I'm really struggling to understand this. If we could get the whitelisted classes to not get prefixed at all the problem would be solved.

PHP_Token* should stay PHP_Token* and should NOT be prefixed to PHPUnit\PHP_Token*

[theofidry]

Could you explain one more time why it is the case that if I put something on the whitelist it gets prefixed anyway?

I've added that doc entry for it, you might want to take a look: https://github.com/humbug/php-scoper/#implementation-insights

I know there are class aliases and it should work, but why bother?

A few reasons:

• Autoloading: you cannot easily mix different namespaces unless you do a "classmap": all" kind of thing
• You would need to tell the content of the file beforehand as if there is multiple classes in the file and one of the class should not be prefixed, figuring out how to wrap the two in different namespaces is really hard

Whereas always prefixing and appending class aliases when appropriate is very straightforward.

The whitelisted namespace PHPUnit doesn't get prefixed into PHPUnit\PHPUnit* and aliased.

Because whitelisting a whole namespace is easier to achieve without breaking much stuff than selectively whitelisting some elements that way. But note that it has some limitations as well cf. namespace whitelisting

I understand the feeling that always prefixing and adding a class alias feels weird, but whitelisting some elements just didn't work well enough.

[sebastianbergmann]

@sebastianfeldmann Not sure it matters, but PHPUnit_Framework_MockObject_MockObject is gone in master now.

[sebastianfeldmann]

That's good news.
Our current issue is, that the PHP_Token* classes get prefixed by php-scoper. And because of those classes getting created dynamically those dynamic instantiations fail because of the wrong namespace.
php-scoper puts the right fallback class_alias statements in place but because of the current php-ab autoloading the classes can't be loaded.
php-ab knows the classes by their new prefixed namespace, and if you instantiate them with the original class names php-ab has no knowledge about them and can't load the right file.
@theofidry was talking about using the scoper-autoload.php file, but so far I'm not seeing this file anywhere.
I'll try to hardcode the original PHP_Token* classes into the PHAR autoloader to make sure this is the only problem left to solve and if that's the case try to figure out a good way to solve it.

[theofidry]

Is it really an issue though? As I said before: before shopping the PHAR you need to make sure the scoping is done right and from what I see in the tests there is still 1 or 2 cases that should be inspected. The scoper-autoload.php file is in vendor next to the Composer autoload.php

…

On Mon 19 Nov 2018 at 14:22, Sebastian Feldmann ***@***.***> wrote: That's good news. Our current issue is, that the PHP_Token* classes get prefixed by php-scoper. And because of those classes getting created dynamically those dynamic instantiations fail because of the wrong namespace. php-scoper puts the right fallback class_alias statements in place but because of the current php-ab autoloading the classes can't be loaded. php-ab knows the classes by their new prefixed namespace, and if you instantiate them with the original class names php-ab has no knowledge about them and can't load the right file. @theofidry <https://github.com/theofidry> was talking about using the scoper-autoload.php file, but so far I'm not seeing this file anywhere. I'll try to hardcode the original PHP_Token* classes into the PHAR autoloader to make sure this is the only problem left to solve and if that's the case try to figure out a good way to solve it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3086 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE76gVG97B8IrXao7UIW8PiZgScpYWUqks5uwrCYgaJpZM4TLqmm> .

[theseer]

For the record: phpab is - afaik - only used when a phar is being build.

On top of that, the generated "autoload" is a static require list. That means, all files will be required even before one single line of other code is being executed. Any class_alias definition in the required file would thus also be executed right away.

Or in other words: There's not autoloading in phar mode. Given that phpab is only used when and for building the phar, I fail to see how phpab could cause any issues?

Or am I missing the point? ;)

[theofidry]

@theseer that was my understanding and it should make it easy. I was just pointing out that getting the scoping working outside the PHAR is needed first as it makes it easier to know where a potential issue might come from as well as allows to easily test the scoped code by running the (non-scoped) test suite against it

[sebastianfeldmann]

@theseer the problem is/was, that phpab sorted the static file list wrong. In a way that a class that depended on another class was loaded first and the require statement failed because of a missing dependency. That's why I switched to a dynamic autoloading.
The cause for this was the prefixed PHPUnit_Framework_MockObject_MockObject, but since it is gone in master I'll go back to the static file list and see if that is working again.

@theofidry currently I'm creating the scoped files with this ant task

    <exec executable="${basedir}/build/tools/php-scoper" taskname="php-scoper">
        <arg value="add-prefix" />
        <arg value="--no-ansi" />
        <arg value="--force" />
        <arg value="--config" />
        <arg path="${basedir}/build/scoper.inc.php" />
        <arg value="--no-interaction" />
        <arg value="--stop-on-failure" />
        <arg value="--output-dir" />
        <arg path="${basedir}/build/phar-scoped" />
        <arg value="--prefix" />
        <arg value="PHPUnit" />
        <arg path="${basedir}/build/phar" />
    </exec>

./build/tools/php-scoper add-prefix --no-ansi --force --config ./build/scoper.inc.php --no-interaction --stop-on-failure --output-dir ./build/phar-scoped --prefix PHPUnit ./build/phar

PHAR generation for PHPUnit works like this:

1. Copy stuff that should be included in the PHAR from vendor to build/phar
2. Add manifest file
3. Create PHAR including autoloading with phpab with the temp build/phar directory
4. Delete the build-phar directory

Now there is a 2.a where everything in build/phar is getting scoped into build/phar-scoped by php-scoper and phpab is then using the build/phar-scoped dir to create the PHAR

With the command above no scoped-autoload.php seems to be generated.

[theofidry]

I've checked this morning, with the following configuration:

scoper.inc.php

<?php
declare(strict_types=1);
/*
 * This file is part of PHPUnit.
 *
 * (c) Sebastian Bergmann <sebastian@phpunit.de>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

return [
    'whitelist-global-classes' => false,

    'whitelist' => [
        'PHPUnit\*',
        'SebastianBergmann\CodeCoverage\*',
        'PharIo\*',
        'PHP_Token*',
        'PHPUnit_Framework_MockObject_MockObject',
        'MultiDependencyTest',
    ],
];

When I use the following script:

compile.sh

#!/usr/bin/env bash

set -eof pipefail
set -x

rm -rf ../tmp || true
rm -rf scope || true
cp -r . ../tmp/
rm -rf ../tmp/tests

$PHPSCOPER_BIN add-prefix ../tmp --force --output-dir=scope --config=build/scoper.inc.php

rm -rf ../tmp
cp -r tests scope/

cd scope
composer dump-autoload
php phpunit

Which in short scope the whole project except the tests, dump the autoloader and run the scoped PHPUnit agains the tests, it mostly works. There is one failure related to an extension relying on the Compactor not being scoped which I think is a point up for discussion.

So what is left is:

• Shipping the scoped code in the PHAR
• Have a test for the scoped PHAR
• Optional but I would strongly recommend it: have a way to run the scoped code against the tests like above, be it the scoped PHAR or the scoped code alone

@sebastianfeldmann looks good to me, but I think you are missing dumping the autoloader: all the namespaces and everything changes so the Composer autoloader needs to be dumped again. PHP-Scoper could avoid that by either doing it itself (like I do in Box, but this is why I'm not keen to do it in PHP-Scoper, it is never a sure solution) or Scoper the autoloader itself (quite complex so I'm not really willing to do that) but meanwhile it is necessary otherwise the whole autoloading will be completely broken.

[theseer]

@theseer the problem is/was, that phpab sorted the static file list wrong. In a way that a class that depended on another class was loaded first and the require statement failed because of a missing dependency. That's why I switched to a dynamic autoloading.
The cause for this was the prefixed PHPUnit_Framework_MockObject_MockObject, but since it is gone in master I'll go back to the static file list and see if that is working again.

I'm not sure I understand how that would be logically possible. phpab scans a given directory for classes, interfaces and traits. It resolves dependencies and uses that for sorting the list (aka, performs a topological sort) before writing the require file. Unless I have a bug in the sorting code, there is no way processing the generated list top to bottom can cause undefined class errors.

As with autoloading this of course is a all or nothing approach. If not all units are defined and found at build time, the list will be incomplete.

I guess, I'm just trying to figure out whether or not there's something I can change or even have to fix in phpab ;)

Given that running php-scoper doesn't change anything but the names, no dependency changed. So if phpab would be broken, the problem should have been there to begin with. Again, I fail to see how scoping the names should have an effect. If the scoper doesn't rename the files, you could even generate the require list before changing the unit names...

[glensc]

@theseer I can see one scenario when static sorting fails: in the case of circular dependencies. but that would require multiple classes defined in the same file. not sure if that's the case here.

[theseer]

@theseer I can see one scenario when static sorting fails: in the case of circular dependencies. but that would require multiple classes defined in the same file. not sure if that's the case here.

Afaik, PHP can't do resolve circular dependencies on this level.

[theofidry]

It resolves dependencies and uses that for sorting the list (aka, performs a topological sort) before writing the require file. Unless I have a bug in the sorting code, there is no way processing the generated list top to bottom can cause undefined class errors.

Does this topological sorting handles class aliasing?

[theseer]

It resolves dependencies and uses that for sorting the list (aka, performs a topological sort) before writing the require file. Unless I have a bug in the sorting code, there is no way processing the generated list top to bottom can cause undefined class errors.

Does this topological sorting handles class aliasing?

No. Good point.

I guess that would require finding and "understanding" the class_alias function call. Currently the parser looks up class/interface/trait definitions as well as their extends, implements and uses.

That's a problem, or is it?

[theofidry]

I think that could be the source of the issue since almost all whitelisted code get aliased.

But since you are scanning the files first and operate a sorting, maybe adding support for the class aliasing wouldn't be too big of a deal?

[sebastianfeldmann]

After being called out yesterday by @sebastianbergmann at the php user group Frankfurt I immediately after arriving at home sat down and made some checks I had only layed out in my head so far.

So I rebased to the latest master.
Moved the latest version of php-scoper to the tools directory
Now on the ant phar build a build/phar-scoped directory gets created with all the contents of the previously prepared build/phar directory.

I used the created PHAR to run all phpunit 8 ready test suites on my machine (not much but some) and everything worked.

When I run the phpunit test suite with the created phar file I get one error.

There was 1 error:

1) PHPUnit\Framework\TestFailureTest::testExceptionToStringForExpectationFailedExceptionWithComparisonFailure
TypeError: Argument 2 passed to PHPUnit\Framework\ExpectationFailedException::__construct() must be an instance of PHPUnit\SebastianBergmann\Comparator\ComparisonFailure or null, instance of SebastianBergmann\Comparator\ComparisonFailure given, called in /Users/sf/Development/github.com/forks/phpunit/tests/unit/Framework/TestFailureTest.php on line 69

phar:///usr/local/bin/phpunit-beta/phpunit/Framework/ExpectationFailedException.php:30
/Users/sf/Development/github.com/forks/phpunit/tests/unit/Framework/TestFailureTest.php:69
phar:///usr/local/bin/phpunit-beta/phpunit/Framework/TestCase.php:938
phar:///usr/local/bin/phpunit-beta/phpunit/Framework/TestCase.php:680
phar:///usr/local/bin/phpunit-beta/phpunit/Framework/TestResult.php:559
phar:///usr/local/bin/phpunit-beta/phpunit/Framework/TestCase.php:646
phar:///usr/local/bin/phpunit-beta/phpunit/Framework/TestSuite.php:556
phar:///usr/local/bin/phpunit-beta/phpunit/Framework/TestSuite.php:556
phar:///usr/local/bin/phpunit-beta/phpunit/Framework/TestSuite.php:556
phar:///usr/local/bin/phpunit-beta/phpunit/TextUI/TestRunner.php:418
phar:///usr/local/bin/phpunit-beta/phpunit/TextUI/Command.php:101
phar:///usr/local/bin/phpunit-beta/phpunit/TextUI/Command.php:70

I'm not quite sure why exactly this one is crashing because the other tests in that class look quite similar.

You can check by creating your own scoped phar by:

$ git clone https://github.com/sebastianfeldmann/phpunit.git
$ git checkout feature/php-scoper
$ ant phar

Note to myself: social pressure works ;)

[sebastianbergmann]

It was not my intention to "call you out" and "build up pressure". Quite the opposite: I wanted to you credit for the work you did. Sorry if I messed that up.

I think it is time we closed this pull request and @sebastianfeldmann sends a new pull request with the current state of his work. This would make it a lot easier for me to work on this.

[sebastianfeldmann]

Don't worry, you messed nothing up :)
I got your intention, but in order to deserve the credit...
I will open a new pull request with the current state.

[sebastianbergmann]

Superseded by #3575.