Whitelist instances instead of specific classes in Twig sandbox policy

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') }}

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

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?

Uploading my patch again

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

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

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

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

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

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

fine by me...

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

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

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?

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;
      }
    }

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

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?

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

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.

back to rtbc now summary is correct.

Hide test-only patches so they are not tested all the time.

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... :)

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.

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.

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