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

Conditional attributes: Broken since 1.3.1 #100

Closed
ponychicken opened this issue May 26, 2016 · 6 comments
Closed

Conditional attributes: Broken since 1.3.1 #100

ponychicken opened this issue May 26, 2016 · 6 comments
Milestone

Comments

@ponychicken
Copy link
Contributor

ponychicken commented May 26, 2016

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
Copy link
Member

TorbenKoehn commented May 30, 2016

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

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

@ponychicken
Copy link
Contributor Author

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

@TorbenKoehn
Copy link
Member

TorbenKoehn commented May 30, 2016

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 TorbenKoehn added this to the 1.5 milestone May 30, 2016
TorbenKoehn pushed a commit that referenced this issue Aug 23, 2016
Updated composer.json

Updated Symfony mb polyfill to 1.2.0

Addressed #100, still not satisfied with the solution (but it works well, for ternary operators at least)

Addressed #109, the ignored variables are now a constant Tale\Jade\Compiler\IGNORED_SCOPE_VARIABLES. Define it prior to loading Tale Jade to specify own ignored variables (separated by `:`)
@TorbenKoehn
Copy link
Member

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
Copy link
Member

Here now, it's supported for all ternaries (I know of):
f611a93

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
Copy link
Member

I'll close this.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants