-
Notifications
You must be signed in to change notification settings - Fork 93
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
[FEATURE] Allow null coalesce operator in evaluation (#522) #572
base: main
Are you sure you want to change the base?
Conversation
['{a ?? b ?? c}', ['a' => 'd', 'b' => 'e', 'c' => 'f'], 'd'], | ||
['{a ?? b ?? c}', ['a' => 'd', 'b' => null, 'c' => 'f'], 'd'], | ||
['{a ?? b ?? c}', ['a' => null, 'b' => 'e', 'c' => 'f'], 'e'], | ||
['{a ?? b ?? c}', ['a' => null, 'b' => null, 'c' => 'f'], 'f'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 4 cases can be dropped since they are basically the same as the 4 before them, only a bit more confusing. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, absolutely, but I guess I'll leave them, because in the beginning, $a
was always equals to a
which ended in some false positives that I tried to avoid with this test, but yeah, makes sense to remove them, although assuming that '1'
is interpreted as string instead of an integer might be a bit of a strech. I'll have to test it :-)
I would strongly urge to not support quoted strings because aribrary strings within inline syntax has a very high risk of breaking the parsing engine in v2. While we can support simple strings, inviting people to use syntax which contains odd characters or even nested Fluid code will inevitably lead to some nasty edge cases or unpredictable parsing. Even for v3 I would not invite such usage, although unexpected parsing behavior is much less of an issue on that version. |
Ok, so only this syntax should be allowed? <span>{ model.field ?? Default }</span> Or would you just keep one of two possible string quotations? Is the parser able to recognize something like this? <span>{ model.field ?? Sorry, this value has not been set. }</span> |
No, what I mean is we shouldn't allow arbitrary strings at all - quoted or otherwise. Basically, treat everything as a variable. The problem is that the parser (v2, being regexp-based) is only able to recognize a very specific set of characters as part of an inline expression and arbitrary strings have a very high likelihood of containing something that causes the expression to not be detected as Fluid at all. We could make an exception that allows using hardcoded numeric values since they don't carry a risk of breaking the detection patterns, though. |
Got it, so if I want to have a string as fallback, I'd proceed something like this: <f:variable name="default" value="This is my default value" />
<span>{model.field ?? default }<span> That sounds very good to me, I'll try to think about it when documenting this feature. |
use TYPO3Fluid\Fluid\Core\Variables\VariableExtractor; | ||
|
||
/** | ||
* Ternary Condition Node - allows the shorthand version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CDRO, the comments in this file still refer to the ternary operator...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out, I've corrected this now.
No description provided.