Add strict_properties, array_methods options #3913
Conversation
There was a problem hiding this comment.
I'm not a fan of this strict_properties option.
The only way for a package to use Twig while being compatible with any project would then be to never rely on the implicit getter behavior, which would be a major breakage for the ecosystem (for instance, many things in Symfony are implemented as getters, not as public properties, as this allows computing things on-demand).
I don't think it is a good idea to force all shared packages to write non-idiomatic Twig code.
| return $object[$arrayItem]; | ||
| if ($type === 'method') { | ||
| if (is_callable($object[$arrayItem])) { | ||
| return $object[$arrayItem](...$arguments); |
There was a problem hiding this comment.
This is not properly integrated with the sandbox system.
| return false; | ||
| } | ||
|
|
||
| if ($ignoreStrictCheck || !$env->isStrictVariables()) { |
There was a problem hiding this comment.
Why a check on strict variables here ?
There was a problem hiding this comment.
This avoids falling through to the method call logic below, without throwing an exception (for consistency with the chosen strict_variables setting).
There was a problem hiding this comment.
The strict_variables setting has a well-defined semantic: deciding whether undefined values are returned as null or throwing an exception.
There was a problem hiding this comment.
This logic aims precisely at maintaining this behavior, regardless of the value of strict_properties.
|
Hi @stof,
@ work we have a very large amount of twig templates, but we don't actually use any libraries from the symfony ecosystem (apart from twig), thus the fallback-to-methodcall behavior for properties was causing us issues, without any valid reasons to keep it enabled.
|
|
@danog the issue is that a shared package cannot assume that the current config is the default config. It must work in all modes. Otherwise, we create a split ecosystem. |
|
@stof I now understand your point, however twig is not used exclusively with Symfony libraries: it is a standalone templating engine, that can be used even without symfony libraries; I see no real reason to offer an option to disable a behavior that, while idiomatic for twig templates in the context of Symfony, is not mandatory in contexts outside of Symfony (and in our specific case the default behavior is actually harmful, since it causes breakage and potentially unwanted behavior for thousands of templates). A (subjective) argument can even be made that the default behavior brings actually more harm than good, as it introduces unexpected side effects for what should be just a simple property fetch, and this opinion is shared by my peers and CTO @ work. Regardless, this PR does not aim to remove this behavior altogether, but rather provide an option to increase strictness, which frankly can only do good :) I've added some additional docs to clarify that |
|
@danog being a standalone library makes its ecosystem event larger than the Symfony ecosystem, which only means that splitting the ecosystem between packages compatible with Introducing this option in Twig means introducing the splitting of the ecosystem, which has a huge cost for the Twig community. |
|
@stof The same argument of an ecosystem split can be made for |
|
Could I also get a second opinion from @fabpot as well? I strongly feel that the |
|
@danog For |
strict_properties: if true, treats property accesses in templates only as property accesses, without ever trying to invoke methods with the same name if the property does not exist (defaults to false).array_methods: if true, treats method calls on callable array elements as if they were object method calls.Both rules came in extremely handy @ nicelocal, as the first one increases strictness, also avoiding issues in our automated migration from smarty to twig, while the second one allows the usage of some specific abstractions.