pyrocms / lex

A lightweight template parser used by PyroCMS.
110 stars 31 forks source link

Not handling invalid variable gracesfully #2

Closed andrew13 closed 12 years ago

andrew13 commented 12 years ago

If a variable isn't defined it should return false:

{{ if navType == 'forum' }} {{ navigation:links group="subnavforum" }} {{ else }} {{ navigation:links group="subnav" }} {{ endif }}

results in: A PHP Error was encountered Severity: Notice Message: Use of undefined constant navType - assumed 'navType' Filename: Lex/Parser.php(725) : eval()'d code

See Smarty example: http://www.smarty.net/syntax_comparison

PHP <?php if(!empty($foo)): ?> <?php foreach($foo as $bar): ?> <?=$bar['zag']?> <?=$bar['zag2']?> <?=$bar['zag3']?> <?php endforeach; ?> <?php else: ?> There were no rows found. <?php endif; ?>

Smarty {foreach $foo as $bar} {$bar.zag} {$bar.zag2} {$bar.zag3} {foreachelse} There were no rows found. {/foreach}

The empty check should be in the foreach or if checks

philsturgeon commented 12 years ago

Could you wrap that up as a pull request? Much easier to handle that way.

andrew13 commented 12 years ago

Yeah I tried to make it look nice with GitHub Flavored Markdown and broke their parser :)

http://pastebin.com/y8RUrkSE

andrew13 commented 12 years ago

As for the pull request, not sure where I would put in it as I don't know the solution.

dhrrgn commented 12 years ago

If the variable is not defined, it should not return false. This would lead to confusion when the possible value of the variable (if it is defined) is false. null would not work either as that is also a possible value. The correct thing to do here would for us to throw an undefined variable exception (the 'undefined constant' error shouldn't happen).

philsturgeon commented 12 years ago

I'm not sure on how the order of parsing goes, but instead of assuming the variable exists can an isset() check be done?

! isset($foo) and $foo = null;

That just means that if it is null, it will still be null. If its not even set, then it will be null - which is fine because null !== "forum". I can't think of many examples where users would actively be checking === null and I can't think how it would be a problem in any of the examples provided.

dhrrgn commented 12 years ago

What if null is a possible value, but you want to give some sort of message if the variable doesn't exist at all? I am looking into adding a new Lex keyword/function to help here. Asking people's opinions about the syntax: https://twitter.com/dhorrigan/status/250646314365362176

philsturgeon commented 12 years ago

Template syntax is never that complicated. Foreaching through a undefined variable should just not foreach, if'ing on an undefined variable should not error. There has never been an occasion I can think of where somebody in a template would differenciate between null and not set, because its just designers doing if on basic crap.

philsturgeon commented 12 years ago

Besides in PHP itself, if a value is undefined it technically has a null value (once errors are hidden/surpressed) so there IS no difference between null and undefined. If it's undefined it should act like null, and just not error. This error can be hidden by finding out if isset($foo) and if its NOT set give it a null value, meaning the if would act as expected.

dhrrgn commented 12 years ago

That is true. I am adding 'exists' which will act just like isset, and I am also modifying how undefined variables are handled...I will post the result.

dhrrgn commented 12 years ago

Fixed Here (see changelog) https://github.com/fuelphp/lex/commit/a38d61d4e1ea23077e0847e487d91060d7798b2d

andrew13 commented 12 years ago

Solution works for me. Thanks guys.