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

Whitelisting #192

Closed
sebastianbergmann opened this issue Apr 7, 2018 · 13 comments
Closed

Whitelisting #192

sebastianbergmann opened this issue Apr 7, 2018 · 13 comments

Comments

@sebastianbergmann
Copy link
Contributor

Hi!

At the PHPUnit Code Sprint here in Hamburg yesterday and today, @kambo-1st worked on sebastianbergmann/phpunit#2015. Based on his findings, we came to the conclusion that (at least) the {classes|interfaces|traits} of phpunit/phpunit and phpunit/phpunit-mock-objects need to be whitelisted when building a prefixed PHAR. By whitelisting we mean that the names of these units of code are not prefixed. The code that declares these units of code needs to be processed, though, as it need to be updated to used the prefixed names of the other dependencies.

A problem that we ran into is the fact that php-scoper requires a list of the classes to be whitelisted. Is there any way that whitelisting could work based on namespace names or (FQCN) class name prefixes?

@theofidry
Copy link
Member

It's currently not possible. So far I was more focused on getting something that was working well, but looking at PHPUnit codebase I think it's a clear requirement: listing every classes would be too cumbersome for a project of that size plus there is the risk of leaving out some classes.

Right now the syntax for whitelisting classes is:

return [
    'whitelist' => [
        'PHPUnit\Framework\TestCase',
    ],
];

So here are two questions for me regarding that feature, first would the following syntax work out for you?

return [
    'whitelist' => [
        'PHPUnit\Framework\*',
    ],
];

Second question: the whitelisting currently applies only to classes, should it also be applied to constants and functions?

That second question is more tricky than it looks. Indeed imagine you have the following code:

namespace Acme;

const FOO = 'FOO';

function foo() {};

class X {};

Supposing we are whitelisting everything here, right now what you would get is:

namespace PhpScoperPrefix\Acme;

const FOO = 'FOO';

function foo() {};

class X {};

class_alias('PhpScoperPrefix\Acme\X', 'Acme\X', false); # this is the actual whitelisting happening

So as you can see, the whitelisting is right now affecting only classes, not constants neither functions. Would that be an issue? Should the whitelisting affect constants and functions as well and if yes how should that be achieved? The issue is unlike classes you cannot rely on a class_alias trick and besides that, constants can be defined dynamically:

define('Acme\FOO', 'FOO');

PS: I also think #175 would be mandatory in your case.

@sebastianbergmann
Copy link
Contributor Author

@theofidry I did not look at this myself recently, @kambo-1st spent Friday and Saturday working on this during the code sprint. He promised to summarize his findings in a comment on sebastianbergmann/phpunit#2015 and I am sure he will.

In the meantime, I think that @kambo-1st managed to create a PHAR of PHPUnit that is processed by php-scoper with the classes and interfaces of PHPUnit and phpunit-mock-objects whitelisted. For now, I believe, that he put logic into the php-scoper configuration file that crawls the code to find the classes and interfaces that should be on the whitelist.

Regarding your questions:

This syntax

return [
    'whitelist' => [
        'PHPUnit\Framework\*',
    ],
];

makes sense to me, thank you for considering to add this.

When you write

"the whitelisting currently applies only to classes"

then you mean that whitelisting currently only applies to classes, interfaces, and traits -- right?

With regards to constants and functions: I need to think about this some more.

@theofidry
Copy link
Member

then you mean that whitelisting currently only applies to classes, interfaces, and traits -- right?

Yes

@kambo-1st
Copy link

@theofidry Is there any way how can I help you with this? Should I try to implement this by myself?

@theofidry
Copy link
Member

theofidry commented Apr 17, 2018

I think before jumping to the implementation, the first thing that needs to be done is thinking of the API (as a consumer). How do we want to make it work basically. I've been thinking about it for a while but I'm still not quite certain of the best way to go. Maybe you have some idea as a user of what you would wish to have?

I was thinking of something like:

'whitelist' => [
  'PHPUnit\Framework*',
],

or

'whitelist' => [
  'PHPUnit\Framework' => 'namespace',
  'PHPUnit\FOO' => 'const',
  'PHPUnit\foo' => 'func',
],

@sebastianbergmann
Copy link
Contributor Author

Both make sense, I guess.

@kambo-1st
Copy link

Same goes for me, both make sense. Personally I prefer the second option.

@TomasVotruba
Copy link
Contributor

I prefer first one, as more intuitive and all I need

@theofidry
Copy link
Member

theofidry commented Jun 5, 2018

Follow up in #213 (regarding the namespace whitelisting). It's not finished yet but on the way and with luck finished this week

@TomasVotruba
Copy link
Contributor

(I guess you meant #213)

@theofidry
Copy link
Member

indeed, wrong copy paste :)

theofidry added a commit that referenced this issue Jun 7, 2018
Closes #192

Allow to whitelist a complete namespace with the `*` notation, e.g. `*` (for the global namespace) or `Acme\*`

**[BC break]:** the API signature has changed to convert the `array` `$whitelist` into the `Whitelist` object
@sebastianbergmann
Copy link
Contributor Author

@theofidry Thank you!

@kambo-1st Will you update sebastianbergmann/phpunit#3086?

@TomasVotruba
Copy link
Contributor

Thank you, can't wait to try it

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

No branches or pull requests

4 participants