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

[BUGFIX] Cast string values to float or int #773

Merged
merged 4 commits into from
Jun 7, 2023

Conversation

tlueder
Copy link
Contributor

@tlueder tlueder commented May 11, 2023

Fixes #672

@tlueder tlueder changed the title Cast string values to float or int [BUGFIX] Cast string values to float or int May 11, 2023
@s2b
Copy link
Contributor

s2b commented May 11, 2023

@tlueder
Copy link
Contributor Author

tlueder commented May 11, 2023

I'm not sure why the tests are not failing without this patch.

@s2b
Copy link
Contributor

s2b commented May 11, 2023

I had the same problem at first: The problem only occurs with variables that are passed as string. The existing test cases use integers.

@tlueder
Copy link
Contributor Author

tlueder commented May 11, 2023

Nope, thats not it.

I removed my patch and added a var_dump

    protected static function evaluateOperation($left, $operator, $right)
    {
        var_dump($left);

to src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php

and run ./vendor/bin/phpunit -v

This is the result:

PHPUnit 10.1.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.17
Configuration: /Users/tlueder/VSCodeProjects/Typo3/Fluid/phpunit.xml.dist

.............................................................   61 / 1012 (  6%)
.............................................................  122 / 1012 ( 12%)
.............................................................  183 / 1012 ( 18%)
...............................................string(1) "1"
int(0)
.string(1) "1"
.string(1) "1"
.string(1) "2"
.string(1) "2"
.string(1) "2"
.string(1) "4"
.string(1) "4"
.int(1)
.string(1) "1"
.int(1)
.string(1) "1"
...  244 / 1012 ( 24%)
.............................................................  305 / 1012 ( 30%)
.............................................................  366 / 1012 ( 36%)
.............................................................  427 / 1012 ( 42%)
.............................................................  488 / 1012 ( 48%)
.............................................................  549 / 1012 ( 54%)
.............................................................  610 / 1012 ( 60%)
.............................................................  671 / 1012 ( 66%)
.............................................................  732 / 1012 ( 72%)
.............................................................  793 / 1012 ( 78%)
.............................................................  854 / 1012 ( 84%)
.............................................................  915 / 1012 ( 90%)
.............................................................  976 / 1012 ( 96%)
....................................                          1012 / 1012 (100%)

Time: 00:00.353, Memory: 26.00 MB

OK (1012 tests, 2022 assertions)

The tests are using both int and string and should fail, but aren't.

String + String works fine in the unit test but not in a real project.

@tlueder
Copy link
Contributor Author

tlueder commented May 11, 2023

I can get the test to fail only if I add:

declare(strict_types=1);

And change:

protected static function evaluateOperation($left, $operator, $right)

to

protected static function evaluateOperation(int|float $left, ?string $operator, int|float $right)

Here the test result:


PHPUnit 10.1.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.6
Configuration: /Users/tlueder/VSCodeProjects/Typo3/Fluid/phpunit.xml.dist

.............................................................   61 / 1012 (  6%)
.............................................................  122 / 1012 ( 12%)
.............................................................  183 / 1012 ( 18%)
...............................................EEEEEEEEEE.E..  244 / 1012 ( 24%)
.............................................................  305 / 1012 ( 30%)
.............................................................  366 / 1012 ( 36%)
.............................................................  427 / 1012 ( 42%)
.............................................................  488 / 1012 ( 48%)
.............................................................  549 / 1012 ( 54%)
.............................................................  610 / 1012 ( 60%)
.............................................................  671 / 1012 ( 66%)
.............................................................  732 / 1012 ( 72%)
.............................................................  793 / 1012 ( 78%)
..................................E..........................  854 / 1012 ( 84%)
.............................................................  915 / 1012 ( 90%)
.............................................................  976 / 1012 ( 96%)
....................................                          1012 / 1012 (100%)

Time: 00:00.347, Memory: 26.00 MB

There were 12 errors:

1) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #0
TypeError: TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\Expression\MathExpressionNode::evaluateOperation(): Argument #1 ($left) must be of type int|float, string given, called in /Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php on line 62

/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:74
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:62
/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:47

2) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #1
TypeError: TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\Expression\MathExpressionNode::evaluateOperation(): Argument #1 ($left) must be of type int|float, string given, called in /Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php on line 62

/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:74
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:62
/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:47

3) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #2
TypeError: TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\Expression\MathExpressionNode::evaluateOperation(): Argument #1 ($left) must be of type int|float, string given, called in /Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php on line 62

/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:74
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:62
/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:47

4) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #3
TypeError: TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\Expression\MathExpressionNode::evaluateOperation(): Argument #1 ($left) must be of type int|float, string given, called in /Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php on line 62

/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:74
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:62
/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:47

5) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #4
TypeError: TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\Expression\MathExpressionNode::evaluateOperation(): Argument #1 ($left) must be of type int|float, string given, called in /Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php on line 62

/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:74
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:62
/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:47

6) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #5
TypeError: TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\Expression\MathExpressionNode::evaluateOperation(): Argument #1 ($left) must be of type int|float, string given, called in /Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php on line 62

/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:74
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:62
/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:47

7) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #6
TypeError: TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\Expression\MathExpressionNode::evaluateOperation(): Argument #1 ($left) must be of type int|float, string given, called in /Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php on line 62

/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:74
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:62
/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:47

8) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #7
TypeError: TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\Expression\MathExpressionNode::evaluateOperation(): Argument #1 ($left) must be of type int|float, string given, called in /Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php on line 62

/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:74
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:62
/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:47

9) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #8
TypeError: TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\Expression\MathExpressionNode::evaluateOperation(): Argument #3 ($right) must be of type int|float, string given, called in /Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php on line 62

/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:74
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:62
/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:47

10) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #9
TypeError: TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\Expression\MathExpressionNode::evaluateOperation(): Argument #1 ($left) must be of type int|float, string given, called in /Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php on line 62

/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:74
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:62
/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:47

11) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #11
TypeError: TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\Expression\MathExpressionNode::evaluateOperation(): Argument #1 ($left) must be of type int|float, string given, called in /Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php on line 62

/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:74
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:62
/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:47

12) TYPO3Fluid\Fluid\Tests\Functional\ExamplesTest::exampleScriptValues with data set "example_math.php"
TypeError: TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\Expression\MathExpressionNode::evaluateOperation(): Argument #3 ($right) must be of type int|float, string given, called in /Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php on line 62

/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:74
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/Core/Parser/SyntaxTree/Expression/MathExpressionNode.php:62
/private/var/folders/xy/fsw2pq4x3f7639p4y70sbtkm0000gn/T/fluid-examples/Default_action_Default_ae95cb433c550b5ab9d345d5360ee34be73aa932.php:37
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/View/AbstractTemplateView.php:257
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/ViewHelpers/RenderViewHelper.php:172
/private/var/folders/xy/fsw2pq4x3f7639p4y70sbtkm0000gn/T/fluid-examples/layout_Default_html_cd8722f7e13b9ba5b231f6aa5b0028e31e735f90.php:56
/Users/tlueder/VSCodeProjects/Typo3/Fluid/src/View/AbstractTemplateView.php:199
/Users/tlueder/VSCodeProjects/Typo3/Fluid/examples/example_math.php:36
/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Functional/ExamplesTest.php:223

--

There were 12 risky tests:

1) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #0
This test did not perform any assertions

/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:43

2) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #1
This test did not perform any assertions

/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:43

3) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #2
This test did not perform any assertions

/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:43

4) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #3
This test did not perform any assertions

/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:43

5) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #4
This test did not perform any assertions

/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:43

6) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #5
This test did not perform any assertions

/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:43

7) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #6
This test did not perform any assertions

/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:43

8) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #7
This test did not perform any assertions

/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:43

9) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #8
This test did not perform any assertions

/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:43

10) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #9
This test did not perform any assertions

/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:43

11) TYPO3Fluid\Fluid\Tests\Unit\Core\Parser\SyntaxTree\Expression\MathExpressionNodeTest::testEvaluateExpression with data set #11
This test did not perform any assertions

/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Unit/Core/Parser/SyntaxTree/Expression/MathExpressionNodeTest.php:43

12) TYPO3Fluid\Fluid\Tests\Functional\ExamplesTest::exampleScriptValues with data set "example_math.php"
* Test code or tested code did not close its own output buffers

* This test did not perform any assertions

/Users/tlueder/VSCodeProjects/Typo3/Fluid/tests/Functional/ExamplesTest.php:212

ERRORS!
Tests: 1012, Assertions: 2001, Errors: 12, Risky: 12.

If I apply my patch now all errors are gone again.

@s2b
Copy link
Contributor

s2b commented May 11, 2023

Okay... A functional test with the exact code from the issue passes. I also tested it in a TYPO3 v12 project in development context, no warnings.

Maybe you need to check first under which conditions this is reproducible in a TYPO3 project (or otherwise). Nevertheless, it can't hurt to add the test cases.

@s2b
Copy link
Contributor

s2b commented May 11, 2023

@tlueder
Copy link
Contributor Author

tlueder commented May 11, 2023

My project is Typo3 v11 and I've tested it with development context and live context.

I don't think we need new tests, as the existing tests are already passing strings to the function.
The issue with the existing one is that they are not failing.

@s2b
Copy link
Contributor

s2b commented May 11, 2023

I just set up typo3 v11 with ddev (php 8.2, development context, TYPO3's debug options enabled). I can't reproduce the error.

Could you look into how your setup might differ from the default setup? Maybe we need to change something in the Fluid testing setup to cover your configuration.

@tlueder
Copy link
Contributor Author

tlueder commented May 11, 2023

I run into this error today and found this issue.

My local dev environment is a Docker and composer setup.

TYPO3 Version 11.5.27
Webserver nginx/1.18.0
PHP Version 8.0.27
Debugger xdebug 3.2.0
Database (Default) MySQL 10.3.36-MariaDB-1:10.3.36+maria~ubu2004
Application Context Development
Composer mode Enabled
Operating System Linux 5.15.49-linuxkit

Which is different from the one in the issue. So it can't be that.

As I also can't reproduce this with the UnitTest and only in a working project, my guess is that some setting related to strong types must be different.

That is why I have opened #778.
If you apply this patch without this one, the UnitTests fail as expected.

If you apply both, all tests are passing again.

@tlueder
Copy link
Contributor Author

tlueder commented May 30, 2023

Hi, any news on how to progress here?

@lolli42
Copy link
Member

lolli42 commented Jun 7, 2023

The change is fine codewise, I suppose. I don't fully understand why the tests did not find this, but I don't think it is currently necessary to fully understand this, either. We'll merge the patch and will harden the class as prepared.

@tlueder
Copy link
Contributor Author

tlueder commented Jun 7, 2023

The test would have found it with this #778

@lolli42
Copy link
Member

lolli42 commented Jun 7, 2023

The test would have found it with this #778

I know. I'll merge #778 afterwards.

@lolli42 lolli42 disabled auto-merge June 7, 2023 09:07
@lolli42
Copy link
Member

lolli42 commented Jun 7, 2023

@tlueder I need to rebase or merge the PR with current main before merging. Unfortunately, pushing to your branch is denied for me. I'll thus cherry-pick your changes to an own branch now and will push as new PR.

@tlueder
Copy link
Contributor Author

tlueder commented Jun 7, 2023

Ok, or I could have rebased it for you, what ever gets the job done.

@tlueder tlueder force-pushed the Unsupported-operand-types branch from 1345d52 to 0bb8a86 Compare June 7, 2023 09:16
@lolli42 lolli42 merged commit f4fce3b into TYPO3:main Jun 7, 2023
2 checks passed
@lolli42
Copy link
Member

lolli42 commented Jun 7, 2023

Thanks!

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

Successfully merging this pull request may close these issues.

Unsupported operand types: string - string
3 participants