Updated: Comment #0

Problem/Motivation

There are scenarios where you need to be able to merge #attached arrays (i.e. merge different sets of assets).

This is a blocker for a critical D8 cacheability issue: #2090783-25: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user, hence this issue is critical as well.

Proposed resolution

Provide a drupal_merge_attachments() function (API addition).

Note that the exact same logic already existed in drupal_render_collect_attached(), I'm just pulling it out of there to prevent duplication.

Uses in core of drupal_merge_attachments() (and hence in the patch):

  1. drupal_render_collect_attached()
  2. drupal_pre_render_links() (which is how it was discovered that this API function is missing, and this is where it blocks #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user.

Remaining tasks

None.

User interface changes

None.

API changes

New API function: drupal_merge_settings()

[#2090783

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
10.96 KB

And patch. With DrupalUnitTestBase tests. Less than ideal, but until we have a more OOPified asset library system, this is as good as it gets.

catch’s picture

+++ b/core/includes/common.inc
@@ -2462,6 +2462,28 @@ function drupal_merge_js_settings($settings_items) {
+function drupal_merge_attachments($a, $b) {

We should be able to type hint $a and $b. Would be nice to have more descriptive variable names but they can't both be $attached...

amateescu’s picture

FileSize
941 bytes

I tried really hard to see what's the difference between this new drupal_merge_attachments() function and NestedArray::mergeDeep().. but I couldn't. Wouldn't this simple patch solve your problem?

Edit: Note that I'm leaving the code intact in drupal_render_collect_attached() to save some totally unnecessary function calls, while drupal_pre_render_links() doesn't seem to be so important.

Wim Leers’s picture

#2: d'oh, stupid, fixed.

#3:

  1. We want something like drupal_merge_attachments() to indicate that that is the single, correct, accepted method of merging attachment arrays. Just like we have drupal_merge_js_settings() for merging JS settings.
  2. Next, NestedArray::mergeDeep() yields different results than calling array_merge() on each of the three keys. See the docs of NestedArray::mergeDeep(), grep for "array_merge".
  3. Finally, since drupal_render_collect_attached() was already calling array_merge() on each of the three keys, that is de facto already the accepted algorithm on how to merge attachment arrays, and hence I'm just following that. I'd rather not venture changing that right now.
amateescu’s picture

  1. We want something like drupal_merge_attachments() to indicate that that is the single, correct, accepted method of merging attachment arrays. Just like we have drupal_merge_js_settings() for merging JS settings.

    Take a look at what drupal_merge_js_settings() is doing ;)

  2. Next, NestedArray::mergeDeep() yields different results than calling array_merge() on each of the three keys. See the docs of NestedArray::mergeDeep(), grep for "array_merge".

    I tested locally with most of the arrays from your test and I had exactly the same results. Can you elaborate on the difference?

  3. Finally, since drupal_render_collect_attached() was already calling array_merge() on each of the three keys, that is de facto already the accepted algorithm on how to merge attachment arrays, and hence I'm just following that. I'd rather not venture changing that right now.

  4. I already talked about this in #3, we don't need to change drupal_render_collect_attached() in any way, it can continue doing array_merge() since that makes sense for it, performance-wise. In drupal_pre_render_links() we can do whatever we want in order to fix a bug, I assume?

Wim Leers’s picture

Take a look at what drupal_merge_js_settings() is doing ;)

Yes, my point exactly! :) Instead of having Drupal code call NestedArray::mergeDeepArray($settings_items, TRUE) directly all over core, we essentially curried that into a function that is much more discoverable and documentable. That's precisely why I strongly disagree with the patch in #3: that'd mean people would have to know the exact incantation of NestedArray::mergeDeep(), which is not documentable nor discoverable.

I tested locally with most of the arrays from your test and I had exactly the same results. Can you elaborate on the difference?

From the code docblock:

   * This function is similar to PHP's array_merge_recursive() function, but it
   * handles non-array values differently. When merging values that are not both
   * arrays, the latter value replaces the former rather than merging with it.
   *
   * Example:
   * @code
   * $link_options_1 = array('fragment' => 'x', 'attributes' => array('title' => t('X'), 'class' => array('a', 'b')));
   * $link_options_2 = array('fragment' => 'y', 'attributes' => array('title' => t('Y'), 'class' => array('c', 'd')));
   *
   * // This results in array('fragment' => array('x', 'y'), 'attributes' => array('title' => array(t('X'), t('Y')), 'class' => array('a', 'b', 'c', 'd'))).
   * $incorrect = array_merge_recursive($link_options_1, $link_options_2);
   *
   * // This results in array('fragment' => 'y', 'attributes' => array('title' => t('Y'), 'class' => array('a', 'b', 'c', 'd'))).
   * $correct = NestedArray::mergeDeep($link_options_1, $link_options_2);
   * @endcode

My bad, it's talking about array_merge_recursive() and not array_merge(), so it looks like it might indeed yield exactly the same results.

amateescu’s picture

Yes, my point exactly! :) Instead of having Drupal code call NestedArray::mergeDeepArray($settings_items, TRUE) directly all over core, we essentially curried that into a function that is much more discoverable and documentable. That's precisely why I strongly disagree with the patch in #3: that'd mean people would have to know the exact incantation of NestedArray::mergeDeep(), which is not documentable nor discoverable.

This whole argument could be made for adding that drupal_merge_js_settings() helper, but it doesn't apply to our #attached arrays since all we want to do with them is what NestedArray::mergeDeep() provides by 'default'.

Let's not try to fix problems we don't have. :)

Wim Leers’s picture

I disagree. It *is* a problem. Even if in this case it would amount to just "aliasing" NestedArray::mergeDeep(), I think we should do that. We must remove all doubt that this is the single correct way to merge attachment arrays.

Wim Leers’s picture

(double post)

d.o, you're drunk. I definitely pressed the submit button only once.

catch’s picture

If we eventually put the render system (ignoring render arrays) into a service, then I can see that having a method that just calls back to NestedArray::mergeDeep(), so I think it's fine to have a wrapper for now.

tstoeckler’s picture

I think the naming could be improved here. From reading the actual function, I have no idea why the DocBlock and the function name have anything to with 'attached'. It's just a specific way to merge two arrays. Since this is a utility function anyway, I think it would be cooler if the function took the two elements as arguments directly and then instead of foreach ($a ...), do foreach ($a['#attached'] ...). In other words callers could do drupal_merge_attached($element, $child); directly.

tstoeckler’s picture

Oh, yeah and even though 'attached' is not actually a noun, I would prefer drupal_merge_attached() as a function name, as we already have drupal_process_attached(), not drupal_process_attachments(). So while I think in theory attachments make more sense (as that's what they are), I personally think consistency wins here.

Wim Leers’s picture

#11: I disagree with that, because it prevents e.g. the use case in drupal_render_collect_attached(), where all the attachments are being collected in an $attached variable.

#12: I considered both, and chose "attachments". "attached" works for me too. catch, what do you think?

catch’s picture

Title: Add drupal_merge_attachments() function, to merge #attached assets » Add drupal_merge_attached() function, to merge #attached assets

drupal_merge_attached() works for me. Attached isn't a noun but we're referring to 'attached stuff that ends up in ' which would be a long function name.

Not sure on drupal_merge_attached($element, $child); I think it'd be more specific/less verbose in terms of usage, but as Wim says it's then less generically useful - but can't we use NestedArray:mergeDeep() in the general case anyway?

Wim Leers’s picture

#14: we could use NestedArray:mergeDeep() in the general case. I just don't think we should. I prefer to always be explicit, to not confuse those reading the code "hey why is he doing X in one case and Y in the other?".

Wim Leers’s picture

Alright, rerolled to s/drupal_merge_attachments()/drupal_merge_attached()/.

tstoeckler & amateescu: are you okay with this patch now?

amateescu’s picture

Not really.. I'd prefer drupal_merge_attached() to just be a wrapper around NestedArray:mergeDeep(), like you say in #8 and like @catch hints in #10.

Wim Leers’s picture

Oh, right, of course. Sorry to have missed that. Done.

amateescu’s picture

I would also remove the new tests or move some relevant parts of them to NestedArrayTest::testMergeDeepArray().

Wim Leers’s picture

#19: I disagree strongly there. This is about ensuring that the expected behavior is guaranteed by drupal_merge_attached(), regardless of internals. I hate useless tests as much as the next guy. In this case, however, they're testing expectations specific to merging arrays of #attached assets. So IMHO, these tests are appropriate.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Well, if you feel so strongly about them, I don't want to stand in your way :P

Wim Leers’s picture

LOL :D

(I wish that the test coverage for this was more extensive, even. Merging declarative things… a lot that can go wrong. And since this is at the very center of what Drupal does, I think it only merits that it's tested to ensure it's working as expected!)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks! Fine with the explicit higher-level test.

Wim Leers’s picture

Issue tags: -sprint

Thanks! :)

Wim Leers’s picture

d.o tag monster :/

Wim Leers’s picture

Issue tags: +Performance

Adding "Performance" tag for tracking purposes. This was a blocker for #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user, a performance-related issue.

Status: Fixed » Closed (fixed)

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