Bug #87978

Missing declarations in MathExpressionNode.php

Added by Daxboeck no-lastname-given 5 months ago. Updated 3 months ago.

Status:
Needs Feedback
Priority:
Must have
Assignee:
-
Category:
Fluid
Start date:
2019-03-22
Due date:
% Done:

0%

TYPO3 Version:
9
PHP Version:
7.2
Tags:
8LTS, 9LTS missing declarations
Complexity:
no-brainer
Is Regression:
Sprint Focus:

Description

Dear Ladies and Gentlemen,

unfortunately someone closed #85604 and #85741 without implementing the reported fix which is quite a pity !

As the issue was reported more than 6 months ago in 8LTS and still persists in current 9LTS, I kindly ask you to fix MathExpressionNode.php for all these TYPO3 versions:

--- MathExpressionNode.php-2019-03-22-dax 2019-03-22 07:43:16.926881483 0100
+
+ MathExpressionNode.php 2019-03-22 07:44:46.027915978 +0100
@ -70,17 +70,17 @
protected static function evaluateOperation($left, $operator, $right) {
if ($operator === '%') {
- return $left % $right;
+ return (float)$left % (float)$right;
} elseif ($operator === '-') {
- return $left - $right;
+ return (float)$left - (float)$right;
} elseif ($operator === '+') {
- return $left + $right;
+ return (float)$left + (float)$right;
} elseif ($operator === '*') {
- return $left * $right;
+ return (float)$left * (float)$right;
} elseif ($operator === '/') {
- return (integer) $right !== 0 ? $left / $right : 0;
+ return (integer) (float)$right !== 0 ? (float)$left / (float)$right : 0;
} elseif ($operator === '^') {
- return pow($left, $right);
+ return pow((float)$left, (float)$right);
}
return 0;
}

So, please before you close this issue, make sure this patch is actually being included in 8LTS and 9TLS as this is really a no-brainer.

Best regards,

Patrick


Related issues

Duplicates TYPO3 Core - Bug #85604: PHP warning when doing math in Fluid Closed 2018-07-20
Duplicates TYPO3 Core - Bug #85741: Bugfix for MathExpressionNode.php Closed 2018-08-03

History

#1 Updated by Claus Due 5 months ago

Hi Patrick,

First, MathExpressionNode belongs to a different package than TYPO3 CMS. You can find the source code and issue tracker here: https://github.com/TYPO3/Fluid. Mind the PHP namespace if you're in doubt where a class belongs. Repeatedly asking for a fix to be integrated here won't do much since the class is not part of TYPO3 CMS.

Second, this is far from a no-brainer and you can't just cast values, in particular you can't just cast them to floats and expect comparisons to work. But that's not a topic for this issue tracker, just thought I'd mention it if this is a strategy you normally use. Be careful, PHP types can be deceptive. You should try running the unit tests in the Fluid engine repository with that change applied ;)

Also, a reason was actually given for closing both of those issues - two reasons, even. First reason was inactivity above 90 days, second was that https://github.com/TYPO3/Fluid/issues/300 plus https://github.com/TYPO3/Fluid/commit/5b6d5d3a48f4ea9e386a0584336801899d1d16f4 is expected to have solved this issue.

Additionally https://github.com/TYPO3/Fluid/pull/396 has been created to address exactly the "non-numeric" warning but this pull request has not yet been merged. You are welcome to follow the status there (but please avoid the "any updates?" question because the answer will always be no; everything that's being done about the pull request will be in the comment history).

And you are very welcome - any time! - to create pull requests on GitHub to get code merged into Fluid. In this case of course it's not needed because a PR is already open but in similar future cases please come to GitHub and lend a hand ;)

Cheers,
Claus

PS: I'll leave it to you to close the issue. If you wish, you may wait for the PR to be merged and a release to be created.

#2 Updated by Daxboeck no-lastname-given 5 months ago

Dear Claus,

I have been reporting all issues concerning typo3_src-x.x.x.tar.gz here because this place is for me the first contant to report bugs.
Although I personally don't mind reporting bugs also elsewhere. However by telling people to go elsewhere, you achieve that they stop reporting stuff.
I reported this issue half a year ago because after I started to use fluid math, my logs got flooded with error messages. (12GB within 5days).
Doing a Typecast into float ensured to me that basic math did work, so it solved the problem. My templates worked and the log did not get flooded.
Now your answer reveals to me a second issue. Bugs are being fixed somewhere, but the fix is not being pulled for months.
It is my opinion that either TYPO3 will get temporary patches to fix such issues temporarily until new versions of the affected patches are being pulled or that the pull frequency is increased.
It is now half a year since TYPO3 9LTS has been published and there are some severe incompatibility issues which I have been fixing last week and thus reporting.
Honestly that kind of bugs I stumbled over leave quite a bad feeling. 9LTS is more Beta and 8LTS has a clock ticking. Despite all the very great improvements and long overdue changes over the last couple of years in TYPO3 CMS, there are some small things which turn users away.
But you are the wrong guy to write this to. So I will write to Olivier as I love TYPO3, but some minor things give me some real concern.

Thank you very much for the reply and best regards,

Patrick

#3 Updated by Claus Due 5 months ago

Hi Patrick,

I need to address a couple of points you write:

I have been reporting all issues concerning typo3_src-x.x.x.tar.gz here because this place is for me the first contant to report bugs.

This is becoming less and less the case for the average bug that you may encounter. In recent years TYPO3 started using more and more external dependencies instead of custom inventions, which means that the shipped source archive actually contains (more and more) code from third parties. We will of course be happy to receive the information that the bug exists and investigate, but if the bug isn't in TYPO3 source code it may happen that we refer to that and simply close the issue. This is the case where suggesting a patch again is not helpful. To be completely frank: you, as a user, should have checked those links that were given and preferably should also have checked the issue tracker of Fluid for updates on that particular issue.

However by telling people to go elsewhere, you achieve that they stop reporting stuff.

I don't think that's the average reaction. And I sincerely don't think that it's wrong to point to third parties' issue trackers and say "watch for issue status here".

Bugs are being fixed somewhere, but the fix is not being pulled for months.

This is true, but three other things are also true:

1. The third party dependencies of TYPO3 get updated automatically if you use composer.
2. When you do not, the dependencies are locked to whichever version is in composer.lock of TYPO3 itself. Those are the ones that ship in the archive and the archives are final.
3. If we indiscriminately update all third party dependencies with every release we risk breaking a TYPO3 release with no rollback like you could do on composer, therefore we do need to process these carefully and that takes longer.

My overall recommendation for this point is: use composer. Then you decide when/where you wish to update these third party dependencies and do not depend on gatekeeping/processing of a single team.

It is my opinion that either TYPO3 will get temporary patches to fix such issues temporarily until new versions of the affected patches are being pulled or that the pull frequency is increased.

I personally don't think that's a reasonable expectation, that TYPO3 should provide class overrides or forked versions of any third party library for every potential bug. That's an obligation I would not put on any open source project anywhere.

I have to repeat the recommendation to use composer here: this makes it a non-issue. With composer you yourself can even do things like apply pull requests to your installation for those extreme-priority fixes. It really means your experience can be improved if you value immediate updates to all dependencies and perfect control over versions being installed.

Honestly that kind of bugs I stumbled over leave quite a bad feeling.

I'm sorry to hear that - I can't speak for your other experiences but this specific issue was not taken as high priority (hence the six-month wait) because 1) it has not been frequently experienced by a lot of users, 2) the warnings were visible only in Development context or logs, and 3) applied only to a very specific use case which has a large amount of possible workarounds.

But you are the wrong guy to write this to.

Actually, I'm one of the right people to write this to. I'm one of the maintainers of the external Fluid repository so I can provide you with insight from both sides. I read and acknowledge your criticism of the main complaint - the speed with which this patch is being processed - but I also provided an explanation and backgrounds for the decisions.

So I will write to Olivier as I love TYPO3

I'm sure Olivier can help with the user experience and expectation management aspects, and definitely he would appreciate feedback which he always collects, but don't take this the wrong way: he does not dictate the development or shipping strategies. And we all love TYPO3 (or we wouldn't have spent a decade working with it) ;)

#4 Updated by Oliver Hader 5 months ago

  • Duplicates Bug #85604: PHP warning when doing math in Fluid added

#5 Updated by Oliver Hader 5 months ago

  • Duplicates Bug #85741: Bugfix for MathExpressionNode.php added

#6 Updated by Benni Mack 3 months ago

  • Target version changed from next-patchlevel to Candidate for patchlevel

#7 Updated by Benni Mack 3 months ago

  • Status changed from New to Needs Feedback

Hey Patrick,

we've released TYPO3 v9.5.6 with an upgrade to Fluid. Can you let me know if your issue have been solved?

Thanks.
Benni.

Also available in: Atom PDF