Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Scoper] Improve the WP support experience #303

Closed
wujekbogdan opened this issue Feb 19, 2019 · 111 comments
Closed

[Scoper] Improve the WP support experience #303

wujekbogdan opened this issue Feb 19, 2019 · 111 comments
Milestone

Comments

@wujekbogdan
Copy link

wujekbogdan commented Feb 19, 2019

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
Copy link
Member

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
Copy link

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

@vielhuber
Copy link

vielhuber commented Feb 5, 2020

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
Copy link
Member

theofidry commented Feb 5, 2020 via email

@vielhuber
Copy link

vielhuber commented Feb 5, 2020

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
Copy link
Member

theofidry commented Feb 5, 2020 via email

@vielhuber
Copy link

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

@vielhuber
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link
Member

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
Copy link

@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
Copy link

@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
Copy link

@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
Copy link

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
Copy link
Member

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

@vielhuber
Copy link

vielhuber commented Feb 24, 2020

@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
Copy link

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
Copy link

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

@theofidry
Copy link
Member

theofidry commented Feb 26, 2020

@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
Copy link

One question: Why doesn't the following work?

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

Did I misunderstand the whitelist-option?

@theofidry
Copy link
Member

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
Copy link

vielhuber commented Mar 4, 2020

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
Copy link

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

@theofidry
Copy link
Member

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

@theofidry
Copy link
Member

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

@Huluti
Copy link
Contributor

Huluti commented Jul 29, 2021

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
Copy link
Contributor

XedinUnknown commented Aug 17, 2021

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
Copy link
Member

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
Copy link
Contributor

XedinUnknown commented Aug 17, 2021

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
Copy link
Contributor

pxlrbt commented Aug 17, 2021

@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
Copy link
Member

@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
Copy link
Contributor

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

@ahegyes
Copy link

ahegyes commented Jan 25, 2022

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
Copy link
Member

👍 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
Copy link
Contributor

@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
Copy link
Member

theofidry commented Feb 2, 2022

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
Copy link
Contributor

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

@theofidry
Copy link
Member

theofidry commented Feb 3, 2022

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

I think so. Also might be worth aggregating some into regexes? For example /^_?wp_.+$/. But I notice they are not all prefixed this way

@calvinalkan
Copy link
Contributor

There is no consistency at all.

I generate them all automatically on every release with

https://github.com/sniccowp/php-scoper-excludes

@calvinalkan
Copy link
Contributor

I deliberately made this a little cli app so that you can generate these lists for other external code references aswell, a stub file is not required.

@calvinalkan
Copy link
Contributor

https://github.com/sniccowp/php-scoper-wordpress-excludes

Now contains a list of all auto-updated wordpress functions/classes as json.

The list is checked daily against the latest version of WordPress

@davidmondok
Copy link

davidmondok commented Jun 1, 2022

Thx a lot @calvinalkan for your work! Dumb question, but how would I go about using the generated exclusions in php-scoper?

@calvinalkan
Copy link
Contributor

Thx a lot @calvinalkan for your work! Dumb question, but how would I go about using the generated exclusions in php-scoper?

snicco/php-scoper-wordpress-excludes#3

@theofidry
Copy link
Member

So I think the API for it is now more stable.

Ideally, there would be a project such as https://github.com/snicco/php-scoper-wp-cli-excludes that keeps track of the WP API and then one could do:

$ composer require acme/wp-symbols

And:

// scoper.inc.php

[$wpClasses, $wpConstants, $wpFunctions] = require __DIR__.'/vendor/acme/wp-symbols/symbols.php';

return [
  'exclude-classes' => $wpClasses,
  'exclude-constants' => $wpConstants,
  'exclude-functions' => $wpFunctions,
  // ...
];

And then there could also be more opinionated tools as alternatives.

From PHP-Scoper perspective however, I don't think there is much more to do apart from a doc entry. If one is willing to contribute to one it will be welcomed.

If there is such a project equivalent to the acme/wp-symbols example above, if it helps, I am happy to include it under the humbug umbrella. I think such a project may not be trivial to setup, but from a maintenance perspective this could probably be automated a fair bit.

On that note, I think it's time to close this chapter :)

@calvinalkan
Copy link
Contributor

calvinalkan commented Nov 2, 2023

If there is such a project equivalent to the acme/wp-symbols example above, if it helps, I am happy to include it under the humbug umbrella. I think such a project may not be trivial to setup, but from a maintenance perspective this could probably be automated a fair bit.

On that note, I think it's time to close this chapter :)

Unless I'm missing something, what you describe is already implemented by our repos.

The symbols are auto-generated on each release of WordPress.

Example usage:

https://github.com/GoogleForCreators/web-stories-wp/blob/main/scoper.inc.php#L15

@theofidry
Copy link
Member

theofidry commented Nov 2, 2023

Ho, I did check it but somehow I read "3 years" instead of "3 weeks" 😅

I'm not sure why those are JSONs? But regardless this pretty much does what I think is needed to be done, so if you're wiling I'm happy to have a doc entry for it to recommend it as the canonical way to do it

@calvinalkan
Copy link
Contributor

I'm not sure why those are JSONs?

I don't remember why I chose JSON, probably because it'd be easy to consume in non-PHP use cases.

The CLI tool can generate both JSON and PHP arrays.

https://github.com/snicco/php-scoper-excludes/blob/a094eab86687a40f219c5ceebe9094e3d088fa0a/src/Console/GenerateExcludes.php#L122

@calvinalkan
Copy link
Contributor

calvinalkan commented Nov 2, 2023

so if you're wiling I'm happy to have a doc entry for it to recommend it as the canonical way to do it

@theofidry

Opened: #880

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

No branches or pull requests