Twig icon indicating copy to clipboard operation
Twig copied to clipboard

Add strict_properties, array_methods options

Open danog opened this issue 2 years ago • 7 comments

  • 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.

danog avatar Nov 22 '23 12:11 danog

Hi @stof,

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.

@ 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.

strict_properties is set to false by default, thus this does not affect the default behavior for anyone.

danog avatar Nov 22 '23 14:11 danog

@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 avatar Nov 22 '23 14:11 stof

@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 strict_properties should not be used in conjunction with Symfony libraries.

danog avatar Nov 23 '23 12:11 danog

@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 strict_properties or not compatible with it is even worse. It is not limited to the Symfony ecosystem.

Introducing this option in Twig means introducing the splitting of the ecosystem, which has a huge cost for the Twig community.

stof avatar Nov 23 '23 13:11 stof

@stof The same argument of an ecosystem split can be made for strict_variables, autoescape with html, js values, yet these options exist and can be enabled or disabled at will; especially the html/js autoescape strategies, which were added at a later time in Twig 1.8, not directly at release, just like the proposed strict_properties.

danog avatar Nov 23 '23 14:11 danog

Could I also get a second opinion from @fabpot as well? I strongly feel that the strict_properties option is an overall useful addition to Twig :)

danog avatar Nov 23 '23 14:11 danog

@danog For strict_variables, the best practice is anyway to deal properly with the structure of your variables (making you compatible with strict_variables being turned on). Writing code compatible with strict_variables won't be non-idiomatic Twig.

stof avatar Nov 23 '23 14:11 stof