Skip to content

Replace PHP-Scoper name resolver by the PHP-Parser one (Part 1) #496

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

Merged
merged 3 commits into from
Jun 14, 2021

Conversation

theofidry
Copy link
Member

This is a continuation of #400 but taking an easier starting path. I had some misunderstanding about PHP-Parser's name resolver, and from the doc (see https://github.com/nikic/PHP-Parser/blob/master/doc/component/Name_resolution.markdown#name-resolution) it actually replaces all name nodes by its fully qualified form whenever possible. Whilst it's possible to toggle off the replace, I believe this will lead to simpler cases in PHP-Scoper so we might as well go for it.

However it currently breaks all the tests so need to fix that first

@theofidry
Copy link
Member Author

@marekdedic I believe quite a few cases broken related to not adding prefixes. I'm trying to look at it but can't find a solution at the moment. If you have any lead it would be very welcomed

@marekdedic
Copy link
Contributor

Hi,
I will try to fix what I broke... :D

Any good place where to start? E. g. a test that fails because of prefixes...

@theofidry
Copy link
Member Author

I'm really not sure... This PR is kinda of a start from #400 already :P i couldn't check in much more details after scoping this down a bit

@theofidry
Copy link
Member Author

@marekdedic I think I have a good lead, I hope I will have something out very soon

@theofidry
Copy link
Member Author

There is more changes & cleanup I wish to do but this PR is the first step and doing the strict minimum: register the PHP-Parser name resolver visitor and fixing the tests

@theofidry theofidry changed the title WIP: Replace PHP-Scoper name resolver by the PHP-Parser one (Part 1) Replace PHP-Scoper name resolver by the PHP-Parser one (Part 1) Jun 14, 2021
@marekdedic
Copy link
Contributor

marekdedic commented Jun 14, 2021

Hi,
I looked at the failing tests and it seems to me that they have nothing to do with prefixing - I see

$ bin/box compile
Fatal error: Uncaught Error: Call to undefined method Humbug\PhpScoper\Configuration::load() in /home/user/php-scoper/vendor/humbug/box/src/Configuration/Configuration.php:2700

Looking at the file in question, it seems like an issue with box - it tries to call a non-existent method.

I can't see how this is related to any of the changes you made though...

@theofidry
Copy link
Member Author

No the e2e tests failure are expected: it's due to a BC break of the config

@marekdedic
Copy link
Contributor

Ok, ok, so is there any issue with the "simplified" prefixes then?

@theofidry
Copy link
Member Author

Not that I can see unless you see any offender there in the change of the tests

Copy link
Contributor

@marekdedic marekdedic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I am still a bit confused by whitelisting, though...

@theofidry
Copy link
Member Author

I'll come up with a rename to remove the confusion hopefully

@theofidry theofidry merged commit 565b8dc into humbug:master Jun 14, 2021
@theofidry theofidry deleted the refactor-name-resolution branch June 14, 2021 22:06
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

Successfully merging this pull request may close these issues.

2 participants