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

Fix hanging on some invalid php code #358

Merged
merged 24 commits into from Mar 6, 2020
Merged

Conversation

@50bhan
Copy link
Contributor

50bhan commented Feb 10, 2020

Q A
Bug fix? yes
New feature? no
Fixed tickets #322

This PR will close issue #322.

Due to suppressing errors with "@", process will hangs without any prompt. With this changes, errors will be considered but user will not see them during process. At the end of process, "insight" report will be contains of occurred errors as parse errors.

50bhan added 3 commits Feb 10, 2020
@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Feb 13, 2020

When running on a file with the following input

<?php
class MyClass extends Illuminate\Database\Eloquent\Model; {}

It is stuck in an infinite loop, where the console keeps spamming

Notice: Undefined index: scope_closer in /Users/olivernybroe/Code/phpinsights/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/UseStatementHelper.php on line 195
PHP Notice:  Undefined index: scope_closer in /Users/olivernybroe/Code/phpinsights/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/UseStatementHelper.php on line 195

Looks like it cannot break out of this while statement
https://github.com/slevomat/coding-standard/blob/master/SlevomatCodingStandard/Helpers/UseStatementHelper.php#L187

@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Feb 13, 2020

@olivernybroe It is exactly what's happening. That infinite loop is causing a problem because of invalid token. Now if you remove "@" and use my solution with Try{} and empty catch, insights will work perfectly and you can see this bad code report at the end of process ("Unparsable php code: syntax error or wrong phpdocs.")

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Feb 13, 2020

@50bhan Yes, but I am actually still getting this error when using your branch.

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Feb 13, 2020

@50bhan I Just added a test for it, so you can see what I mean with it is still failing for me.

@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Feb 13, 2020

@olivernybroe Thanks! I'm working on it :)

50bhan added 3 commits Feb 16, 2020
@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Feb 16, 2020

@olivernybroe After couple of hours struggling, following result achieved:

  • With my primary changes (removing "@" and add an empty catch in FixerFileProcessor.php), the bug will be fixed. Of course your provided test will be failed but if you install my branch on an actual project, it will work perfectly.

  • Cause of test failure: In previous test, we were actually testing sniffer library and not our code! Because in following code we were specific about code flow and control of code directly passed to another library which we can't control:

        $file = self::prepareFixtureWithSniff(
            UselessAliasSniff::class,
            __DIR__ . '/Fixtures/InvalidPhpCode/SemiColonAfterExtendClass.php'
        );

And in that library E-NOTICE errors somehow are suppressed, so code will stuck in an infinite loop. But in actual project, we run insight from CLI and code flow is much different from our test and we can catch errors with my solution.

So I changed test in two independent ways (just for demonstration). We can change analyzer to detect this kind of bad code, but it will be beneficial just for testing purposes. Another way was activate error reporting to catch occurred exception! I don't know which one (or no one) of them is good, so please share your opinion.

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Feb 17, 2020

@50bhan Oh wauw, sorry about that, I totally forgot that, that method is actually running the phpcs library directly 🤦‍♂

Option 1 (change analyzer): This is nice, but could result in extending the analyzer with a lot of edge cases, which makes extending the analyzer harder (which already is kinda hard). However I think we can benefit other places than in test only from this, but primarily it will prob. affect tests.

Option 2 (Error reporting): I think this makes a lot of sense. Looking into it this is also what phpstan does
https://github.com/phpstan/phpstan-src/blob/master/bin/phpstan#L9
and psalm https://github.com/vimeo/psalm/blob/master/src/psalm.php#L15 and propably also other tools.

I think I'll prefer option 2, even though how you implemented option 1 is nice.
@50bhan What do you prefer yourself here?

@Jibbarth @nunomaduro Could we get a third opinion from one of you?

@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Feb 17, 2020

@olivernybroe Actually changing analyzer is nice but if we can really benefit from it (at this moment I couldn't find any specific use case beside one we have). But using error reporting has less trouble and we can use it in such scenarios.

So if we can't find another way, we should go with error reporting, unless we can gain much from analyzer change.

I appreciate if any of guys has a third option or any opinion on current ones.

@olivernybroe olivernybroe linked an issue that may be closed by this pull request Feb 18, 2020
src/Domain/Analyser.php Outdated Show resolved Hide resolved
src/Domain/Analyser.php Outdated Show resolved Hide resolved
src/Domain/Insights/ForbiddenBadClasses.php Outdated Show resolved Hide resolved
src/Domain/Insights/ForbiddenBadClasses.php Outdated Show resolved Hide resolved
src/Domain/Analyser.php Outdated Show resolved Hide resolved
@Jibbarth

This comment has been minimized.

Copy link
Collaborator

Jibbarth commented Feb 24, 2020

Option 1 (change analyzer): This is nice, but could result in extending the analyzer with a lot of edge cases, which makes extending the analyzer harder (which already is kinda hard). However I think we can benefit other places than in test only from this, but primarily it will prob. affect tests.

Option 2 (Error reporting): I think this makes a lot of sense. Looking into it this is also what phpstan does
phpstan/phpstan-src:bin/phpstan@master#L9
and psalm vimeo/psalm:src/psalm.php@master#L15 and propably also other tools.

I have no really opinion on this, I follow yoy @olivernybroe, I'm 👍 to add error reporting

@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Feb 24, 2020

@Jibbarth Thanks for the review! Actually I wrote them just for the demo to choose between two options or even a new one. I believe if there is no third option at this point, error reporting is a better way, unless @olivernybroe has a different opinion.

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Feb 25, 2020

@50bhan Alright, let's go error reporting 👍

Thanks for the amazing research here and examples of how to code will look like.

50bhan added 2 commits Feb 28, 2020
@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Feb 28, 2020

@olivernybroe As a recap, for fixing this bug, I just removed "@" to avoid suppressing errors and receive all of them (and avoid hangs on bad codes). Then insight will log the error and pass the file to the fixer and because of bad token, fixer will fail. So I added an empty exception handling block just to avoid failing.

Regarding error reporting, because of removing "@", it is not needed unless in testing which I decided to change it. I just tested the file fixer, because second point of failure was there and testing via analyzer was not a choice anymore.

P.S. I couldn't find a better way to test the file fixer! Let me know if there's room for improvement.

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Mar 4, 2020

@50bhan Sounds cool!

Hmm, so I just tried running it locally on the test fixture file and it still gives the infinite recursion result showing these two lines

Notice: Undefined index: scope_closer in /Users/uruloke/Code/phpinsights/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/UseStatementHelper.php on line 195
PHP Notice:  Undefined index: scope_closer in /Users/uruloke/Code/phpinsights/vendor/slevomat/coding-standard/SlevomatCodingStandard/Helpers/UseStatementHelper.php on line 195
50bhan added 2 commits Mar 4, 2020
@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Mar 4, 2020

@olivernybroe I added an error handler to "SniffDecorator" to active error reporting and catch fatal errors, then convert the errors to exception so it become catch-able via try/catch block. This is the result after this change:

[ARCHITECTURE] 70 pts within 1 files

Classes ...................................................... 100.0 %
Interfaces ..................................................... 0.0 %
Globally ....................................................... 0.0 %
Traits ......................................................... 0.0 %

[MISC] 96.4 pts on coding style

• [Code] Unused private elements:
  badClass.php:2: Unparsable php code: syntax error or wrong phpdocs.

• [Code] Useless alias:
  badClass.php:1: Unparsable php code: syntax error or wrong phpdocs.

• [Code] Declare strict types:
  badClass.php:1: Missing declare(strict_types=1).

• [Architecture] Normal classes are forbidden. Classes must be final or abstract:
  badClass.php

• [Architecture] Valid class name:
  badClass.php:2: Possible parse error: class missing opening or closing brace

• [Architecture] Property per class limit:
  badClass.php:2: Unparsable php code: syntax error or wrong phpdocs.

• [Architecture] Superfluous abstract class naming:
  badClass.php:2: Unparsable php code: syntax error or wrong phpdocs.

• [Architecture] Superfluous exception naming:
  badClass.php:2: Unparsable php code: syntax error or wrong phpdocs.

• [Architecture] Useless alias:
  badClass.php:1: Unparsable php code: syntax error or wrong phpdocs.

• [Style] Alphabetically sorted uses:
  badClass.php:1: Unparsable php code: syntax error or wrong phpdocs.

• [Style] Unused uses:
  badClass.php:1: Unparsable php code: syntax error or wrong phpdocs.

• [Style] Use spacing:
  badClass.php:1: Unparsable php code: syntax error or wrong phpdocs.
@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Mar 5, 2020

Alright, so I just tested out locally and the new changes made it work!
image

Seems to also work if I change my error reporting level in php.ini

olivernybroe added 2 commits Mar 5, 2020
Copy link
Collaborator

olivernybroe left a comment

Amazing work @50bhan!

Thanks for spending all this time debugging the error and finding a suitable solution.

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Mar 5, 2020

Alright, so @50bhan when the tests are fixed, I'll say we are ready to merge this in 👍

50bhan added 2 commits Mar 5, 2020
WIP
WIP
@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Mar 5, 2020

@olivernybroe It's my pleasure! It was a nasty bug 😆 but it looks like we are ready to go!

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Mar 5, 2020

@50bhan Nice that you found a solution 👍 I had no idea how to solve it 😟

@Jibbarth I have tested the functionality and it seems to work, so can you make one of your amazing reviews 🙏

Copy link
Collaborator

Jibbarth left a comment

Last question, promise.

In our phpstan.neon.dist, line 33, we have '#Error suppression via "@" should not be used.#'

As we remove the @, could we remove this line in phpstan.neon.dist ?

@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Mar 5, 2020

@Jibbarth Nice catch 😄 but there is one other place remain with @ usage. I just removed it and it seems everything is fine.

Copy link
Collaborator

Jibbarth left a comment

LGTM 👍 Thanks @50bhan

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Mar 6, 2020

@50bhan Sorry, but removing that other one will cause new errors 😉

PHP Warning:  Unterminated comment starting line 10 in  on line 10

I have added a test case for you 👍

The places where the php warning is happening is
Analyserline 87
and
FixerFileProcessor line 63 and 77.

@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Mar 6, 2020

@olivernybroe So should I put @ back to the code to just fix this bug and close this PR or you want me to find a solution for this one as well?

@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Mar 6, 2020

@olivernybroe I suggest to put than one back, unless we can catch errors which we couldn't catch already!

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Mar 6, 2020

@50bhan Actually adding that back in won't fix it, you would have to add it back in and add it to FixerFIleProcessor also.

If you are up for finding a solution then let's do that. If you wanna wait or want this fix rolled out, then let's just make the solution for this other problem in another PR.

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Mar 6, 2020

Oops forgot to change my commit message to this branch 🤦‍♂

Oh well, we squash merge anyways

@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Mar 6, 2020

@olivernybroe Sounds like a plan! Let's make this one work and I will work on other one as well.

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Mar 6, 2020

@50bhan Cool!

Thanks a lot for the work on this btw, really nice to get the internals cleaned up and more stable.

@50bhan

This comment has been minimized.

Copy link
Contributor Author

50bhan commented Mar 6, 2020

@olivernybroe Any time :) Tests are failing because of low code quality?! Any ideas?

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

olivernybroe commented Mar 6, 2020

@50bhan I have taken care of it, just waiting for travis and then I'll merge it 😄

@olivernybroe olivernybroe merged commit 5935ae0 into nunomaduro:master Mar 6, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@50bhan 50bhan deleted the 50bhan:bug/fix-hangs branch Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.