Fix twig environment

[mjauvin]

Fix #578

Credit to @RomainMazB

[LukeTowers]

Well it's somewhat hacky, but I don't have any major concerns with it. @bennothommo any thoughts?

[bennothommo]

@mjauvin @RomainMazB is controller the only one needed? Would we need to support the other variables too, as per the Markup docs like this.page, this.layout, etc.?

[RomainMazB]

@bennothommo, all the needed variables is mentioned here in the code:

winter/modules/cms/classes/Controller.php

Lines 309 to 317 in b90b4fe

	$this->vars['this'] = [
	'page' => $this->page,
	'layout' => $this->layout,
	'theme' => $this->theme,
	'param' => $this->router->getParameters(),
	'controller' => $this,
	'environment' => App::environment(),
	'session' => App::make('session'),
	];

Kind of surprised that add variables as global is an acceptable fix, sounds to me like a regression compared to the work made by @LukeTowers in PR #455 but I'm no the guy in charge here ;)

[mjauvin]

@bennothommo as far as I could see, controller is the only one needed:

# rgrep "'this'\]\['" modules

modules/cms/twig/Extension.php:        return $context['this']['controller']->renderPage();
modules/cms/twig/Extension.php:        return $context['this']['controller']->renderPartial($name, $parameters, $throwException);
modules/cms/twig/Extension.php:        return $context['this']['controller']->renderContent($name, $parameters);
modules/cms/twig/Extension.php:        return $context['this']['controller']->renderComponent($name, $parameters);
modules/cms/twig/Extension.php:        return $context['this']['controller']->makeAssets($type);
modules/cms/twig/Extension.php:     * @param array $context The Twig context for the call (relies on $context['this']['controller'] to exist)
modules/cms/twig/Extension.php:        return $context['this']['controller']->pageUrl($name, $parameters, $routePersistence);
modules/cms/twig/Extension.php:     * @param array $context The Twig context for the call (relies on $context['this']['controller'] to exist)
modules/cms/twig/Extension.php:        return $context['this']['controller']->themeUrl($url);

[RomainMazB]

I discussed with @mjauvin on Discord about a related issue he met with the plugin: vojtasvoboda /
DynamicPDF.
He discovered that this issue is not only under the macro scope as the issue appeared within this plugin without the use of macro.

The plugin uses the registered twig.environment Facade from the system module to parse templates into an html page and then render it as a PDF file.

The plugin parses templates without being in any controller context, which results to the same "undefined this issue".

I may investigate furthermore on how to solve these issues without adding global variables, but as of today, re-introducing a global scope may be the best affordable solution.

For the needed variables: to me, even if I only use the controller variable through the |page filter, I guess that all the variable I mentioned earlier may be needed by someone else.
An example would be the access to a theme color config from a macro, or an url param using this.param.

[Edit] To confirm I just tested to use the theme in a macro, I confirm that it doesn't work with this basic test:

#theme.yaml
form:
  fields:
    main_color:
      label: Main color
      type: colorpicker
      default: "#FF0000"

{# Inside a macro #}
<span style="color: {{ this.theme.main_color }}">Some dummy text</span>

This doesn't work without adding the theme as a global variable.

[LukeTowers]

Hmm, the global variables solution might not be enough then. With regards to the DynamicPDF plugin that's a separate issue, if someone wants to use the CMS twig environment to render a twig template then they need to refer to the correct namespace for it; we're not going to make the CMS features available in the system twig environment.

@RomainMazB What was the other solution that you proposed regarding overriding the processing of Macros in order to inject the correct context information into it? That sounds like that would be a better fix, especially when you consider a template itself might inject variables into the context that we might not have access to if we just go the global variable route.

[mjauvin]

This is still a breaking change @LukeTowers. If many plugins start breaking for 1.2, this is not going to be a fun ride.

…

-- Marc

On Mon., Jul. 4, 2022, 17:10 Luke Towers, ***@***.***> wrote: Hmm, the global variables solution might not be enough then. With regards to the DynamicPDF plugin that's a separate issue, if someone wants to use the CMS twig environment to render a twig template then they need to refer to the correct namespace for it; we're not going to make the CMS features available in the system twig environment. @RomainMazB <https://github.com/RomainMazB> What was the other solution that you proposed regarding overriding the processing of Macros in order to inject the correct context information into it? That sounds like that would be a better fix, especially when you consider a template itself might inject variables into the context that we might not have access to if we just go the global variable route. — Reply to this email directly, view it on GitHub <#593 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPLTPQKRC7Z4P6JWAB4UKTVSNHMTANCNFSM52RNGFEQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[RomainMazB]

@LukeTowers, I just finished to work on my day project, I'll try to make a draft PR to see if what I suggested would work and if it could be more appropriate.

[LukeTowers]

@mjauvin I don't see how it is, the System twig environment was never meant to have CMS features in it and before the changes I made in my PR to separate everything there were all sorts of issues that would crop up if you were to initialize it before doing things like sending emails for example.

Could you clarify what the BC break is?

[bennothommo]

@mjauvin @RomainMazB would either of you be interested in making a unit test for both scenarios (the macro issue, and the issue from the Dynamic PDF plugin)? It would be worth it at least to ensure that any given solution we implement covers both scenarios.

Also, as for backwards compatibility - while it is one of our main goals, our major updates like with 1.2 does give us the leeway to make some backwards-incompatible changes if they are strongly justified. I am in agreement with Luke's rationale for the Twig changes in the first place, and reverting it back to fix these new issues is just going to bring back the issues that Luke's changes fixed.

[RomainMazB]

I have a solution I made on my local machine, at least for the macro issue.

I was searching for any more convenient way before post it as a PR.

I'll post my code as a draft PR as soon as I am at home.

I didn't write tests for it atm.

[bennothommo]

@RomainMazB @mjauvin I have confirmed that you will need the other variables as mentioned here defined in the globals if you want to support this.theme or this.page within a macro.

For example, using {{ this.page.fileName }} outside a macro will work fine, and uses the vars from the block of code that @RomainMazB mentioned here, but using that inside a macro will come up blank.

[LukeTowers]

@bennothommo I'm hoping that @RomainMazB's suggestion of overriding the macro handling to include the correct context will pan out and we don't need to define a bunch of globals, especially since some of those don't exist when we would typically be defining the globals.

[bennothommo]

I just tried to do that very thing. If Maz has worked that one out, he's a genius. 😜

[RomainMazB]

I just tried to do that very thing. If Maz has worked that one out, he's a genius. 😜

Probably not a genius yet! I'm not fully convinced of the actual draft work: #598
It can be a starting point, at least it works.

[LukeTowers]

Replaced by 5b8d189