humbug/php-scoper

[Scoper] Improve the WP support experience

wujekbogdan opened this issue · 111 comments

wujekbogdan commented

Edit: @matzeeable has created WP React Starter which helps with this. However a cleaner extension point has been found and could help out as well outside of WP. Check #303 (comment) for more details

the whitelist-global-functions setting doesn't seem to work... or maybe I don't understand how it works. I'm trying to prefix a WordPress plugin. The whitelist-global-functions setting is set to true but global functions are prefixed anyway.

For example this:

add_action('admin_print_scripts', [$this, 'admin_enqueue_scripts']);

Becames this:

\_PhpScoper5c6c3100152e6\add_action('admin_print_scripts', [$this, 'admin_enqueue_scripts']);
theofidry commented

Yes it is expected, you find more details here. So the function is still scoped, but you should find a statement in scoper-autoload.php aliasing the scoped function to the original one

vielhuber commented

Is including vendor/scoper-autoload.php a required step when using php-scoper? I've seen it nowhere in the documentation and wonder how I should do it in the original file (with file_exists?)

  • 👍1
vielhuber commented

Another question on this: It seems that in this case it's the other way around.

test1.php (that is outside of php-scoper):

<?php
function update_option()
{
    echo 'FOO';
}
require_once 'test2.php';

test2.php (that php-scoper changed):

<?php
namespace _PhpScoper282d0d739942;
require_once __DIR__ . '/vendor/scoper-autoload.php';
\_PhpScoper282d0d739942\update_option();

scoper-autoload.php

if (!function_exists('update_option')) {
    function update_option() {
        return \_PhpScoper282d0d739942\update_option(...func_get_args());
    }
}

The problem is, that if I run test1.php, I get "Call to undefined function _PhpScoper282d0d739942\update_option()", because you alias the update_options function (just the other way round).

What is the normal way to solve this? One straightforward way is to simply tell php-scoper to not prefix those wordpress native functions, but I haven't found a way to do that.

theofidry commented
vielhuber commented

Regarding your second question it looks to me that you are not using
Composer. The typical happy path is: require the Composer auto loader and
then the PHP-Scoper aliases. Function declarations are typically registered
as static files in Composer which are unconditionally all required (unlike
regular files which go through the classmap and are lazily loaded).

I'm using composer, I just stripped out the libraries in the demo example.
The problem is, that my code mixes composer classes with native wordpress functions, which are not in any namespace and don't use composer.

I just managed to get it somewhat working by adding:

<?php
declare(strict_types=1);
return [
	'whitelist' => ['*'] // exclude native wordpress global functions
];

The native functions are not prefixed anymore. The problem here is that all other parts are also not prefixed anymore.

theofidry commented
vielhuber commented

Thanks in advance, I will fiddle around in the meantime.

vielhuber commented

I fixed this with:

  • Add a namespace MyCustomPlugin to my WordPress plugin file
  • Add 'whitelist' => ['MyCustomPlugin\*'] to scoper.inc.php to exclude all global functions called in this file.
rustygoldcoin commented

I agree with @theofidry on this one. PHP-Scoper is doing its job here. It is definitely an antipattern to bring in global dependencies into a library you are trying to scope. By using PHP-Scoper, you are agreeing that you want to bundle your code in such a way that it can operate in its own environment without interference from the global context. Everything function/class you depend on should be somewhere defined within the code itself and not in an external library.

So PHP-Scoper is working just like it was designed. With that being said, there are many of us that would like to use PHP-Scoper to isolate dependencies for our WP plugins. Which requires us to depend on global functions such as \add_action() for the plugins to work.

I have a solution and am trying to package it up so that others can use it as well for this problem. I have a patch that will be pulled in as a separate Composer dependency. When it is pulled into your project, it should just work without any extra configuration. I'm working on getting it out this week and hope to have something done by 2/21/2020.

rustygoldcoin commented

You guys are welcome to try out the solution I developed.

https://github.com/ua1-labs/php-scoper-globals

Let me know if you guys need anything fixed. This Repo is hot off the presses and I haven't tested the code too much. But think I got everything we need to at least bring functions into scope.

vielhuber commented

Wow, you address exactly the issue, thanks for your work.

May I ask: Is this possible without extra configuration? I use a lot of WordPress functions overall in my plugins and don't want to add every single WP function to the configuration file.

rustygoldcoin commented

So the way that this works means you have to predefine all of the functions you are using in your plugin. I started to create https://github.com/ua1-labs/php-scoper-wp-patch/ but realized that it is going to be very resource intensive. If you'd like to take over the repo, I'll gladly let you take it over, but its not something I'm going to continue to support. Rather, I'm going to keep building out php-scoper-globals.

theofidry commented

Thanks @joshualjohnson, I get a much better understanding of the issue now and it's an interesting solution.

A possible alternative I just though of is to enrich the Reflector. Indeed the reflector is what is saying is an internal symbol or not, and internal symbols are not prefixed.

So a solution would be to tell PHP-Scoper that all WP symbols are internal. I'm not entirely sure though how it should be achieved. So just a few points as a kind of brainstorming session:

  • I do not just want to add unconditionally the WP symbols as internal: it might cause false positives for non-WP projects, might be an issue when scoping a WP project (is that a thing?) or others
  • We could have a setting just for WP, keep all the WP symbols and mark them as internal when that setting is enabled. It however brings the question on how to get that list of symbols in a reliable way and keeping it up to date
  • This problem could expand outside of WP, so ideally it would be nice to be able to register some arbitrary symbols
  • I don't want to require all WP-plugin authors to have to provide the list of WP symbols in each of their project: it's not a nice UX and error-prone
rustygoldcoin commented

@theofidry I understand you might be interested in supporting this feature. However, It's my belief that your php-scoper is doing exactly what it is supposed to. By adding support for registering globals, it's my belief that we would be perpetuating and antipattern. I think leaving php-scoper as is would be just fine and if people need the ability to register globals for applications like WP they are welcome to use ua1-labs/php-scoper-globals. My studio will continue to support PhpScoperGlobals as the solution for registering globals for humbug/php-scoper.

I do not, however, want to support https://github.com/ua1-labs/php-scoper-wp-patch because I believe that it is a major killer of memory and CPUs. During testing, I found that I reached my memory exception many times over because we are registering 1000s of functions for each request. So I'm going to probably deleting that repo and removing it from Packagist. I was asking if @vielhuber wanted to support the repo and I could transfer it over to them before I go and delete it.

vielhuber commented

@joshualjohnson Can you please give me a short readme how to get started with https://github.com/ua1-labs/php-scoper-wp-patch ? I will test and try it out if it works and would be pleased to maintain it in the future.

vielhuber commented

@theofidry: Just for brainstorming: Would it be possible to provide an additional setting like:
'exclude_global_functions_from_dir' => './.../path/to/outside/dir/'
php-scoper then scans all global functions inside that directory (where for example WordPress lays) and excludes those functions from being prefixed.

rustygoldcoin commented

Actually @vielhuber it's as simple as pulling in the code using composer require ua1-labs/php-scoper-wp-patch. Then it patches WP for you. https://github.com/ua1-labs/php-scoper-wp-patch/blob/master/UA1Labs/wp-functions.php includes all wp functions that need to get registered using PhpScoperGlobals. https://github.com/ua1-labs/php-scoper-wp-patch/blob/master/UA1Labs/wp-patch.php#L34 adds an autoloader for classes in the global scope.

theofidry commented

@vielhuber adding a setting listing the symbols to consider as internal yes, from a dir no

vielhuber commented

@joshualjohnson

Thanks for the instructions.

1st problem: I add php-scoper with:

require_once __DIR__ . '/vendor/scoper-autoload.php';

Now I get

Fatal error: Cannot redeclare add_submenu_page() (previously declared in ../vendor/scoper-autoload.php:1447) in /wp-admin/includes/plugin.php on line 1345

When I replace scoper-autoload.php with autoload.php, it works (but then other parts don't work!).

2st problem:
I get

Fatal error: Uncaught Error: Maximum function nesting level of '10000' reached, aborting! in .../wp-content/plugins/gtbabel/vendor/composer/ClassLoader.php on line 373

When I comment out

    \spl_autoload_register(function ($name) {
        return \_PhpScoperb314e498b17b\UA1Labs\PhpScoperGlobals::registerGlobalClass($name);
    });

it works. Do you have a clue what causes this problem?

rustygoldcoin commented

Yeah, the autoloader that is being registered needs to be updated to account for other logic I did not account for. That is one of the major reasons I don't intend to support the wp-patch solution. I feel it would be too much overhead for a WP plugin and would kill performance.

So with that being said, the issue is that the autoloader registered is trying to load a class that it shouldn't be trying to load. So, you'd have to figure out what class it is trying to load and the reason it is getting caught in a loop.

vielhuber commented

Then I would prefer providing a simple array of functions to phpscoper.
@theofidry: Is this already possible with the whitelist-option?

theofidry commented

@vielhuber not right now, but shouldn't be too hard to add one. What's to be done:

  • add an option to the php-scoper.inc.tpl template, e.g. internalSymbols?
  • add an option to Configuration
  • pass the added option to Reflector
  • adapt Reflector to use the added option
vielhuber commented

One question: Why doesn't the following work?

return [
    'whitelist' => [
        'add_submenu_page',
        'admin_print_scripts',
        /* ... */
    ],
];

Did I misunderstand the whitelist-option?

theofidry commented

The PHP-Scoper whitelisting mechanism assumes the whitelisted symbols are autoloaded within the scoped code. So in this case where the code you want to whitelist is from WP, it won't work

vielhuber commented

I managed to get it working with patchers:

$wp_functions = ['__','_admin_notice_multisite_activate_plugins_page','_e','...'];
return [
    'patchers' => [
        function (string $filePath, string $prefix, string $content) use ($wp_functions): string {
            // don't prefix native wp functions
            foreach($wp_functions as $wp_functions__value) {
                $content = str_replace('\\'.$prefix.'\\'.$wp_functions__value.'(', '\\'.$wp_functions__value.'(', $content);
            }
            return $content;
        }
    ]
];

This seems like a reasonable solution, or am I something missing?

rustygoldcoin commented

Yeah, that seems to be a fine fix. That is the exact reasons patchers were created.

theofidry commented

Is there a way to get all the WP functions easily?

rustygoldcoin commented

Not an easy way, but you could copy them from https://developer.wordpress.org/reference/functions/

vielhuber commented

No way I know. I used the list on https://codex.wordpress.org/Function_Reference, semiautomatically parsed it and added important classes like WP_Query to it.

rustygoldcoin commented

Thanks @vielhuber for the input. I've updated my library to include your findings you posted here. https://github.com/ua1-labs/php-scoper-globals/blob/1.1.0/patchers/globalIncludes.php

To use first include this file into your scoper.inc.php

require_once(__DIR__ . '/vendor/ua1-labs/php-scoper-globals/patchers/globalIncludes.php');

Now in your patchers config, include the following configuration

    'patchers' => [
        globalIncludes(['__', 'do_action'])
    ],

@vielhuber If you'd like, we can create a wpGlobalIncludes() that will simply return the patcher preconfigured with all wp functions/classes/constants.

Let me know.

vielhuber commented

Hello @joshuajohnson,

that sounds good.

I've found in the meantime the following functions, classes and constants, that were missing (perhaps you already included them):

'WP_Query', 'WP_Term_Query', 'WP_Widget', 'WP_PLUGIN_DIR', 'url_to_postid',

Then I adjusted the patcher so that also classes and function_exists('') works (perhaps you can update your code):

$content = str_replace('\\'.$prefix.'\\'.$include, '\\'.$include, $content); // "\PREFIX\foo()", or "foo extends nativeClass"
$content = str_replace($prefix.'\\\\'.$include, $include, $content); // "if( function_exists('PREFIX\\foo') )"
matzeeable commented

I also try to scope my WordPress plugins. I want to implement PHP-Scoper to WP React Starter but I come across this thread because I encounter the same issue. Just an idea: Would it be possible to whiltelist from a stub file like this one: https://github.com/php-stubs/wordpress-stubs ?

theofidry commented

Yes it should be

matzeeable commented

Proposel: Allow an option whitelist_stubs that allows to pass an array of file pathes, they get parsed with e. g. https://github.com/nikic/PHP-Parser and all kind of classes, function and so on automatically are passed to whitelist. This allows for example to whitelist WooCommerce stubs, too.

matzeeable commented

For all who read this issue and are interested in working with WordPress and PHP-Scoper, I have successfully implemented this in my open source boilerplate - right now in develop branch, but I will release soon: WP React Starter

@theofidry I thought about to start a PR to enhance the functionality of PHP-Scoper itself. Unfortunately I couldn't go that way because I needed more special logic due to the fact I am working in a monorepo. You can find my implementation in this commit: devowlio/wp-react-starter@c9513b3

Especially the following files could be of interest for you:

common/Gruntfile.plugin.ts # task "php:scope"
common/php-scope-stub.ts # extract all global classes, function, interfaces and traits from given files
common/php-scoper.php # PHP-Scoper configuration file with whitelisting and custom patcher

Technically said, it creates a whitelist for all available stubs and found plugins in our monorepo. It is a combination of JavaScript and PHP, but I think the logic is more important, it can be easily ported to PHP.

matzeeable commented

Hi again all!

The last few days I intensively worked on scoping WordPress plugins and found an ultimative way of doing this. I introduced PHP-Scoper to my boilerplate WP React Starter, see also as highlighted feature here:

Scoping your PHP coding and dependencies so they are isolated (avoid dependency version conflicts)

As I have mentioned previously (#303 (comment)) we need to consider stubs when we want to scope a WordPress plugin. I have introduced a documentation about this in my boilerplate documentation: read here. Another way would to go the opposite and only namespace a set of vendors, as you can read here: #378, but this is not yet possible.

Generally said, you can now install stubs or create custom stubs and simply put it to package.json#stubs and WP React Starter is doing the rest. 🎉

@theofidry Perhaps it is worth to put a link to WP React Starter into your README.md as possible solution for WordPress development.

What do you think?

theofidry commented

Perhaps it is worth to put a link to WP React Starter into your README.md as possible solution for WordPress development.

Yes definitely. Feel free to do a PR to add it :)

Thanks a lot for the work :)

I'm glad there is good solutions being proposed and valid existing alternatives. I think the next step now is to make the integration a bit smoother/performant. For this, I think the following could be done:

  • ❤️2
matzeeable commented

Yes definitely. Feel free to do a PR to add it :)

@theofidry Yeah, I have added it to my todo list, thank you! 🙂

[...] to PHP-Scoper as a dependency

Please do not add the stubs as dependency as PHP-Scoper should not contain stubs from third-parties. PHP-Scoper should be that configurable, so you can pass PHP files - it doesn't even have to be stubs (#303 (comment))

Add a map to https://github.com/php-stubs/wordpress-stubs the same way it is done for the PhpStorm stubs, see:

That is also not necessary, for the first. Please have a look at my implementation: https://github.com/devowlio/wp-react-starter/blob/master/common/php-scope-stub.ts

  1. A function which accepts an array of file pathes to stubs (as mentioned above, it does not need to be stubs, it can also be plain code)
  2. The function reads all the content of the passed files and passes the content to an AST conversation tool. In my case I am using a node implementation php-parser - in your case it should be glyazzle/php-parser.
  3. The AST gets recursively read and all available classes, functions, interfaces and traits are put into a string array

See #303 (comment) about adding an extension point for specifying what should be considered internal or not: this is not specifically about WP;

This is correct, you need to allow to specify a whitelist of identifiers (functions, classes, ...).

theofidry commented

That is also not necessary, for the first. Please have a look at my implementation:

No indeed it's not a necessity. But it's by far way more performant and don't require extra tooling :) Hence a nice to have but not a necessity

  • 👍1
dugajean commented

@theofidry Any news on this? Is there a way I can support development here?

mundschenk-at commented

I'd also like to help - I'm currently "manually" patching used WordPress functions in my plugins build process, but this is of course very error prone.

theofidry commented

Hi @mundschenk-at :) If you're willing to help I believe there is a few actionable points listed here

pxlrbt commented

@theofidry Based on @matzeeable solution I implemented a version of the stub extractor and patcher in PHP which probably is easier to integrate into PHP-Scoper: https://github.com/pxlrbt/php-scoper-prefix-remover

If you see a way how we could add this to PHP-Scoper let me know.

XedinUnknown commented

Of course, the problem is that if I require-dev this stuff, then I won't have any access to it during the build, because the build, naturally, installs non-dev dependencies: dev dependencies are not required in the final product. Which is why it is such a great thing that one could install PHPScoper with Phive: it doesn't need to be a code dependency. If the other supplementary tools cannot be installed that way too, then I'm not sure how they could be used without implementing super hacky workarounds with declaring a dependency on something that is not needed for production.

XedinUnknown commented

Correct me if I'm wrong, though: does this issue stem from the fact that WordPress is not available to PHPScoper during prefixing? Because I was puzzled as to why the global WP functions are getting prefixed when whitelist-global-functions etc is on. Would it help if the functions were explicitly used from the global namespace in the code, e.g. \add_action('init', function() {})? Or perhaps if they are imported with use function add_action?

pxlrbt commented

Of course, the problem is that if I require-dev this stuff, then I won't have any access to it during the build, because the build, naturally, installs non-dev dependencies: dev dependencies are not required in the final product. Which is why it is such a great thing that one could install PHPScoper with Phive: it doesn't need to be a code dependency.

In this case you can download that part manually. Or only composer require that package during your build. There are options. I'd also prefer an official way or integrating this into PHP-Scoper.

Correct me if I'm wrong, though: does this issue stem from the fact that WordPress is not available to PHPScoper during prefixing

Not sure what's causing this as I have no knowledge about PHP-Scoper internals.

Would it help if the functions were explicitly used from the global namespace in the code, e.g. \add_action('init', function() {})? Or perhaps if they are imported with use function add_action?

Tried it both and it didn't work. Either the function statement or the use function statement will be prefixed.

XedinUnknown commented

I settled for downloading the file with Gulp during build. However, with the php-scoper-prefix-remover things aren't so simple, because it's a pacakge.

What I would do if I wanted to integrate this into PHP Scoper:

  1. Merge IdentifierExtractor into this project. Maybe needs a bit of polishing, but overall it would be great to have this functionality in PHP Scoper itself.

  2. Have users get the stubs however they want. For example, they could download the stub file during build, like I did.

  3. Include an example of a WP patcher that uses the above extractor in conjunction with the stub file. Just put it into the readme. Something along the lines of:

    <?php declare(strict_types=1);

// scoper.inc.php

use IdentifierExtractor;

$identifiers = (new IdentifierExtractor())
	->addStub('stub-file.php')
    ->extract();

return [
    'patchers' => [
        function (string $filePath, string $prefix, string $content) use ($identifiers): string {
	        $prefixDoubleSlashed = str_replace('\\', '\\\\', $prefix);
	        $quotes = ['\'', '"', '`'];
	
	        foreach ($identifiers as $identifier) {
	            $identifierDoubleSlashed = str_replace('\\', '\\\\', $identifier);
	            $content = str_replace($prefix . '\\' . $identifier, $identifier, $content); // "PREFIX\foo()", or "foo extends nativeClass"
	
	            // Replace in strings, e. g.  "if( function_exists('PREFIX\\foo') )"
	            foreach ($quotes as $quote) {
	                $content = str_replace(
	                    $quote . $prefixDoubleSlashed . '\\\\' . $identifierDoubleSlashed . $quote,
	                    $quote . $identifierDoubleSlashed . $quote,
	                    $content
	                );
	            }
	        }
        },
    ],
];

My reasoning is that the need to extract identifiers from arbitrary PHP code is generic, while the code itself and the WP patcher are specific to the user's project, and to WP, respectively.

pxlrbt commented

However, with the php-scoper-prefix-remover things aren't so simple, because it's a pacakge.

Sure. I see it as a temporary fix until we solved that issue within PHP-Scoper.

Merge IdentifierExtractor into this project. Maybe needs a bit of polishing, but overall it would be great to have this functionality in PHP Scoper itself.

Happy for any feedback for improvements. I see whether I can create a PR for this.

Have users get the stubs however they want. For example, they could download the stub file during build, like I did.

Isn't that the current state? I downloaded them via composer but you could also just add them to your project and reference that file.

My reasoning is that the need to extract identifiers from arbitrary PHP code is generic, while the code itself and the WP patcher are specific to the user's project, and to WP, respectively.

Do you have any examples that also need that identifier extraction but have nothing to do with removing that prefix? I think it makes it easier for users to just include that specific patcher from PHP-Scoper but I also understand that this could lead to more maintenance work.

There's also a SymfonyPatcher in the core of PHP-Scoper. Maybe it's useful to add some more common patchers.

XedinUnknown commented

However, with the php-scoper-prefix-remover things aren't so simple, because it's a pacakge.

Sure. I see it as a temporary fix until we solved that issue within PHP-Scoper.

Yep

Merge IdentifierExtractor into this project. Maybe needs a bit of polishing, but overall it would be great to have this functionality in PHP Scoper itself.

Happy for any feedback for improvements. I see whether I can create a PR for this.

I don't have concrete suggestions right now, but I thought that maybe PHP Scoper uses some DI container or something else that could be used to, for example, inject the parser factory into the extractor.

Have users get the stubs however they want. For example, they could download the stub file during build, like I did.

Isn't that the current state? I downloaded them via composer but you could also just add them to your project and reference that file.

Yes, it is. I basically wrote how I see things eventually. So, I think that the way things are with regard to obtaining the stubs file is good already.

My reasoning is that the need to extract identifiers from arbitrary PHP code is generic, while the code itself and the WP patcher are specific to the user's project, and to WP, respectively.

Do you have any examples that also need that identifier extraction but have nothing to do with removing that prefix? I think it makes it easier for users to just include that specific patcher from PHP-Scoper but I also understand that this could lead to more maintenance work.

There's also a SymfonyPatcher in the core of PHP-Scoper. Maybe it's useful to add some more common patchers.

I don't have examples. But what matters to me is separation of concerns. There can be countless cases in the future which for now we have not thought about. The only thing we know right now is that extracting identifiers is a generic task used during scoping, and that patching WP code is a usecase-specific task.

Of course, removing the prefix for any set of arbitrary identifiers is a generic task that is useful during scoping, so if the patcher is not WP-specific, it too can be included in PHP Scoper, no problem 🙂

XedinUnknown commented

When I run Scoper with the new stuff, I get this:

PHP Fatal error: Uncaught Error: Class 'PhpParser\ParserFactory' not found in /app/build/tests/stub/external/IdentifierExtractor.php:40

In my scoper.inc.php I do this:

require_once sprintf('%1$s/tests/stub/external/IdentifierExtractor.php', __DIR__);
require_once sprintf('%1$s/tests/stub/external/RemovePrefixPatcher.php', __DIR__);

use pxlrbt\PhpScoper\PrefixRemover\IdentifierExtractor;
use pxlrbt\PhpScoper\PrefixRemover\RemovePrefixPatcher;
use Isolated\Symfony\Component\Finder\Finder;

Any ideas?

pxlrbt commented

@XedinUnknown I'd guess: Since you didn't install the package via composer the nikic/php-parser package is missing. The version PHP-Scoper is using probably can't be used since it's bundled in the phar file.

I'm working on a PR for PHP-Scoper.

XedinUnknown commented

It sounds about right that the dep is missing. But why wouldn't I be able to use the one bundled in the Phar? After all, the config file is being required by the Phar, no?

pxlrbt commented

I don't fully understand how the classes can be accessed from outside the phar file. Still stuck on that part. Apart from that I submitted a first draft which works with PHP-Scoper so far but not with the phar.

  • ❤️1
XedinUnknown commented

So, key takeaways up to now:

  1. A better approach than patching would be to use the Reflector class to flag WP symbols as internal.
  2. PHPScoper classes (most) cannot be used in the config because PHPScoper itself is scoped. It's possible to temporarily monkey-patch the problem by creating a class alias in the config, mapping the prefixed name to the unprefixed one.
  3. Adding symbols to whitelist doesn't work, even if the stub files are added to the list of files to process. The original symbols in wordpress-stubs.php aren't getting prefixed, while the ones in the target files are. It wouldn't work anyhow, because whitelisting works by prefixing anyway and then aliasing.
XedinUnknown commented

I've started exploring the idea of adding internal symbols to the Reflector class, but there's a problem: the IdentifierExtractor puts all symbols in the same list, while Reflector must be given functions, classes, and constants separately.

XedinUnknown commented

@pxlrbt, with the class alias for ParserFactory, I tried again. The IdentifierExtractor isn't finding any identifiers in the stub file. Any ideas?

The class names in ParserFactory will never match because they're prefixed in PHPScoper.

  • 👍1
XedinUnknown commented

@pxlrbt some fixes that are important if we want to use the work-around in pxlrbt/php-scoper-prefix-remover#2.

XedinUnknown commented

@theofidry, the prefix remover now contains an extractor which can return symbols from the source files separated by type This correlates with what Reflector seems to accept. Gonna try and get those symbols into the reflector now.

  • 👍1
XedinUnknown commented

@theofidry, upon composer install in my fork of this project, I get the following:

Your lock file does not contain a compatible set of packages. Please run composer update.

  Problem 1
    - humbug/box is locked to version 3.9.0 and an update of this package was not requested.
    - humbug/box 3.9.0 requires humbug/php-scoper ^0.13 -> found humbug/php-scoper[dev-master, 1.0.x-dev (alias of dev-master)] but it does not match the constraint.

Perhaps the lockfile is out of date?

XedinUnknown commented

composer update results in the following:

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

  Problem 1
    - humbug/box[dev-master, 3.8.4, ..., 3.9.0] require humbug/php-scoper ^0.13 -> satisfiable by humbug/php-scoper[0.13.0, ..., 0.13.9] from composer repo (https://repo.packagist.org) but humbug/php-scoper[dev-master, 1.0.x-dev (alias of dev-master)] from root package repo has higher repository priority. The packages with higher priority do not match your constraint and are therefore not installable. See https://getcomposer.org/repoprio for details and assistance.
    - humbug/box[3.8.0, ..., 3.8.1] require humbug/php-scoper ~0.12 -> satisfiable by humbug/php-scoper[0.12.0, ..., 0.13.9] from composer repo (https://repo.packagist.org) but humbug/php-scoper[dev-master, 1.0.x-dev (alias of dev-master)] from root package repo has higher repository priority. The packages with higher priority do not match your constraint and are therefore not installable. See https://getcomposer.org/repoprio for details and assistance.
    - humbug/box[3.8.2, ..., 3.8.3] require humbug/php-scoper ^0.12 -> satisfiable by humbug/php-scoper[0.12.0, ..., 0.12.4] from composer repo (https://repo.packagist.org) but humbug/php-scoper[dev-master, 1.0.x-dev (alias of dev-master)] from root package repo has higher repository priority. The packages with higher priority do not match your constraint and are therefore not installable. See https://getcomposer.org/repoprio for details and assistance.
    - humbug/box 3.x-dev is an alias of humbug/box dev-master and thus requires it to be installed too.
    - Root composer.json requires humbug/box ^3.8 -> satisfiable by humbug/box[3.8.0, ..., 3.x-dev (alias of dev-master)].
theofidry commented

you need to do source .composer-root-version first, it's unfortunately a quirk of having circular dependency packages

  • 👍1
XedinUnknown commented

This project doesn't use a DI container, so it's appears to be really difficult to get the configuration into the reflector: The place that creates the Reflector doesn't have access to the config, as the config is created and loaded in the add-prefix command.

It feels like, in order to create a good solution, some re-factoring would need to take place. Would you consider something like this? Here's what I would do:

  • Use a real DI container, and create everything inside service definitions. This is an application, rather than just a library, and it's composed of parts. So many different parts are difficult to control. A cascading set of service definitions would make many things a lot simpler.
  • Make all config accessible through the container. After all, what the container exposes is in fact configuration, because it decides which values are received by the configured services.
  • The above would mean that all config is accessible to any service. Thus it would be possible to inject these values into the AddPrefixCommand, and any other command that may be added in the future.
  • It would also mean that the Reflector could then receive any config values, including a list of identifiers to consider "internal". The TraverseFactory receives the configured Reflector, pass it to the node visitors, and thus the decision of what to truly whitelist becomes controllable via configuration.

Thoughts?

theofidry commented

This project doesn't use a DI container

There is a DI container, see Container. But not everything is blindly registered as a service there because the application is small enough for it to not be needed, at least up to this point.

Use a real DI container, and create everything inside service definitions. This is an application, rather than just a library, and it's composed of parts. So many different parts are difficult to control. A cascading set of service definitions would make many things a lot simpler.

I am a bit cautious of the word "real" here :) The Container is not anything less than a real container, it's more capable than the PSR-11 container since it provides type-hinted services although less generic and a full-blown a la Symfony container is way overkill: it's a lot of code (which keep in mind increase the PHAR size – which matters since is distributed and loaded, unlike a standard app which can lazily load some parts of the code and can benefit from opcache/preloading), is way more complex (requires to "compile" the container, which needs to be done before bundling the PHAR – a PHAR is readonly...).

Make all config accessible through the container. After all, what the container exposes is in fact configuration, because it decides which values are received by the configured services.

The issue with this approach is that you either make your container unusable in some context (due to the configuration not being present) or require to register an empty configuration and make it mutable at run-time (either the configuration itself or replace the registered configuration). Both methods introduce state inconsistencies (which sometimes cannot be avoided for sure).

The above would mean that all config is accessible to any service. Thus it would be possible to inject these values into the AddPrefixCommand, and any other command that may be added in the future.

This approach is fine in itself, but I think it's pushing a bit too much refactoring for what we want to do here.

Overall to be clear I don't think that your proposition is bad at all, but I think it is going either a bit too far or introducing too much complexity for what PHP-Scoper does (at least at the moment).

Maybe simpler approach:

  • Introduce a new ReflectorConfiguration which
    • could be empty – may be useful for testing purposes
    • a mutator to register any arbitrary symbol which allows the ability to handle conflicting symbols (e.g. a symbol that is defined with 2 different min PHP version)
  • Introduce a ScoperSymbols class which just register the symbols currently listed in Reflector: they serve the same purpose but I think having it in a more dedicated place than either Reflector or ReflectorConfiguration will make them a bit more accessible
  • the container creates a ReflectorConfiguration with the default symbols registered (exposed by the container)
  • the Reflector service takes the injected symbols now instead of initializing them in the constructor, taken from the ReflectorConfiguration
  • the user defined symbols can be registered to the Configuration class
  • the AddPrefixCommand can register the symbols found in the configuration to the ReflectorConfiguration

This allows also to add a safeguard to prevent to register a symbol to the ReflectorConfiguration when the Reflector service has already been instantiated.

Whilst the list itself is not small, it actually requires little changes to the container or command, the configuration do need changes but there is not much we can do there, and makes the reflector class & config a bit more testable which becomes necessary as we now expose and manipulate state to those services.

WDYT?

XedinUnknown commented

Thanks for the detailed reply!

What I meant by the container not being real is:

  1. Not possible to configure container from outside with arbitrary service names.
  2. Only registers 2 services, while there are dozens of things in the application.

In order to avoid complicating things, I was thinking to simply add methods on the container for many other services, such as configuration, etc. But this would lead to mostly the same changes in the application as with a PSR-11 implementation, and therefore it's better to just go full on DI Container from the beginning.

There are a few container implementations out there, many of them much smaller than Symfony's, or PHP DI, etc. One example is my dhii/containers. It works with PSR-11 and with an experimental Service Provider standard. Together those standards lie in the base of my dhii/module-interface, which describes a configurable module. This architecture is very simple and predictable, and has time and time again shown itself as both solid and flexible architecture for several commercial projects, including but not limited to WordPress plugins. With this approach, everything is in its own place, very abstracted and configurable, and automatically overridable and extendable from outside of the module by other modules. But of course, modularity could be seen as an overkill at this point (although I personally don't think it ever is). In any case, simply accessing all services via a PSR-11 container already makes everything organized, while being future-proof. So, I think this should really be done anywhere.

I didn't really understand your point about the container being sometimes unusable due to the configuration not being present. The configuration is always present: it is the container itself, with parts of it probably being defined in the scoper.inc.php, and parts in another place like services.php, with sensible defaults that can be overridden or extended by other parts optionally (see above about modules). It is loaded on any command, whenever the application is being run. Why wouldn't e.g. scoper.inc.php not get loaded, if it's the source of configuration for the application?

If we consider each run of the application as a request (which it would be in a SAPI environment), we could say that for each request there should only be one configuration. I have been following this rule, and have never encountered a situation where the container must be made mutable, i.e. altered after its initial configuration at creation time. This doesn't seem to be an issue at all to me.

Introducing ReflectorConfiguration de-centralizes configuration in the application. If this was a PSR-11 container, the Service Locator anti-pattern would quickly become apparent here. Configuration is configuration: something that services receive at creation time. Each configuration value, retrieved from the DI container, can simply be passed to the constructors of the services. If concerns are properly separated, there will never be too many constructor arguments this way: if there are, then that service has too much responsibility. From my point of view, the DI container represents the Single Source of Truth for all configuration of the application. This is extremely robust, predictable, and flexible - from my experience.

In any case, if ReflectorConfiguration is retrieved from the same scoper.inc.php that other configuration is retrieved from (and why wouldn't it be?), we would just need to load the same file twice. This is more complicated than it needs to be. From my perspective, a tried and tested approach is to load configuration once from a file, and merge it into the container. This configuration would include a collection of symbols to consider "internal" (maybe from an internal config key). This can be represented by something like IdentifiersInterface from pxlrbt/php-scoper-prefix-remover#3. Since PHP Scoper config is imperative, it doesn't matter where these symbols come from. If the default set of identifiers needs to be extended, scoper.inc.php may use the IdentifierExtractor to obtain a list of identifiers that have been extracted from files, like a stub file or anything else. Eventually, the list of "invernal" identifiers (which may consist of the current default values) simply gets injected into the Reflector instance just like any other configuration into any other class.

I think my approach avoids a lot of complications, as well as even perhaps some redundant implementations. Its very predictable, and there's no way any configuration can be injected anywhere after intialization of services. I know it may look like a lot, but it really is very simple. I can demonstrate some work done this way, if you want. wp-oop/plugin-boilerplate is a template for WP plugins that use this approach, which is in-detail described in Cross-Platform Modularity in PHP.

theofidry commented

Hm I'll try to break it down as much as I can.

Only registers 2 services, while there are dozens of things in the application.

I don't think there is that many:

  • All the node visitors, the node traverser and its utility must have a limited lifetime, so whilst TraverserFactory is a legit service, all of what it contains should not be.
  • All the scopers are indeed services although not configured: this is mostly because writing it by hand is trivial whereas with a container you need one that have tags with priority to support that chain of decorators – that alone already ticks off most simple containers
  • Without the ones above, the remaining ones are:
    • TraverserFactory
    • ParserFactory
    • Lexer

And if they are not exposed it's because there was no need so far, but it's trivial to expose them.

I didn't really understand your point about the container being sometimes unusable due to the configuration not being present. The configuration is always present: it is the container itself, with parts of it probably being defined in the scoper.inc.php, and parts in another place like services.php, with sensible defaults that can be overridden or extended by other parts optionally (see above about modules).

By configuration here I mean Configuration: as you can see it is not exposed or registered to the Container. The application has a Container passed at instantiation time and by that time, you don't necessary have a scoper.inc.php to load (and not necessarily the need to yet). So you have the following choices:

  • Have a Configuration with default values anyway and you make it overridable; either by mutating directly some Container parameters or the Configuration instance (the later is more dangerous since it means if you had a service depending on a configuration that has already been instantiated, it won't be updated) or by having an immutable container (cf. https://github.com/infection/infection for example).
  • Have a Configuration with aims at containing a state passed at runtime: services don't depend on those parameters to be registered and instantiated

The 2nd option is what I went for at first due to simplicity, and at least so far there was no need to change it.

It is loaded on any command, whenever the application is being run. Why wouldn't e.g. scoper.inc.php not get loaded, if it's the source of configuration for the application?

That is a big assumption and design constraint: when you start a project you don't have one, so what about the init command? Do you always want to have a config file? (e.g. in Box I made it completely optional.

It is currently the case for one and one reason only: I don't use PHP-Scoper as an app myself but mostly for Box. So whilst I don't want to prevent anyone to use it as a standalone app (and there is users, like yourself or PHPUnit), how many features are currently available is limited by its users. A good example is that in Box the scoping is done in parallel or you can scope one file without changing anything as a "dry-run", yet none of those features are available here.

Introducing ReflectorConfiguration de-centralizes configuration in the application. If this was a PSR-11 container, the Service Locator anti-pattern would quickly become apparent here. Configuration is configuration: something that services receive at creation time. Each configuration value, retrieved from the DI container, can simply be passed to the constructors of the services. If concerns are properly separated, there will never be too many constructor arguments this way: if there are, then that service has too much responsibility. From my point of view, the DI container represents the Single Source of Truth for all configuration of the application. This is extremely robust, predictable, and flexible - from my experience.

So here maybe you see what I mean, "Configuration is configuration: something that services receive at creation time", is:

  1. not necessarily true
  2. what you suggest here is to make the container configurable at run-time (i.e. after it is instantiated), which is not a benign design choice

So I don't see ReflectorConfiguration as decentralizing the configuration, but making 2. possible (which is what you request), however I think pushing for it for all the configuration parameters when most of the state is passed directly to the services as runtime state (i.e. not the constructor), is pushing for a lot of changes that are not strictly necessary for now.

I think my approach allows a lot of complications, as well as even perhaps some redundant implementations. Its very predictable, and there's no way any configuration can be injected anywhere after intialization of services. I know it may look like a lot, but it really is very simple. I can demonstrate some work done this way, if you want.

Don't worry I'm not questioning your approach, I am familiar with an all-Container oriented applications and CLI applications :) I however don't think it's necessary everywhere and for example even Box which has a lot of commands and configuration do just fine without.

So TL:DR; I am not saying the changes you suggest are wrong: it is a perfectly and sound valid design decision. I think however that it's more changes than necessary at the moment.

XedinUnknown commented

I was not suggesting making the container mutable. In fact I wrote that I never had the need to make a DI container mutable. And of course, if there's no config file present then it simply doesn't get loaded, and defaults get used instead, that's no problem. Now I see, though, that because the path to the config file may be passed on the CLI, the application (which should already be initialized at that time) may have a problem getting that config into the DI container without making the container mutable. I would solve this in the following way: make a core application that is agnostic of how it is run (CLI, SAPI, etc), and which requires everything to be initialized including a completely configured DI container; and then wrap it into a CLI environment using Symfony like you are doing, but this doesn't require the application's DI container. For a CLI command to be initialized, it doesn't need much; only when it is run do we need to run the actual application.

But OK. Let's say we go with ReflectorConfiguration, and don't change much in the application. I don't really understand how the configuration from scoper.inc.php, which presumably would contain the "internal" identifiers list, and which right now is read and loaded only during a specific CLI command, would get into the Reflector that is initialized in the Container. Could you kindly elaborate a little more on that?

theofidry commented

We would have:

  • ReflectorConfiguration registered to the Container with the default symbols registered
  • The Reflector service which uses the ReflectorConfiguration to get the symbols
  • The AddPrefixCommand which has access to the container can get ReflectorConfiguration and register the symbols found from the config to it

This does require though that we register the symbols before getting the Reflector service.

I would solve this in the following way: make a core application that is agnostic of how it is run (CLI, SAPI, etc), and which requires everything to be initialized including a completely configured DI container; and then wrap it into a CLI environment using Symfony like you are doing, but this doesn't require the application's DI container. For a CLI command to be initialized, it doesn't need much; only when it is run do we need to run the actual application.

Yes, so basically the Container that you see at the moment is the "core application" that is agnostic – that's how Box uses it. So having more around it would make sense, but only if re-used somewhere else IMO, e.g. another command that needs a similar logic to AddPrefixCommand command. Until then AddPrefixCommand can take a bit more complexity for that;

XedinUnknown commented

With regard to the configuration, that would require ReflectorConfiguration to me mutable. Bad idea.

About the container, and in general, this would add complexity only to have to remove it later. So basically it would make your future life harder. Why do that, if you know it's not good? It does not take much more time to write simpler code. Why make AddPrefixCommand more complicated and thus spend time decreasing software quality, when we could spend the same time making things simpler and thus increasing the software quality?

theofidry commented

So let's list the solutions we have:

  • Having a mutable configuration – which I agree is not ideal hence restricting it to the only strictly necessary which means only one element of the configuration
  • Having the Configuration registered & accessible to the Container which means:
    • Having services depending on the Configuration
    • Having a default Configuration (which may be complete non-sense e.g. Configuration#path)
    • Either having the exposed container configuration mutable (i.e. you can replace the configuration service) – which means if a service relying on the configuration has already been instantiated it will use the old config values; or create a new container instance but then you are dealing with multiple container instances (which is not simpler IMO)
  • Have two types of container (one which is configuration agnostic – in which looking at it you would have one and only one service...) and the other which takes the configuration (which again is for 3 services)

IMO there is no clear winner in there...

theofidry commented

Looking at it again and I'm double checking the usage in Box as well, I'm wondering if the current Container makes sense tbh, it's only used for Container::getScoper() to it could very well be a ScoperFactory::createScoper(ReflectorConfiguration) which is could be immutable and it would work great all the same

XedinUnknown commented

I am all for simplifying the current Container, which IMHO is also redundant.

With regard to solutions, I don't see having 2 containers as complexity. They belong to different "applications", if you will:

  1. One dealing with the CLI interface that triggers the scoping application. Using an immutable PSR container which exposes all of the CLI application's configuration via a single interface. This container contains e.g. the CLI commands.
  2. One dealing with actual scoping features. Uses an immutable PSR-11 container which exposes all application configuration via a single interface.

The CLI interface application takes care of the CLI environment, commands, env variables, CLI documentation, etc. It wraps and triggers the bootstrap of the scoping features application. The latter doesn't know how it was triggered (i.e. in this case via CLI), and simply performs scoping as per its standardized configuration from its container.

Nothing needs to be mutable. Reduced coupling (run PHP-Scoper directly from PHP, perhaps). Single source of truth. All services configurable from outside potentially. All config is available to all services. Dependency inversion everywhere: no class decides what configuration to use. Easy future add-ons. Etc etc etc.

If we take the above approach, I can pitch in. But of course it would be great if someone would also be handling the tests, because they would need to be updated. But of course it's your decision.

theofidry commented

If you feel you can drastically simplify things with a dedicated container you can do a PoC to showcase it, but again, I think this is far from being necessary for achieving what we want here.

XedinUnknown commented

Right, sorry for the delay. What would you consider to be a PoC? What scale, specifically?

theofidry commented

Enough to demonstrate your point on how it would simplify things here

XedinUnknown commented

It seems to me that a proof of concept that would demonstrate how it would simplify things is something that would simplify all of these things, because right now those things are rather entangled. I don't have time to do this at the moment. This may change if my management decides that we want to use PHP Scoper specifically, and nothing else works.

XedinUnknown commented

Meanwhile, I have been trying alternative approaches. It appears that even if WordPress is present at the time of scoping, it gets scoped anyway, even though PHP Scoper should be able to see that the identifiers are in the global namespace. So, that doesn't work, for some reason.

XedinUnknown commented

Is there any movement on this at all?

theofidry commented

I'm working on something :) Might not come out before a few months still but I hope it will get there

theofidry commented

Could someone give a got at the master branch? With #494 it should be possible to exclude completely some symbols. So registering Wordpress symbols there should do the trick I think

  • ❤️1
swissspidy commented

At first glance that change looks really good! This in combination with a dynamic list obtained from WP stubs will be very powerful.

Tried to get it to work locally, but getting an Invalid configuration key value "excluded-constants" error. exclude-constants doesn't work either. #494 uses both key names interchangeably, so not sure which one is correct.

theofidry commented

Ha, that's an issue that would be captured by the end-to-end tests (but they are broken right now as Box needs to be adapted for the BC breaks introduced in PHP-Scoper). Sorry about that, fixed in #497

  • 👍1
xoich commented

@theofidry I am still experiencing the problem on master after #497. I solved by adding the recently added keywords in ConfigurationFactory.php in the KEYWORDS array that is just below.

XedinUnknown commented

@xoich, can you expand on what exactly you did, please?

@theofidry, can you confirm that there is still indeed a bug?

theofidry commented

@XedinUnknown the bug should be fixed by #514

  • 👀1
swissspidy commented

The current version on master works great now!

Huluti commented

Hi! The exclude options works well except on one function from my side. It's the function WC() from WooCommerce.

Here's my config:

    'exclude-functions' => [
        '__',
        'WC',
        'esc_attr',
        'wp_kses_post',
        'sanitize_text_field',
        'wc_add_notice'
    ],

All functions are not prefixed except WC. I've tried to displace it in exclude-classes and exclude-constants but it doesn't work too. It seems that is is a function: http://hookr.io/functions/wc/
Thank's for your work.

theofidry commented

Thanks for the report @Huluti, would it be possible to have a small reproducer?

Huluti commented

Here's a small reproducer: test.zip
Just install deps and run PHP Scoper on master version and you should see that the WC function keeps going in the final scoper-autoload.php

XedinUnknown commented

If it's working, any chance you could make an alpha release? Alternatively, any ideas how I can depend on a specific branch with Phive?

theofidry commented

Unlikely: this introduces a few BC breaks and there is more coming so I would like to have too many releases with not insignificant BC breaks in succession

I'll try to finish this in the coming month or two

  • 👍1
XedinUnknown commented

Well, BC breaks can be signalled through SemVer, and the fact that it's still in testing can be signalled through stability (like alpha). I'm only asking because I cannot install this with Composer right now into my project, due to conflicting requirements.

pxlrbt commented

@XedinUnknown Couldn't you install it in a subdirectory with a separate composer.json for testing? E.g. ./tools/composer.json and only require the dev version of PhpScoper.

theofidry commented

@XedinUnknown the issue is not so much about communicating them and respecting semver, but having several in a row about the configuration: it is annoying as a user (myself included) to do and even more so if it includes back-and-forth.

I'm only asking because I cannot install this with Composer right now into my project, due to conflicting requirements.

Is there any reason why you can't use the dev-master version otherwise?

  • 👍2
calvinalkan commented

@theofidry @XedinUnknown @pxlrbt @swissspidy

I got so annoyed with this but desperately needed this since we are about to release a gigantic WordPress framework.

I always wanted to tinker around a little with symfony/console so I created a standalone command-line tool that uses nikic/php-parser and can dump any php file into a set of files that can be used with the new exclusion features in master.

So far the following files will be generated:

  • All class names with their full namespace
  • All function names with their full namespace
  • All constant names with their full namespace
  • All interface names with their full namespace
  • All define statements with their full namespace
  • All trait names with their full namespace

I already created exclusion lists for WooCommerce, WordPress core, and WP-Cli:

You can find them here:

This is the low-level command-line tool:

You can use it to generate excludes for anything. Its not limited to WordPress.

Let me know what you think.

  • 👍2
  • 👀1
ahegyes commented

I came across this issue last April, and developed a similar workflow to that proposed by @calvinalkan.

I didn't make it as modular as he has, but I actually wrote an article about my approach today (what a coincidence!), and I wanted to share it in case it helps someone: https://www.deep-web-solutions.com/how-to-avoid-dependency-conflicts-using-composer-and-php-scoper/

  • 👍1
  • 👀1
theofidry commented

👍 I unfortunately cannot dive into this issue yet as there is a big overhaul on how the symbols are registered and process on the way. Once that is done I can check the PHP 8.1 compat and this issue. It will however definitely get some attention before considering a new release

calvinalkan commented

@theofidry

Will the public API for the configuration be the same?

IE. Provide a plain array of strings for classes, functions, constants, etc?

Or will the mechanism be completely different?.

Also, I had no idea whether interfaces and traits are supported by the excludes and whether they will belong to the classes category, so I went and separated them into their own files.

Do you have something in mind for this yet?

theofidry commented

You can find the (user) API changes here: https://github.com/humbug/php-scoper/releases/tag/0.16.0

Also, I had no idea whether interfaces and traits are supported by the excludes and whether they will belong to the classes category, so I went and separated them into their own files.

Interfaces are supported by both exclude & expose. Traits cannot be exposed though as there is no alias mechanism possible for it at least atm. Maybe this could be achieved with something similar to what is done for functions? Not sure

calvinalkan commented

At least for WP traits are not relevant, neither is the expose functionality I think. As long as WP symbols are considered "internal" everything should be good.

Screenshot_20220203-103427

So passing the automatically generated files into this should work just fine.

https://github.com/sniccowp/php-scoper-wordpress-excludes/blob/master/generated/exclude-wordpress-functions.php