Fluid icon indicating copy to clipboard operation
Fluid copied to clipboard

Automatic conversion of arguments to numbers is lossy

Open yol opened this issue 6 years ago • 9 comments

ViewHelper arguments that "look like numbers" are automatically converted to NumericNode here: https://github.com/TYPO3/Fluid/blob/master/src/Core/Parser/TemplateParser.php#L545

This is a problem because the argument might really be a string and must be treated as such, especially when the argument was registered as type string.

Example: <f:format.htmlspecialchars value="01234" /> gives 1234 (because the argument is converted to a number and then back) while it should be giving 01234.

yol avatar Feb 27 '18 08:02 yol

While I understand the problem, there are some challenges and disconnects that mean this isn't a simple matter:

  • NumericNode is a parse-time construct and it only gets used for hardcoded numbers
  • The node may exist at any point: nested in an array or isolated like in your example
  • When represented the way it is in your example, there is no way to further distinguish your number from a string (in inline syntax, quotes indicate you wanted a string - but in tag mode, quotes are attribute value delimiters also for numbers)
  • Because of the second point above, the node itself is completely disconnected from the ViewHelper and vice versa: the ViewHelper won't receive the node until it is already created with that value - and at the time the node is created, there is no knowledge in the TemplateParser which arguments a VH supports and which type it has
  • The node type also has implied meaning when compiling the template (as in, values may be cast)

However, when you put your number in a variable where it has an explicit PHP type, this type will be preserved (unless you force it to become a string through other formatters). This means that declaring a variable $view->assign('myString', '01234'); this is clearly a string value and when parsed, instead of a NumericNode you will get an ObjectAccessorNode which transparently returns the exact value and type of the variable you assigned.

So while your very specific (and frankly, extremely rare!) use case does amount to some confusion in the TemplateParser, it 1) is very far from easy to solve and in particular has no sensible solution to be aware of a certain ViewHelper argument's required type, and 2) there's a fairly easy workaround which although it isn't as easy as hardcoding the number, means you completely control which type it has (including if you wanted to use an octal).

I lean towards a "won't solve" response here. And I leave you with a teaser to say that perhaps, one day, there will be a complete rewrite of the way the parser works, which refactors those internals in a way that arguments actually don't get analysed at all until the VH is ready to be called - which when done correctly actually means your use case becomes natively supported through type coercion and strict checking (there will simply no longer be any NumericNode; it becomes redundant and string will be the default type for anything hardcoded). But that's a bit into the future and currently just a pet project.

NamelessCoder avatar Feb 27 '18 17:02 NamelessCoder

Some thoughts about this:

First place, the given example doesn't seem to be of real nature but rather a theoretical thing. (Why would you escape a static number?)

Second, I agree with Claus that this is a very rare use case. I thought a bit of "where could this hit myself" and I didn't find a case yet.

In regards to parsing and stuff: @NamelessCoder Why does the parser need to decide a node-type? It could take all arguments as strings. (Actually it's not even the job of a parser.) I would have expected such type conversion to happen, when a value is used. So for arguments you can decide on a cast when you apply it to an argument of a VH, where expected type information shall be present.

In general I can support a "won't fix" here, since rewriting the whole parsing and evaluation stuff is quite some tough work, with little benefit currently.

liayn avatar Feb 27 '18 17:02 liayn

A valid example might be https://forge.typo3.org/issues/83323

<f:form.select property="minute" options="{0: '00', 5: '05', 15: 15, 30: 30, 45: 45}" />

05 is transformed into 5 but funny enough it still works on https://fluidtypo3.org/library/try-fluid-now.html?

georgringer avatar Feb 27 '18 20:02 georgringer

First place, the given example doesn't seem to be of real nature but rather a theoretical thing. (Why would you escape a static number?)

It's an MWE, I didn't try to make it especially purposeful :-)

So to give you something better (@georgringer also has a realistic use-case though): I tried to convert phone numbers entered by the user like 0123 / 3111-4 into tel: links and auto-prefix them with country code if none was specified. I tried this: <a href="tel:{phone -> v:format.pregReplace(pattern: '/[^0-9+]/', replacement: '') -> v:format.pregReplace(pattern: '/^0', replacement: '+49')}">{phone}</a>

It does not work because +49 gets truncated to 49. Of course there are a myriad ways to solve this. But from just looking at the code above it is extremely non-obvious to me that it would not work in the first place. See, I was so flabbergasted that I even looked into the parser to see wth was going on :-)

@NamelessCoder Why does the parser need to decide a node-type? It could take all arguments as strings. (Actually it's not even the job of a parser.)

If the parser cannot decide if a node is numeric or not, I think then it shouldn't do it. I recognize that changing this might require a refactoring, so I can understand if it's not going to be fixed at this point. Then it should at least be very well documented ...

but funny enough it still works on https://fluidtypo3.org/library/try-fluid-now.html?

Interestingly my initial MWE also works there. Which makes me think that this got broken recently maybe? And that's also why not that many people are complaining yet?

yol avatar Feb 28 '18 08:02 yol

<a href="tel:{phone -> v:format.pregReplace(pattern: '/[^0-9+]/', replacement: '') -> v:format.pregReplace(pattern: '/^0', replacement: '+49')}">{phone}</a>

That's indeed a good one. Didn't think of the plus being eaten up by is_numeric.

liayn avatar Feb 28 '18 08:02 liayn

@NamelessCoder Why does the parser need to decide a node-type? It could take all arguments as strings. (Actually it's not even the job of a parser.)

I don't know, you'd have to ask Sebastian Kurfuerst back in 2008/2009. It's not the way I would have chosen to solve that particular thing - see also my little teaser about changing the parser (away from regexp, to lexing, and yielding nodes not in bulk but only those actually needed).

Everyone: the workaround I described works for all of these examples, each and every one. I'm going to take on the academic professor hat now and say: nearly all of the examples you posted really shouldn't be hardcoded into templates, with the exception of Georg's example about minute suffixes like 05 written as option in a selector.

I still lean towards a "won't solve" - but that doesn't mean I disagree with you about how the parser should be handling such values, just that it is so ingrained and breaking to fix that it makes no sense trying to tackle this one separately from the other vision about not using regexp in the first place.

NamelessCoder avatar Feb 28 '18 10:02 NamelessCoder

away from regexp, to lexing

to be very honest: I'm damn surprised how well regexp actually worked for us so far. But I'm fully aware that this is a dead end at some point.

liayn avatar Feb 28 '18 12:02 liayn

We have a similar problem. In our case it is necessary to configure the ViewHelper with formatted numbers in German locale. Something like: <namespace:render arguments="{value: '3.000'}" /> (meaning three thousand with thousands separator).

And the markup output just contains a 3 as value.

Jabychan avatar Dec 07 '18 15:12 Jabychan

@Jabychan please review my earlier notes in this thread, there are several workarounds at this time and they now include f:variable through which you can assign a variable directly in the template. Use tag mode to define the value as tag content and it will be a TextNode.

Your use case, like the earlier presented ones, indicate a somewhat bad practice in that you're hardcoding numbers with a certain regional formatting. Such variables should come from assigned template variables or model properties and ideally would not be formatted at all when you receive them into the template; unless the use case specifically has to do so (e.g. passing options to f:form.select doesn't provide you with a means to decide things like number formatting for each option value).

I still lean towards a "won't solve" and I think it's time we reach a decision whether to keep this issue open.

NamelessCoder avatar Dec 07 '18 15:12 NamelessCoder