Skip to content

Filter out falsy nodes in blocks#85

Merged
ichiriac merged 2 commits intoglayzzle:masterfrom
motiz88:fix-84
Aug 22, 2017
Merged

Filter out falsy nodes in blocks#85
ichiriac merged 2 commits intoglayzzle:masterfrom
motiz88:fix-84

Conversation

@motiz88
Copy link
Copy Markdown
Contributor

@motiz88 motiz88 commented Aug 17, 2017

Fixes #84 and adds a regression test. This is perhaps a crude fix, as I chose not to hunt down the specific null-producing cases in e.g. parser/if.js, but rather to remove them after the fact in the Block constructor.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 94.401% when pulling 1e18f48 on motiz88:fix-84 into a99ae87 on glayzzle:master.

@ichiriac
Copy link
Copy Markdown
Member

Hi @motiz88,

Thanks for the fix. I've digged into the issue, it's caused by this code : https://github.com/glayzzle/php-parser/blob/master/src/parser/statement.js#L317

It can also be reproduced by this code :

<?php if (true): ; ; ?>
<?php endif; ?>

The actual fix works on every case, but may slow down a bit the AST building process. I'm looking at a more early filtering solution.

On the other hand, the bracket { ... } form does not have the problem because the falsy values are filtered here : https://github.com/glayzzle/php-parser/blob/master/src/parser/statement.js#L83

I've also found a helper here https://github.com/glayzzle/php-parser/blob/master/src/parser/utils.js#L15 used on loops (could be fixed and used with multiple tokens)

Do you want to digg more ?

If you don't have the time I can merge the fix and continue to correct accordingly.

@ichiriac ichiriac merged commit 6c4249e into glayzzle:master Aug 22, 2017
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.

Null node found in short-form if body

3 participants