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 abstract/interface methods in PEAR.Functions.FunctionDeclaration #2148

Merged

Conversation

thewilkybarkid
Copy link
Contributor

Currently methods without a body fail to have the whitespace before the opening parenthesis and before the semicolon removed.

@@ -105,7 +105,7 @@ public function process(File $phpcsFile, $stackPtr)
// Unfinished closures are tokenized as T_FUNCTION however, and can be excluded
// by checking for the scope_opener.
if ($tokens[$stackPtr]['code'] === T_FUNCTION
&& isset($tokens[$stackPtr]['scope_opener']) === true
&& (isset($tokens[$stackPtr]['scope_opener']) === true || $tokens[$stackPtr]['level'] === 1)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that checking the token level is the best way to do this. It wouldn't work, for example, if you had the class wrapped in a condition (like an autoloader maybe) and while I can't think of why you'd define an abstract method in an anon class, it wouldn't catch that either. It's probably best to call File::getMethodProperties() and check the isAbstract array index to be sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a failing test in eeb758a, will have another look at the implementation.

@thewilkybarkid thewilkybarkid force-pushed the function-declaration-no-body branch from 80a1e00 to 3ef5716 Compare October 13, 2018 19:47
@thewilkybarkid
Copy link
Contributor Author

@gsherwood Couldn't use is_abstract as it doesn't cover methods on interfaces, but used the same technique to determine if there's a body or not.

@gsherwood gsherwood merged commit 3ef5716 into squizlabs:master Oct 23, 2018
gsherwood added a commit that referenced this pull request Oct 23, 2018
@gsherwood
Copy link
Member

Thanks a lot for making those changes and including tests.

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