Converted addClosure to a public method
#1
Conversation
|
So this would definitely be an easy fix... but I'm wondering if there's a way it can work automatically with a sandboxed environment? Perhaps a different event I could listen to, other than |
|
Maybe? A lot of that closure script was pretty mysterious to me. The event probably isn't being triggered because we're running |
|
So the other alternative I can think of is that you also throw the same Event that Craft does before you render the object template... but that would only work if you instantiated your own That actually might not be a bad route to explore in general, only because you could create this custom view once when your plugin inits, and then you wouldn't have the setup/teardown overhead every time you're rendering via your sandbox (though I don't know if you're actually doing it multiple times). A decent bit of the code in the links that Josh sent along: https://discord.com/channels/@me/545035542098870273/1241514968176791634 ...seems to replicate what is in the Craft I'm thinking it might simplify things for you in general, but just a thought. |
|
I certainly appreciate your creative problem solving on this, although I worry that you may be overcomplicating the solution. This PR is a perfect fix, we simply need to make the I can confidently say that me adding a new event, testing that event, documenting that event, and supporting that event into the future are not a simpler solution than just accepting this ready-to-go PR. I've tested this PR thoroughly on my local machine, it's doing exactly what we need. Is there a reason you are reluctant to accept it as written? Or are you simply searching for a hypothetical "better" solution? |
|
@lindseydiloreto yeah sure I'll merge the PR in, but I think you misunderstand me. If you do what I suggest, you wouldn't need to do any of what you describe. Existing things like Closure would "just work" because you'll be throwing the same event that it already listens for. You aren't creating a new event, so you don't need to document it. Also my suggestion mostly came because it appears that doing it the way I suggest would remove a lot of duplicated code that's already in the current implementation. Might be simpler in the end! |
|
Ok great, thank you. It is possible I don't fully understand what you were going for. This PR will do the trick perfectly for now though, and if a better solution emerges down the road, I'm happy to adjust Notifier to compensate. Thanks @khalwat, much appreciated! 🍺 |
|
Okay, curiosity got the best of me... and I can confirm that it works. Much simpler, too compared to: https://github.com/verbb/verbb-base/blob/63785695d99eadcbceafb0813cf9372c142377b3/src/services/Templates.php <?php
namespace modules\sitemodule\web;
use craft\events\CreateTwigEvent;
use craft\web\View;
use modules\sitemodule\twig\SecurityPolicy;
use Twig\Extension\SandboxExtension;
use Twig\Sandbox\SecurityPolicyInterface;
class SandboxView extends View
{
// The security policy to use for this View
public ?SecurityPolicyInterface $securityPolicy = null;
public function init(): void
{
parent::init();
// Use the passed in SecurityPolicy, or create a default security policy
$this->securityPolicy = $this->securityPolicy ?? new SecurityPolicy(
$this->allowedTags(),
$this->allowedFilters(),
$this->allowedMethods(),
$this->allowedProperties(),
$this->allowedFunctions()
);
// Add the SandboxExtension with our SecurityPolicy after Twig is created
$this->on(View::EVENT_AFTER_CREATE_TWIG, function(CreateTwigEvent $event) {
$twig = $event->twig;
$twig->addExtension(new SandboxExtension($this->securityPolicy, true));
});
}
}Just implement the Then use it like this: $view = new SandboxView();
$result = $view->renderString("{{ dump() }}", []);...and see your exception thrown: |
|
Here's a backwards compatible version of the same code, which doesn't use the <?php
namespace modules\sitemodule\web;
use craft\web\twig\Environment;
use craft\web\View;
use modules\sitemodule\twig\SecurityPolicy;
use Twig\Extension\SandboxExtension;
use Twig\Sandbox\SecurityPolicyInterface;
class SandboxView extends View
{
// The security policy to use for this View
public ?SecurityPolicyInterface $securityPolicy = null;
public function init(): void
{
parent::init();
// Use the passed in SecurityPolicy, or create a default security policy
$this->securityPolicy = $this->securityPolicy ?? new SecurityPolicy(
$this->allowedTags(),
$this->allowedFilters(),
$this->allowedMethods(),
$this->allowedProperties(),
$this->allowedFunctions()
);
}
public function createTwig(): Environment
{
$twig = parent::createTwig();
// Add the SandboxExtension with our SecurityPolicy after Twig is created
$twig->addExtension(new SandboxExtension($this->securityPolicy, true));
return $twig;
}
} |
|
How does this solution work for you exactly? Closure uses the So if I try and run: Craft::dd(Formie::$plugin->getSandboxView()->renderString("
{% set collection = collect(['a', 'b', 'c']) %}
{% set contains = collection.contains((value, key) => value == 'z') %}
{{ contains ? 'test' : 'test1' }}
"));With your provided solution of extending the View class, I'll get an error: Because Closure hasn't been registered. Which brings us back to the original PR of being able to call the registration of the Closure functionality as we need it, not just assuming it's going to be used in Secondly, I'm not a massive fan of the double error's that are thrown when encountering an error. We can indeed add For example, this is an error thrown right now when including This isn't exactly your solutions fault, and is more that the Craft error handler doesn't know about our custom environment. |
|
Oh! I forgot that you merged the PR, so not so much a pressing issue, just an FYI on your solution! |
|
@engram-design lol I actually didn't test it with Closure... but you're right, it won't fix that. But interestingly, nor would it fix it in a non-sandboxed environment, if someone wanted to use Closure in a rendered string template. Which makes me think that I should just register Closure via the The only reason I don't do that now is backwards compatibility, as that event was added in Craft But I think that's a reasonable enough minimum to require for updates to Closure, so I'm going to refactor it to require that as a minimum version of Craft. |
|
As for the error handler, I'll have a look at that end of things too. The reality is if you're rendering a string template, you generally don't want to throw an exception anyway though, because it's part of a large UI. Rather you'd like to swallow the exception, and report it to the user in-context in your UI. I do something similar in SEOmatic where I validate Twig templates, but if you make a mistake in the template it doesn't go full screen and throw a Twig error, it displays that error inline with the field as an error via a validator (which has the try/catch in it): https://github.com/nystudio107/craft-code-editor/blob/develop/src/validators/TwigTemplateValidator.php 
There's even an object template validator: https://github.com/nystudio107/craft-code-editor/blob/develop/src/validators/TwigObjectTemplateValidator.php ...and if you're not using |
|
The only reason I suggest keeping it a fatal error is to keep consistent with Craft rendering, and so that the plugins that implement this sandbox can handle the exception how they like. |
|
@engram-design I've solved that problem... more soon :) |
|
@khalwat Thanks for putting together a common sandbox! I look forward to taking it for a spin. 🍺 Once it's backported to Craft 4, I'll be able to roll it into Notifier. |
|
@lindseydiloreto no problem. The "backport" is the same code, different semver. I'm just waiting until I'm ready to ship 1.0.0 before bothering to do that. Will be shortly. |
|
@lindseydiloreto it's been backwported to Craft 4, and is released -> https://github.com/nystudio107/craft-twig-sandbox/ |
|
Brilliant, thanks @khalwat! Glad to finally have a common sandbox tool to lean on. 🎉 🙏 |
Converts
addClosureto a public method, with a$twigparameter for optionally supplying a custom Twig environment.Once converted to a public method, it will be much easier for 3rd-party packages to support the Closure module...
It will be immediately beneficial to both Formie and Notifier, making it much easier to integrate Closure with both plugins.
Thanks for considering this PR! 🙏