Problem/Motivation

The Twig sandbox policy whitelists specific classes, this makes things less flexible.

Proposed resolution

Whitelist based on instanceof, not a specific class.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

lauriii created an issue. See original summary.

Cottser’s picture

Title: Merging attributes objects is difficult » Merging Attribute objects is difficult
Issue tags: +Twig, +TX (Themer Experience), +DX (Developer Experience), +Needs tests

I like #1 because it would be available from PHP too and there are probably use cases there. Not sure this is a bug but leaving that for now. Thanks @lauriii :)

Pseudo code:

$attributes = $attributes->merge($other_attributes);

{{ attributes.merge(other_attributes).addClass('kittens') }}

I'd think classes or other attributes we store as an array would add on (merge classes array) and string attributes would overwrite (ID from other_attributes would overwrite ID present in attributes for these examples).

And you could also do this I think if you didn't want the ID from other_attributes.

{{ attributes.merge(other_attributes.removeAttribute('id')).addClass('kittens') }}

joelpittet’s picture

Version: 8.0.x-dev » 8.1.x-dev
Category: Bug report » Feature request

I think we can do this, though it's not a bug. This is a feature request and an API addition.

+1 to merge method on the object and maybe on AttributeValueBase

aspilicious’s picture

Well I have troubles in DS. I tried to subclass Attributes to be able to merge now.

BUT than I get

Twig_Sandbox_SecurityError: Calling "mergeAttributes" method on a "Drupal\ds\DsAttribute" object is not allowed in "modules/ds/templates/ds-2col-fluid.html.twig" at line 28. in Drupal\Core\Template\TwigSandboxPolicy->checkMethodAllowed() (line 101 of core/lib/Drupal/Core/Template/TwigSandboxPolicy.php).

In TwigSandboxPolicy you need to overwrite "$whitelisted_classes" but you only can overwrite that in settings.php according to the documentation...

I guess that is for security reasons, so that a crappy module can't introduce security issues.
But it makes it impossible to fix my problem in twig.

The only way to fix this is merging everything in a preprocess function?

aspilicious’s picture

Status: Active » Needs review
FileSize
2.38 KB

Uploading my patch again

aspilicious’s picture

But I can also create a twig function for now in DS.... (option 2)

chx’s picture

Why array_merge_recursive? Do we expect the value of an attribute to be a list of arrays?

joelpittet’s picture

@chx class is always an array ['class' =>['button']]

joelpittet’s picture

Oh nm, it's already looping through the attribute values, that's not needed.

lauriii’s picture

Issue tags: -Needs tests

Some nits:

  1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -240,6 +240,29 @@ public function removeAttribute() {
    +   * @param Attributes[]
    

    S/Attributes/Attribute

  2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -240,6 +240,29 @@ public function removeAttribute() {
    +   *   An array of Attribute objects
    

    Missing dot

  3. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
    @@ -145,6 +145,38 @@ public function testRemoveAttribute() {
    +
    +
    

    Extra line change

joelpittet’s picture

Status: Needs review » Needs work

@aspilicious thanks that looks like a great start. A couple of things:

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -240,6 +240,29 @@ public function removeAttribute() {
+   * @param Attributes[]
+   *   An array of Attribute objects
...
+        $merged_array = array_merge_recursive($merged_array, $arg->toArray());

Maybe it could accept arrays too that way we could pass in arrays in twig? What do you think?

And should we add maybe a test for twig integration as I'm sure this still runs into the Twig sandbox policy issue as it stands.

joelpittet’s picture

Priority: Normal » Major
Issue tags: +Contributed project blocker

Because it's affecting contrib. I'm bumping and tagging.

lauriii’s picture

In Twig filters it makes a little bit sense to make them support everything because in the template you might not know exactly what type of data you have. Adding support for arrays encourages people to use arrays instead of objects what I don't believe we want to do. I think we should create new issue for that and then there we could discuss that through and possibly create new method for that.

lauriii’s picture

Status: Needs work » Needs review
FileSize
2.77 KB
2.36 KB

I don't know what do you think but I would be happy to see it as simple as this.

I also hope we find a way to add this to 8.0.x since this seems pretty common use case (at least I've been struggling with this).

aspilicious’s picture

fine by me...

aspilicious’s picture

You don't need to whitelist merge, the Attributes class is already whitelisted.

joelpittet’s picture

Oh I should have read that a bit closer, I thought it was method whitelisting not class whitelisting.

joelpittet’s picture

Oh we invented the 'class' check in the TwigSandboxPolicy it's not part of the interface from Twig. We should really be checking of instanceof. That may be an easier quick fix to this problem to allow the Attribute extending. And we can do that in 8.0.x then this merge function in 8.1.x. (quicker to production)

Thoughts @aspilicious?

aspilicious’s picture

Yes sure, should be a separate issue if a core committer can accept this as a valid 8.0.x patch...

But it shouldn't be more than changing

    if (isset($this->whitelisted_classes[get_class($obj)])) {
      return TRUE;
    }

Into something like this

    foreach ($this->whitelisted_classes as $class) {
      if ($obj instanceof $class) {
        return TRUE;
      }
    }
aspilicious’s picture

So maybe this doesn't need a new issue...
Here is a patch that allows me to subclass.

joelpittet’s picture

Version: 8.1.x-dev » 8.0.x-dev
Category: Feature request » Bug report
Issue tags: +Needs framework manager review

Adding a needs tag for the framework manager to approve this approach to get into 8.0.x. We can move the other patches to a follow-up issue. @lauriii could you do that please?

joelpittet’s picture

Title: Merging Attribute objects is difficult » Attribute objects are difficult to merge
joelpittet’s picture

@lauriii actually @aspilicious made one so let's just move your change over there #2623960: Add a merge() method on Attribute objects

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

This looks good. Marking RTBC and waiting for framework managers review :)

Cottser’s picture

Title: Attribute objects are difficult to merge » Whitelist instances instead of specific classes in Twig sandbox policy
Issue summary: View changes
Issue tags: +Needs tests
lauriii’s picture

lauriii’s picture

Oops. Posted test only twice

penyaskito’s picture

Status: Needs review » Needs work

I walked through the patch with lauriii and explained what we are doing here. No nitpicks in code review.
Fix looks OK and test is testing correctly what it should, so I was going to RTBC it.
Then I found that an issue summary update is needed, so needs work for that.

The last submitted patch, 26: whitelist_instances-2623708-26-test-only.patch, failed testing.

The last submitted patch, 26: whitelist_instances-2623708-26-test-only.patch, failed testing.

aspilicious’s picture

Status: Needs work » Reviewed & tested by the community

back to rtbc now summary is correct.

The last submitted patch, 26: whitelist_instances-2623708-26-test-only.patch, failed testing.

The last submitted patch, 26: whitelist_instances-2623708-26-test-only.patch, failed testing.

mikeker’s picture

Sorry, late to the party because of the long weekend in the US...

FYI: The instanceof version of this code was removed in #2513266-106: Twig templates can call delete() on entities and other objects among some other performance tweaks, but was never profiled separately. I really can't imagine that this change would have much of an affect, but it might be worthwhile to profile this, just in case. (However, I'm not willing to set an RTBC'ed issue to NW for just that...)

Regardless, I like #20 much better than #14 -- can't imagine what would happen if an Entity::merge() method were added... :)

aspilicious’s picture

In core there is only 1 class in the list, so the foreach isn't a problem (for core).
If your project needs 20 more classes this potentially could become a problem.

catch’s picture

Discussed with @alexpott.

We both think this is fine for 8.0.x.

I tried to think of theoretical breakage this could introduce, and wasn't able to in the end.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 27fe58a and pushed to 8.0.x. Thanks!

  • alexpott committed c91f5a2 on 8.1.x
    Issue #2623708 by lauriii, aspilicious, joelpittet: Whitelist instances...

  • alexpott committed 27fe58a on
    Issue #2623708 by lauriii, aspilicious, joelpittet: Whitelist instances...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.