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 php checker #1045

Merged

Conversation

@zonuexe
Copy link
Member

zonuexe commented Aug 19, 2016

Motivation

PHP script can include shebang. But php -l behave differ from when read using STDIN and when using file.

Reproduce

#!/usr/bin/env php
<?php

namespace Flycheck\Sample;

echo "Hello", PHP_EOL;

Current

2016-08-19 20 30 10

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Aug 19, 2016

CLA assistant check
All committers have signed the CLA.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

lunaryorn commented Aug 19, 2016

@zonuexe Thanks, but please provide a more detailed explanation of the problem. Specifically try to explain why standard input is an issue, and how exactly the behaviour differs. Please be aware that I do not know PHP.

@lunaryorn lunaryorn changed the title Fix PHP checker Do not use standard input for php checker Aug 19, 2016
@zonuexe

This comment has been minimized.

Copy link
Member Author

zonuexe commented Aug 19, 2016

@lunaryorn thanks for response.

PHP 7.0.9 is current stable version.

% php --version
PHP 7.0.9 (cli) (built: Jul 21 2016 14:50:47) ( NTS )
Copyright (c) 1997-2016 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2016 Zend Technologies
    with Xdebug v2.4.1, Copyright (c) 2002-2016, by Derick Rethans
% cat ./sample
#!/usr/bin/env php
<?php

namespace Flycheck\Sample;

echo "Hello", PHP_EOL;
% chmod +x ./sample
% ./sample
Hello
% php -l -d error_reporting=E_ALL -d display_errors=1 -d log_errors=0 ./sample
No syntax errors detected in ./sample
% php -l -d error_reporting=E_ALL -d display_errors=1 -d log_errors=0 < ./sample

Fatal error: Namespace declaration statement has to be the very first statement or after any declare call in the script in - on line 4

Errors parsing -
@zonuexe zonuexe changed the title Do not use standard input for php checker Fix php checker Aug 19, 2016
@lunaryorn

This comment has been minimized.

Copy link
Contributor

lunaryorn commented Aug 25, 2016

@zonuexe Uh, oh 😳 PHP ignores the shebang when reading from standard input? Why's that? Can you find out? Maybe there's another way around this issue that doesn't require us dropping standard input support (which we should do only as the very very last resort if no other option remains).

@zonuexe

This comment has been minimized.

Copy link
Member Author

zonuexe commented Aug 25, 2016

Hmm... This is difficult problem.

PHP was originally a template engine. Therefore, outside of PHP tags (<?php … ?>) content are standard output.

However, because PHP is also a scripting language, do special handling only if executing the file from command line.

% cat hello2.php
#!/usr/bin/env php
<?php

echo "Hello, 2", PHP_EOL;
% php hello2.php
Hello, 2
% php -e < ./hello2.php
#!/usr/bin/env php
<?php

In after case (using stdin), #!/usr/bin/env php means echo '#!/usr/bin/env php';.
But, hello2.php is not warned by php -l. The script does not illegal to have echo statement.

The problem surfaced when combined namespace statement and the shebang.

http://php.net/manual/en/language.namespaces.definition.php

Namespaces are declared using the namespace keyword. A file containing a namespace must declare the namespace at the top of the file before any other code - with one exception: the declare keyword.

Because it can not exist any statement in front of the namespace declaration, this script is essentially illegal. It is overlooked only if it is run as *a script file* from command line.


For PHP users (including me), probably to write a shell script using PHP is only few part of the work. There is no efficient degrade the Flycheck performance because of that.

If there is no way to avoid this problem in Flycheck, I could close this PR.

@lunaryorn Do you have any good ideas?

@lunaryorn

This comment has been minimized.

Copy link
Contributor

lunaryorn commented Aug 25, 2016

@zonuexe Mh, yes, I see. I can't say that I like it, but it appears that in this case we really have no other choice, so LGTM. @cpitclaudel ?

@cpitclaudel

This comment has been minimized.

Copy link
Member

cpitclaudel commented Aug 25, 2016

Hmm. Don't we need to revert the error patterns, too? Currently the error patterns hardocde "-", but I imagine that this change will put actual file names back in the error messages, right?

@zonuexe

This comment has been minimized.

Copy link
Member Author

zonuexe commented Aug 25, 2016

@cpitclaudel
Thanks for your review. It looks like have been fixed.

% cat ~/junk/hello.php
#!/usr/bin/env php
<?php

namespace Flycheck\Sample;

// missing "f"
unction hello () {
}
% /usr/bin/php --version
PHP 5.5.36 (cli) (built: May 29 2016 01:07:06)
Copyright (c) 1997-2015 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2015 Zend Technologies

% /usr/bin/php ~/junk/hello.php

Parse error: parse error in /Users/megurine/junk/hello.php on line 7
% php --version
PHP 7.0.10 (cli) (built: Aug 21 2016 19:14:33) ( NTS )
Copyright (c) 1997-2016 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2016 Zend Technologies
    with Xdebug v2.4.1, Copyright (c) 2002-2016, by Derick Rethans

% php ~/junk/hello.php
PHP Parse error:  syntax error, unexpected 'hello' (T_STRING) in /Users/megurine/junk/hello.php on line 7

Parse error: syntax error, unexpected 'hello' (T_STRING) in /Users/megurine/junk/hello.php on line 7

2016-08-26 3 52 21

@lunaryorn

This comment has been minimized.

Copy link
Contributor

lunaryorn commented Aug 26, 2016

@zonuexe I'm sorry, you lost me 😞 So, is the issue now fixed in the latest PHP version? Should we close this pull request?

@zonuexe

This comment has been minimized.

Copy link
Member Author

zonuexe commented Aug 26, 2016

@lunaryorn

So, is the issue now fixed in the latest PHP version?

No.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

lunaryorn commented Aug 26, 2016

@zonuexe I see. Can you check whether make LANGUAGE=php integ still passes?

@zonuexe

This comment has been minimized.

Copy link
Member Author

zonuexe commented Aug 26, 2016

@lunaryorn

I got the output, such as:

% make LANGUAGE=php integ
cask exec emacs -Q --batch -L .  -l maint/flycheck-compile.el -f flycheck/batch-byte-compile flycheck-buttercup.el
Warning: Lisp directory `/usr/local/Cellar/emacs/24.5/share/emacs/24.5/lisp/emacs-parallel': No such file or directory
Warning: Lisp directory `/usr/local/Cellar/emacs/24.5/share/emacs/24.5/lisp/emacs-parallel': No such file or directory
cask exec emacs -Q --batch -L .  --load test/run.el -f flycheck-run-tests-main \
            '(and (tag external-tool) (language php))'
Warning: Lisp directory `/usr/local/Cellar/emacs/24.5/share/emacs/24.5/lisp/emacs-parallel': No such file or directory
Warning: Lisp directory `/usr/local/Cellar/emacs/24.5/share/emacs/24.5/lisp/emacs-parallel': No such file or directory
Running tests on Emacs 24.5.1, built at 2016-01-18
Loading /Users/megurine/repo/emacs/flycheck/flycheck...
Loading /Users/megurine/repo/emacs/flycheck/flycheck-ert...
Loading /Users/megurine/repo/emacs/flycheck/test/flycheck-test.el (source)...
Finding all versions of R on your system...
Sorry, no version of R could be found on your system.
Running 2 tests (2016-08-26 18:28:30+0900)
   passed  1/2  flycheck-define-checker/php/default
   passed  2/2  flycheck-define-checker/php/syntax-error

Ran 2 tests, 2 results as expected (2016-08-26 18:28:31+0900)
@lunaryorn

This comment has been minimized.

Copy link
Contributor

lunaryorn commented Aug 26, 2016

LGTM, then

@cpitclaudel

This comment has been minimized.

Copy link
Member

cpitclaudel commented Aug 26, 2016

Thanks, LGTM too.

@cpitclaudel cpitclaudel merged commit 82ed054 into flycheck:master Aug 26, 2016
3 checks passed
3 checks passed
approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@zonuexe zonuexe deleted the zonuexe:feature/php-checker-not-use-stdin branch Aug 26, 2016
@zonuexe

This comment has been minimized.

Copy link
Member Author

zonuexe commented Aug 26, 2016

@lunaryorn @cpitclaudel Thank you very much!

@lunaryorn

This comment has been minimized.

Copy link
Contributor

lunaryorn commented Aug 26, 2016

You're welcome. Thanks a lot for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.