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

1.19: Mysterious TypeError with strict_types=1 #1229

Closed
tvdijen opened this issue Nov 7, 2019 · 6 comments
Closed

1.19: Mysterious TypeError with strict_types=1 #1229

tvdijen opened this issue Nov 7, 2019 · 6 comments
Milestone

Comments

@tvdijen
Copy link
Member

@tvdijen tvdijen commented Nov 7, 2019

Describe the bug
After fiddling around with declare(strict_types=1); on all files in the lib-directory, the following TypeError was thrown:

  1. SimpleSAML\Test\Web\TemplateTest::testSyntax
    TypeError: strtr() expects parameter 1 to be string, null given

/apps/development/simplesamlphp-php70/lib/SimpleSAML/Locale/Translate.php:477
/apps/development/simplesamlphp-php70/vendor/simplesamlphp/twig-configurable-i18n/src/Twig/Extensions/Extension/I18n.php:74
/apps/development/simplesamlphp-php70/vendor/twig/twig/src/Template.php:216
/apps/development/simplesamlphp-php70/vendor/twig/twig/src/Template.php:216
/apps/development/simplesamlphp-php70/vendor/twig/twig/src/Template.php:455
/apps/development/simplesamlphp-php70/vendor/twig/twig/src/Template.php:422
/apps/development/simplesamlphp-php70/vendor/twig/twig/src/Template.php:455
/apps/development/simplesamlphp-php70/vendor/twig/twig/src/Template.php:422
/apps/development/simplesamlphp-php70/vendor/twig/twig/src/Template.php:434
/apps/development/simplesamlphp-php70/vendor/twig/twig/src/TemplateWrapper.php:47
/apps/development/simplesamlphp-php70/vendor/twig/twig/src/Environment.php:384
/apps/development/simplesamlphp-php70/lib/SimpleSAML/XHTML/Template.php:514
/apps/development/simplesamlphp-php70/lib/SimpleSAML/XHTML/Template.php:543
/apps/development/simplesamlphp-php70/tests/www/TemplateTest.php:39

After a lot of debugging and banging my head against the wall, I have managed to trace this issue back to this line in a Twig-template:
https://github.com/simplesamlphp/simplesamlphp/blob/master/templates/error.twig#L6

Expected behavior
The TypeError should not happen and if something's wrong with a Twig-Template or one of the Twig-functions, we should print a clear exception message on the reason why..

@tvdijen tvdijen added this to the 1.19 milestone Nov 7, 2019
@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented Nov 7, 2019

I've figured this out.. It's mostly because the unit test that fails doesn't set any variables, which isn't an issue for variables used in a regular manner.
null | trans works fine

However, because the unit test doesn't set any vars, the failing line in the Twig-template becomes:
null | trans(null) and fails
null | default('') | trans(null) also works fine

The fact remains that if someone fails to set a crucial variable, they end up with a horrible headache that's hard to debug.

@tvdijen tvdijen removed this from the 1.19 milestone Nov 9, 2019
@tvdijen tvdijen added low and removed medium labels Nov 9, 2019
tvdijen added a commit that referenced this issue Nov 9, 2019
@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented Nov 9, 2019

I was able to make it work the same with strict_types=1 as it was without:
751dae6

Now null | trans(null) will return an empty string

@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented Nov 9, 2019

Perhaps a better solution is to add the 'strict_variables'-option to the Twig Environment, which will raise a \Twig\Error\RuntimeError if a variable is not set.

@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented Nov 9, 2019

We need to decide whether to:

  1. take the easy way out and use the 'fix' commit mentioned above, or
  2. work with strict variables in Twig and be prepared to go over all Twig-templates again to fix them (most of them break with strict mode enabled)

I think (2) is the way to go as a 1.19 milestone.. If we manage to fix all templates we can truly say our templating system has properly matured and is ready to be the default in 2.0.

@tvdijen tvdijen added this to the 1.19 milestone Nov 9, 2019
@jaimeperez
Copy link
Member

@jaimeperez jaimeperez commented Nov 13, 2019

I agree with your final argument. If we can make it easier to develop templates (in this case, easier to debug them), and we can uncover mistakes in the process, then that's a good idea 👍

tvdijen added a commit that referenced this issue Nov 16, 2019
tvdijen added a commit that referenced this issue Nov 16, 2019
tvdijen added a commit that referenced this issue Nov 20, 2019
tvdijen added a commit that referenced this issue Dec 24, 2019
tvdijen added a commit that referenced this issue Dec 31, 2019
* Use strict variables in Twig, except for our TemplateTest; throw exception on missing variables

* Fix issues due to Twig strict_variables=true

* Fix for #1229

* Fix metadata-template
@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented Dec 31, 2019

Closed in 09f4ee1

@tvdijen tvdijen closed this Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants