-
-
Notifications
You must be signed in to change notification settings - Fork 514
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
fix: tests failing since Twig 3.8.0 #2895
Conversation
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.
just minor documentation changes. Bloody good job! I was already looking into this as well what caused these failed tests.
Just banged my head for more than one hour on that bug 🤯 I don't really understand why we are testing this. Checking if those cases (calling I feel (already noticed before) we are sometimes testing Twig twice. With the extra burden of supporting a wider range. |
By lowering the amount of WP and PHP versions we test, hopefully the amount of these kind of tests deprecation will decrease as well. |
On some other cases, probably. On that particular one, unless bumping Twig to 3.8 (which we might not want), the problem is that we test things that shouldn't require tests from our side (+ all the other similar tests). People using Twig should be aware of how Twig handles variables. |
Twig recently changed the way exceptions are thrown during template rendering. Depending on the version, different types of execption can be thrown. See: twigphp/Twig@85bf01b fix: failing tests for WP <6.4 Add two new methods in base test case to check WordPress version and mark test Co-authored-by: Erik van der Bas <erik@basedonline.nl>
02d0882
to
d9d7348
Compare
And it's all green again. Merging this to make all other PR pass (or not but not polluted with our own failing tests). |
I agree. I guess the idea behind these tests were to check that our custom magic methods work as intended all the way through. I agree with you that we shouldn’t test for argument count errors, though.
In my experience, this is something that Timber developers sometimes are not aware of. |
Maybe, but is it in our scope? Timber users are in fact also using Twig 🙂 Further more, adding those tests won't bring that behavior knowledge to our end users. |
It isn’t a problem big enough that we should act.
No, I didn’t think these are connected at all. If we should handle the problem in any way, it should be with more extensive documentation, but not with tests. |
Twig recently changed the way exceptions are thrown during template rendering. Depending on the version, different types of execption can be thrown.
See: twigphp/Twig@85bf01b