Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Since #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1 there is a merge method on Attributes, which was the original focus of this issue.
But from #17 below this issue pivots to discussing potential improvements to the current merge method.
Proposed resolution
Remaining tasks
/
User interface changes
/
API changes
Data model changes
/
Comment | File | Size | Author |
---|---|---|---|
#8 | merging_attribute-2623708-14.patch | 2.77 KB | lauriii |
#2 | 2623960-merge-attributes.patch | 2.42 KB | aspilicious |
Comments
Comment #2
aspilicious CreditAttribution: aspilicious commentedComment #3
star-szrPlease join us over in #2623708: Whitelist instances instead of specific classes in Twig sandbox policy, that would be lovely :)
Comment #4
joelpittetActually re-opening:) We have a proposal to fix the other in 8.0.x and this can be the 8.1 issue.
Comment #5
joelpittetComment #6
star-szrComment #7
lauriiiComment #8
lauriiiPosting latest patch from the other issue
Comment #9
aspilicious CreditAttribution: aspilicious commentedThis is not needed and dangerous :)
Comment #17
sunIn our project, we're trying to merge an attributes object with dynamic, case-specific attributes into a default attributes object, for which we require the merge() method, too.
We have already implemented the method in the corresponding JavaScript adapter for TwigJS some time ago and it has proven to be a big help: https://github.com/netzstrategen/twig-drupal/pull/15
However, we came to realize that it is not always possible to determine whether an Attributes object is defined or empty or populated, especially with regard to the internal classes property. Therefore I would recommend to make the argument nullable:
That said, in the JavaScript port we also accept plain objects (associative arrays) as data to be merged. It would make sense to also support that here; at least I can't see a reason for not doing so.
Comment #18
markhalliwellPotentially related issue.
Comment #20
sunAs Twig is automatically type-casting many values, we not only removed the type-hint, but are additionally checking very explicitly for the type of the passed variable to be merged (see patch below).
Will the core components separated into standalone packages soon?
Comment #22
sunThe merge method changes the Attributes object instead of cloning/forking into a new one, which causes unexpected behaviors and problems in loops in Twig templates, for example, when generating the output for a hierarchical navigation menu. We adjusted the method to instantiate a new class instead of modifying the existing object.
This makes the behavior of Attributes.merge consistent with the Twig |merge filter.
This only happens when data is merged, but not in the error/early-return conditions, which is an inconsistent behavior. However, I think this is a larger problem with the current Attributes objects; the same problem also exists for addClass(), setAttribute(), and other methods inside of loops. A copy() or clone() method would be helpful and perhaps more appropriate, so the user can control when a new object is necessary.
Comment #28
jonathanshaw