search

Talesoft / tale-jade

A complete and fully-functional implementation of the Jade template language for PHP
http://jade.talesoft.codes
MIT License
88 stars 10 forks source link

Conditional attributes: Broken since 1.3.1 #100

Closed ponychicken closed 8 years ago

ponychicken commented 8 years ago

This used to work in 1.2:

li(class=true ? 'highlight' : '')

After version 1.3.1 it throws an error: Attributes in elements and mixins need a name.

TorbenKoehn commented 8 years ago

Try using () around expressions with : (ternary operator, mostly). I'll have a look at this.

http://sandbox.jade.talesoft.io/id-574c014e65c4a.html

ponychicken commented 8 years ago

Ok, thanks, at least it works with braces. It's still a regression, imo, since Jade supports it without them.

TorbenKoehn commented 8 years ago

The new lexer in the token-enhancements branch is a huge step forward and will probably fix this, but sadly, it also requires a new parser and compiler. The parser is already finished in the parser-enhancements branch, but the compiler will probably still take a while because of missing time (see compiler-enhancements branch) :(

1.3.1 brought functions in attributes, which is implemented pretty broken right now. The logic for the attribute lexing starts here exactly if you're interested in taking a look at it. 1.5 will use the Tale Reader as well as a different lexer structure. This here would be the new parsing algorithm based on the reader.

I'll try to get this finished as soon as possible. Maybe I (or you) can find a quick fix.

TorbenKoehn commented 8 years ago

I fixed this in the latest patch. Please pull *@dev and try it.

Ternary operators at least should be handled better now, even though I'm really not satisfied with the solution.

Let me explain the problem a bit:

What you can do in Jade (and now in Tale Jade) is the following:

a(href=true ? 'a' : 'b' title='some title')

This is absolutely valid Jade. Notice the space placement and the removal of commas. It renders to the following PHTML (mostly, I removed Tale Jade sugar)

<a href="<?=true ? 'a' : 'b'?>" title="some title"></a>

Now imagine a parser parsing that attribute line there. A normal space (`) acts as a separator for attributes just as well as a comma (,`)

As soon as you use spaces in your ternary operator, Tale Jade assumed that you're trying to create another attribute.

First attribute: href = true Second attribute: ? Third attribute: 'a' Fourth attribute: : Fifth attribute: 'b' Sixth attribute: title = 'some title'

As anything that doesn't look like ...?[a-zA-Z\_][a-zA-Z0-9\_] is not a valid attribute name, it switched to using it as the value.

Elements can't have attributes that have no name, but they received multiple attributes without a name and values of ?, 'a', :, 'b' and so on.

That's why that exception occured and that's why it was the absolute correct exception for this error.

Now usually the bracket algorithm catches these cases and counts bracket opening/closing in order to keep your expressions complete. That's why putting brackets around it works, as soon as you do ( it enters an expression and keeps reading it until it found the corresponding ).

You'd have had the same result if you wouldn't have used any spaces in your ternary expression. Don't take my word for it, here is the proof (It's not updated to 1.4.5 yet)

What I am doing now is really static stuff. After reading an attribute value, I also check for the existence of a ? after that value (Notice ?!=-assignments for values have been handled at that point already, no misleading possible here). If there is a ?, it will assume there's a ternary expression coming. It will do another expression read for the middle (if) value, check for a : to end the ternary and read another expression for the last (else) value. After that it goes on reading normally.

I will improve this to also handle ?: and ?? correctly soon. ?: could work out of the box already.

Please test this if it is fixed for you and give me feedback.

Thanks for sticking to Tale Jade :)

TorbenKoehn commented 8 years ago

Here now, it's supported for all ternaries (I know of): https://github.com/Talesoft/tale-jade/commit/f611a935255ba76c6556ea45211c856881f21e11

Oh also, the reason why it broke in 1.3.1 was simply the fact that prior to 1.3.1 only commas were allowed as attribute separators, not spaces :)

TorbenKoehn commented 8 years ago

I'll close this.

If you have further issues with this, feel free to re-open it :)