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

Add support for functions polyfills for the requirement checker #131

Closed
theofidry opened this issue Apr 20, 2018 · 9 comments
Closed

Add support for functions polyfills for the requirement checker #131

theofidry opened this issue Apr 20, 2018 · 9 comments

Comments

@theofidry
Copy link
Member

theofidry commented Apr 20, 2018

The requirement checker currently extension based polyfills. In other words, if the project or a dependency requires a certain extension, when it is declared in their composer.json files like so:

{
    "require": {
         "ext-mcrypt": "*",
         "ext-mbstring": "*"
    }
}

The requirement checker will be able to pick those requirements. It also supports a range of polyfills. So with the example above, if phpseclib/mcrypt_compat is found in the installed dependencies (not as a dev dependency), the extension mcrypt will actually not be required. Likewise, if symfony/polyfill-mbstring is found, it will not require ext-mbstring.

However no function support is currently provided. For example if the project requires the Intl grapheme_* functions, there is no way to tell the requirement checker that those functions are required and that they are not if the symfony/polyfill-intl-grapheme package is installed

@theofidry theofidry changed the title Add polyfills for the requirement checker Add support for functions polyfills for the requirement checker Apr 28, 2018
@theofidry
Copy link
Member Author

@MacFJA if you have any idea of how it could be implemented, I'm all ears. Otherwise I'm for closing this as I'm not sure it's actually possible...

@MacFJA
Copy link

MacFJA commented Apr 28, 2018

I'm working on it.

I should be able to have something to show soon

@MacFJA
Copy link

MacFJA commented Apr 29, 2018

Here the solution that I came up: https://github.com/MacFJA/polyfill-registry

I still need to make some work (add a README, add examples, add to packagist, etc.) but it should work.

To summarize, the code will be something like:

$registry = new PolyfillRegistry();
// ...
$polyfilledExtensions = [];
foreach ($packages as $packageInfo) {
    $polyfilledExtensions = array_merge($polyfilledExtensions, $registry->getPolyfillExtensions($packageInfo['name']));
}
$polyfilledExtensions = array_filter(array_unique($polyfilledExtensions));
$polyfilledExtensions = array_fill_keys(array_values(polyfilledExtensions), true);

The huge drawback of my solution is the list of polyfill to keep up to date

@theofidry
Copy link
Member Author

The huge drawback of my solution is the list of polyfill to keep up to date

Not so much of drawback since it's the same for what is currently done in Box. Maybe a small difference is that right now Box supports some come to be polyfills since it relies on a package name pattern for the Symfony polyfills.

So there's still two things I'm wondering:

  • Is declaring all the functions necessary? There is some polyfills covering only a few functions of an extension as opposed to the whole extension and this is where it's necessary. But otherwise maybe you could omit it which would lower the maintenance work on your end.

  • How does one declares that his application requires those functions which are either provided by an extension or polyfills? And actually when is it needed (I realise I struggle to find a real case scenario)?

In any case I think having a polyfill registry would be a good idea. I would just suggest to support some "pattern based" polyfills like for the Symfony ones to reduce the work.

@MacFJA
Copy link

MacFJA commented Apr 29, 2018

How does one declares that his application requires those functions which are either provided by an extension or polyfills? And actually when is it needed (I realise I struggle to find a real case scenario)?

A tool like PhpCodeAnalyser can found functions/constants/classes of extensions used.
(I'm planning to integrated it into phpqa, maybe I can make it usable as a lib and not only as a CLI)

Is declaring all the functions necessary? There is some polyfills covering only a few functions of an extension as opposed to the whole extension and this is where it's necessary. But otherwise maybe you could omit it which would lower the maintenance work on your end.

As I design the tool to work in both way (find a polyfill for an extension and find extensions of a polyfill), I need to have a relativily precise dataset.
For the maintenance cost I planning to create some helper tools to make most of the work

@theofidry
Copy link
Member Author

Sorry I may not have been clear enough for the second point. Here's the scenarios I can think of:

  1. The application depends on some extension functions but has a fallback. For example psysh has a soft dependency on ext-pcntl. However it won't ever get checked by the requirement checker since at no point the app says "I require the extension pcntl".

  2. The application depends on a certain extension, e.g. mbstring, but it has a polyfill symfony/polyfill-mbstring: the requirement checker supports it so there is nothing more to do.

  3. The application execute third-party code and requires that third-party code to have an extension e.g. mbstring. In that case, it's a different level of complexity since it entirely depends on how the application loads that third-party code. In which case I would argue it's not the responsibility of the requirement-checker to check that. If anything eventually, what would be missing on Box end is to allow the application the run without bailing out because the mbstring extension is missing (right now you could only disable the requirement checker entirely)

@MacFJA
Copy link

MacFJA commented Apr 29, 2018

The application depends on some extension functions but has a fallback. For example psysh has a soft dependency on ext-pcntl. However it won't ever get checked by the requirement checker since at no point the app says "I require the extension pcntl".

For the case of psysh we have an indirect requirement as the composer.json (and lock) have a suggest section where to specify those soft dependency. The requirement checker can warn the user (to indicate that some features can be missing) about those extension if no polyfill exists but as there are soft dependency they should, in theory, works without them. And, IMO, the requirement checker mustn't be blocked by them.

The application execute third-party code and requires that third-party code to have an extension e.g. mbstring. In that case, it's a different level of complexity since it entirely depends on how the application loads that third-party code. In which case I would argue it's not the responsibility of the requirement-checker to check that. If anything eventually, what would be missing on Box end is to allow the application the run without bailing out because the mbstring extension is missing (right now you could only disable the requirement checker entirely)

The composer.lock is a flatted version of all composer.json, so required extensions of third-party code should appear in it (as well as the suggest section). Also, when Composer do its dependency resolution, this case is handled: Composer check all dependencies requirements (from root package level to the deepest dependency)

@theofidry
Copy link
Member Author

I think relying on the suggest is harder:

  • It can be anything, not necessarily a soft dependency
  • In the case of psysh for example, it actually includes those suggest at build time to provide a PHAR with all the suggested polyfills (and I think that's how it should be done)

So I don't think there is anything else to do here.

The composer.lock is a flatted version of all composer.json, so required extensions of third-party code should appear in it (as well as the suggest section). Also, when Composer do its dependency resolution, this case is handled: Composer check all dependencies requirements (from root package level to the deepest dependency)

Sure but how can you know that the application needs to load third-party code and how it should do it? You could very well have:

$ acme.phar scan project # Loads the code
$ acme.phar fix file # does not load the code

So making any assumption here looks very brittle

@theofidry
Copy link
Member Author

Bump on the issue.

However no function support is currently provided. For example if the project requires the Intl grapheme_* functions, there is no way to tell the requirement checker that those functions are required and that they are not if the symfony/polyfill-intl-grapheme package is installed

I think it's actually not an issue per se since there is no way to tell Composer you have a dependency on grapheme_* functions...

@MacFJA unless I'm missing something, I think we can close the issue for now. I still think the idea of a polyfill registry is a good one but maybe we don't need to keep track of the functions.

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

2 participants