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):
drupal_render_collect_attached()
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()
Related Issues
[#2090783
Comment | File | Size | Author |
---|---|---|---|
#18 | drupal_merge_attachments-2113015-18.patch | 10.79 KB | Wim Leers |
#18 | interdiff.txt | 647 bytes | Wim Leers |
#16 | drupal_merge_attachments-2113015-16.patch | 10.92 KB | Wim Leers |
#16 | interdiff.txt | 8 KB | Wim Leers |
#4 | drupal_merge_attachments-2113015-2.patch | 10.97 KB | Wim Leers |
Comments
Comment #1
Wim LeersAnd patch. With
DrupalUnitTestBase
tests. Less than ideal, but until we have a more OOPified asset library system, this is as good as it gets.Comment #2
catchWe 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...
Comment #3
amateescu CreditAttribution: amateescu commentedI 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, whiledrupal_pre_render_links()
doesn't seem to be so important.Comment #4
Wim Leers#2: d'oh, stupid, fixed.
#3:
drupal_merge_attachments()
to indicate that that is the single, correct, accepted method of merging attachment arrays. Just like we havedrupal_merge_js_settings()
for merging JS settings.NestedArray::mergeDeep()
yields different results than callingarray_merge()
on each of the three keys. See the docs ofNestedArray::mergeDeep()
, grep for "array_merge".drupal_render_collect_attached()
was already callingarray_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.Comment #5
amateescu CreditAttribution: amateescu commentedTake a look at what
drupal_merge_js_settings()
is doing ;)I tested locally with most of the arrays from your test and I had exactly the same results. Can you elaborate on the difference?
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. Indrupal_pre_render_links()
we can do whatever we want in order to fix a bug, I assume?Comment #6
Wim LeersYes, 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 ofNestedArray::mergeDeep()
, which is not documentable nor discoverable.From the code docblock:
My bad, it's talking about
array_merge_recursive()
and notarray_merge()
, so it looks like it might indeed yield exactly the same results.Comment #7
amateescu CreditAttribution: amateescu commentedThis 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. :)
Comment #8
Wim LeersI 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.Comment #9
Wim Leers(double post)
d.o, you're drunk. I definitely pressed the submit button only once.
Comment #10
catchIf 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.
Comment #11
tstoecklerI 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 ...)
, doforeach ($a['#attached'] ...)
. In other words callers could dodrupal_merge_attached($element, $child);
directly.Comment #12
tstoecklerOh, 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.
Comment #13
Wim Leers#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?
Comment #14
catchdrupal_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?Comment #15
Wim Leers#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?".Comment #16
Wim LeersAlright, rerolled to
s/drupal_merge_attachments()/drupal_merge_attached()/
.tstoeckler & amateescu: are you okay with this patch now?
Comment #17
amateescu CreditAttribution: amateescu commentedNot really.. I'd prefer
drupal_merge_attached()
to just be a wrapper aroundNestedArray:mergeDeep()
, like you say in #8 and like @catch hints in #10.Comment #18
Wim LeersOh, right, of course. Sorry to have missed that. Done.
Comment #19
amateescu CreditAttribution: amateescu commentedI would also remove the new tests or move some relevant parts of them to NestedArrayTest::testMergeDeepArray().
Comment #20
Wim Leers#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.Comment #21
amateescu CreditAttribution: amateescu commentedWell, if you feel so strongly about them, I don't want to stand in your way :P
Comment #22
Wim LeersLOL :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!)
Comment #23
catchCommitted/pushed to 8.x, thanks! Fine with the explicit higher-level test.
Comment #24
Wim LeersThanks! :)
Comment #25
Wim Leersd.o tag monster :/
Comment #26
Wim LeersAdding "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.