humbug / php-scoper

🔨 Prefixes all PHP namespaces in a file/directory to isolate the code bundled in PHARs.
MIT License
707 stars 76 forks source link

[Scoper] Improve the WP support experience #303

Closed wujekbogdan closed 1 year ago

wujekbogdan commented 5 years ago

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 https://github.com/humbug/php-scoper/issues/303#issuecomment-614097867 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']);
XedinUnknown commented 3 years ago

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 3 years ago

@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 3 years ago

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 3 years ago

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.

XedinUnknown commented 3 years ago

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 3 years ago

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 3 years ago

~@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.

XedinUnknown commented 3 years ago

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

XedinUnknown commented 3 years ago

@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.

XedinUnknown commented 3 years ago

@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 3 years ago

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 3 years ago

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

XedinUnknown commented 3 years ago

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:

Thoughts?

theofidry commented 3 years ago

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:

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 3 years ago

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 3 years ago

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:

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:

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 3 years ago

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 3 years ago

We would have:

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 3 years ago

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 3 years ago

So let's list the solutions we have:

IMO there is no clear winner in there...

theofidry commented 3 years ago

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 3 years ago

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 3 years ago

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 3 years ago

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

theofidry commented 3 years ago

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

XedinUnknown commented 3 years ago

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 3 years ago

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 3 years ago

Is there any movement on this at all?

theofidry commented 3 years ago

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

theofidry commented 3 years ago

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

swissspidy commented 3 years ago

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 3 years ago

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

xoich commented 3 years ago

@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 3 years ago

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

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

theofidry commented 3 years ago

@XedinUnknown the bug should be fixed by https://github.com/humbug/php-scoper/pull/514

swissspidy commented 3 years ago

The current version on master works great now!

Huluti commented 3 years ago

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 3 years ago

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

Huluti commented 3 years ago

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 3 years ago

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 3 years ago

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

XedinUnknown commented 3 years ago

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 3 years ago

@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 3 years ago

@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?

calvinalkan commented 2 years ago

@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:

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.

ahegyes commented 2 years ago

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/

theofidry commented 2 years ago

👍 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 2 years ago

@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 2 years ago

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 2 years ago

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