php-scoper icon indicating copy to clipboard operation
php-scoper copied to clipboard

[WIP] Better WordPress support

Open pxlrbt opened this issue 3 years ago • 12 comments

This PR includes the extractor and patcher from pxlrbt/php-scoper-prefix-remover into PHP-Scoper core to provide better WordPress plugin/theme support out of the box. See https://github.com/humbug/php-scoper/issues/303

Added an extractor to extract the WordPress identifiers from the files, a patcher to remove the prefix on the given identifiers, tests and documentation on how to use it.

Couldn't figure out how to expose the extractor/patcher to be used inside the scoper.inc.php file. Any feedback is welcome.

pxlrbt avatar Nov 09 '20 16:11 pxlrbt

Thanks for the draft @pxlrbt, I'll check it out soonish

theofidry avatar Nov 09 '20 17:11 theofidry

@theofidry Would appreciate if you could point me into the right direction on how to expose classes from the phar to the outside of the phar. For me that's the last part to get it working. The rest is a bit of tweaking and polishing where needed.

pxlrbt avatar Nov 09 '20 17:11 pxlrbt

IMO those classes are not outside of the Phar. Any code in the Phar has same access to all the things. The config, AFAIK, is included by the application that runs from the Phar. So, the thus included config file has all the same symbols available under the same autoloading conditions than any other code in it. I am puzzled as to why it does not work.

XedinUnknown avatar Nov 09 '20 21:11 XedinUnknown

Whilst I am very grateful for the efforts, I am sorry to say I'm not fond of the approach. The main reason is that while RemovePrefixFromIdentifiersPatcher is internal, it's working back with regexes completely instead of leveraging the AST & Reflection. Besides this is applied as a patch, i.e. after scoping, instead of preventing the scoping of the problematic code in the first place.

So whilst this can work perfectly fine as a hotfix or workaround, I'm afraid it may be too risky/fragile for having it work that way internally.

An additional reason is that I believe internally PHP-Scoper already has the appropriate extension point in place. It is only missing exposing it to the user, see https://github.com/humbug/php-scoper/issues/303#issuecomment-591283618 and https://github.com/humbug/php-scoper/issues/303#issuecomment-614097867.

To maybe elaborate more on this point, the Reflector class is what PHP-Scoper uses to know whether a symbol is internal or not, and if a symbol is internal, then it will not scope it (e.g. it will leave Iterable alone). So instead of patching a scoped file, we could feed the Reflector with symbols to consider as internal and everything should work just fine

theofidry avatar Nov 09 '20 22:11 theofidry

Thanks for the feedback. I totally agree. I tried to add my hotfix into PHP-Scoper without exploring the new options I have using PHP-Scoper internals.

Add https://github.com/php-stubs/wordpress-stubs (assume this is the one you use right?) to PHP-Scoper as a dependency

You proposed to add the stubs to PHP-Scoper. This would reduce the time of each PHP-Scoper run as the identifiers only need to be extracted on build and can be shipped with the phar. On the other hand it's puts extra maintenance on you as the stubs need to be updated regularly. I'd leave this to the user.

add an option to the php-scoper.inc.tpl template, e.g. internalSymbols?

I can think of two options here:

  1. Easiest: Add an option which is pointing to stub files and extract the identifiers/symbols from there. This would decrease performance as it needs to extract every time.
  2. As proposed: Add an internalSymbols option. Then we could expose the extractor which could be used at the start of the scoper.in.php and passed to the internalSymbols option. Could be cached in a further step.
  3. Make the whitelist option work with non-autoloaded symbols. This could be a breaking change (?!)

For option 2: I still didn't figure out how to expose a class from the inside of the phar file to be used in scoper.inc.php. Can you point me into the right direction?

pxlrbt avatar Nov 10 '20 07:11 pxlrbt

Make the whitelist option work with non-autoloaded symbols. This could be a breaking change (?!)

I don't really see how this can work as PHP-Scoper does not autoload the code, so it cannot know what is autoloaded or not easily.

As proposed: Add an internalSymbols option. Then we could expose the extractor which could be used at the start of the scoper.in.php and passed to the internalSymbols option. Could be cached in a further step.

To be clear, I would like internalSymbols to be a plain array (as like in the Reflector) since this is neither WP specific neither require a stub file. I'm however fine with exposing an extractor class.

For option 2: I still didn't figure out how to expose a class from the inside of the phar file to be used in scoper.inc.php. Can you point me into the right direction?

It's a combination of two things:

  • register a dedicated fix alias: https://github.com/humbug/php-scoper/blob/master/bin/php-scoper#L54
  • ensure that the later is not altered by the scoping https://github.com/humbug/php-scoper/blob/master/scoper.inc.php#L99

Although I'm wondering if I could not simply register the Isolated\* namespace to the whitelist

theofidry avatar Nov 10 '20 08:11 theofidry

Would it be possible to simply merge the results of IdentifierExtractor#extract() into the whitelist array in the configuration itself? Would just need to solve the problem of some autoloaded symbols somehow not being accessible in the config file.

Here we can see that the config file is simply included in PHP like any other code. This means that any symbols available in PHPScoper should be available in the config file, because it is also inside PHPScoper by being included. Why would it not be able to find the ParserFactory class? 🤔

XedinUnknown avatar Nov 10 '20 09:11 XedinUnknown

@XedinUnknown

This means that any symbols available in PHPScoper should be available in the config file, because it is also inside PHPScoper by being included. Why would it not be able to find the ParserFactory class? 🤔

That's because PHP-Scoper is scoping itself before bundling, the ParserFactory is prefixed and therefore not found.

@theofidry

To be clear, I would like internalSymbols to be a plain array (as like in the Reflector) since this is neither WP specific neither require a stub file. I'm however fine with exposing an extractor class.

Yes, that's what I understood.

It's a combination of two things:

  • register a dedicated fix alias: https://github.com/humbug/php-scoper/blob/master/bin/php-scoper#L54
  • ensure that the later is not altered by the scoping https://github.com/humbug/php-scoper/blob/master/scoper.inc.php#L99

Weird. That's exactly what I did and it told me that IdentifierExtractor was not found. I will give it another try.

pxlrbt avatar Nov 10 '20 09:11 pxlrbt

That's because PHP-Scoper is scoping itself before bundling, the ParserFactory is prefixed and therefore not found.

Just found that out for myself, and was gonna share here. How come Finder is not scoped, though?

Also, here's an example of the class name: _HumbugBoxb6056d4e1e8c\PhpParser\ParserFactory. It looks auto-generated. Would it be possible for PHPScoper to exclude certain classes like this one from being prefixed, either by creating a class alias or by ignoring them altogether? Alternatively, would it be possible to add it to the Isolated namespace? As a last resort, maybe create aliases for classes like that one, or not scope them altogether?

XedinUnknown avatar Nov 10 '20 10:11 XedinUnknown

I temporarily patched the problem by creating a class alias in the config file. The scoping ran no problem, but it still prefixed WP functions. I realize now that it's probably because the whitelist works by creating aliases. But then, I would also expect the wordpress-stubs.php file to get prefixes and aliases as well, but that didn't happen. What gives?

XedinUnknown avatar Nov 10 '20 10:11 XedinUnknown

@XedinUnknown

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.

I answer here to keep that bundled to the PR. I managed to expose the IdentifierExtractor. At the moment my main issue is to pass the configuration to the Reflector class as there are tons of classes in between. Will stop for today.

pxlrbt avatar Nov 10 '20 11:11 pxlrbt

I'm looking around and I found this pull request. The PHP Scoper works great for me for scoping for WordPress, maybe you want to check my implementation :)

https://gitlab.com/wpify/scoper

Dan

mejta avatar Feb 19 '22 20:02 mejta

Hi! Any movement here, @pxlrbt?

XedinUnknown avatar Feb 17 '23 22:02 XedinUnknown

@XedinUnknown Seems like I deleted the repo with the PR during a clean up. I don't think this will land in PhpScoper Core and I moved away from WordPress, so I probably won't use this in the future.

pxlrbt avatar Feb 18 '23 10:02 pxlrbt

A lot has been happening in PHP-Scoper since then as well. I need to sort out some stuff on Box side first but I otherwise think the WP integration can be greatly improved in a near future

theofidry avatar Feb 18 '23 13:02 theofidry