Remove Deprecated Function drupal_merge_attached and its references
Function definition as in core/includes/common.inc
/**
* Merges two #attached arrays.
*
* The values under the 'drupalSettings' key are merged in a special way, to
* match the behavior of:
*
* @code
* jQuery.extend(true, {}, $settings_items[0], $settings_items[1], ...)
* @endcode
*
* This means integer indices are preserved just like string indices are,
* rather than re-indexed as is common in PHP array merging.
*
* Example:
* @code
* function module1_page_attachments(&$page) {
* $page['a']['#attached']['drupalSettings']['foo'] = ['a', 'b', 'c'];
* }
* function module2_page_attachments(&$page) {
* $page['#attached']['drupalSettings']['foo'] = ['d'];
* }
* // When the page is rendered after the above code, and the browser runs the
* // resulting <SCRIPT> tags, the value of drupalSettings.foo is
* // ['d', 'b', 'c'], not ['a', 'b', 'c', 'd'].
* @endcode
*
* By following jQuery.extend() merge logic rather than common PHP array merge
* logic, the following are ensured:
* - Attaching JavaScript settings is idempotent: attaching the same settings
* twice does not change the output sent to the browser.
* - If pieces of the page are rendered in separate PHP requests and the
* returned settings are merged by JavaScript, the resulting settings are the
* same as if rendered in one PHP request and merged by PHP.
*
* @param array $a
* An #attached array.
* @param array $b
* Another #attached array.
*
* @return array
* The merged #attached array.
*
* @deprecated To be removed in Drupal 8.0.x. Use
* \Drupal\Core\Render\Renderer::mergeAttachments() instead.
*/
function drupal_merge_attached(array $a, array $b) {
return Renderer::mergeAttachments($a, $b);
}| Issue category | Task because it is removing deprecated code. |
|---|---|
| Issue priority | Normal because nothing is really broken. |
| Prioritized changes | Prioritized because it finalizes deprecating code marked for removal before 8.0.0 release. |
| Comment | File | Size | Author |
|---|---|---|---|
| #29 | 2452475-29-remove-drupal_merge_attached.patch | 3.5 KB | ianthomas_uk |
Comments
Comment #1
arpitr commentedPending:
References in -
1. Core/Render/BubbleableMetadata.php
* @todo Add unit test for this in
* \Drupal\Tests\Core\Render\BubbleableMetadataTest when
* drupal_merge_attached() no longer is a procedural function and remove
* the '@codeCoverageIgnore' annotation.
2. core/modules/system/src/Tests/Common/MergeAttachments.php
Test class MergeAttachmentsTest
Comment #2
arpitr commentedComment #4
lhangea commentedLet's see if this passes the tests
Comment #6
lhangea commentedThere is still this comment in:
/core/lib/Drupal/Core/Render/BubbleableMetadata.php
Comment #10
lhangea commentedComment #12
lhangea commentedAdded unit test for the merge() method
Comment #13
mile23Patch in #12 still applies, but unit tests fail here:
It looks like Cache's static methods depend on \Drupal. Sigh.
Other than the failing unit test, it looks good. Fix that and we're RTBC.
Comment #14
lhangea commentedWell, in the Cache class we have
that depends on the \Drupal
What we can do for this to pass is to make Cache class implement ContainerInjectionInterface and inject the needed services since the Cache class itself is not a service. But for that we need a separate issue and this one should be postponed until the services are properly injected into the Cache class.
The other static methods used in merge() method don't depend on the \Drupal so, it's ok.
Comment #15
mile23Ideally, yah, we'd refactor Cache, but that's clearly out of scope of this issue. In fact, we might leave the @todo about this test, because it's a bit out of scope anyway.
If we want to move forward with the test, though, we'd mock the cache services, place them in the container, and then assert. I'd say this is the way to go. :-)
UnitTestCase::getContainerWithCacheTagsInvalidator()can show the way. It hands you a mocked container, to which you can add your own mocked service objects with expectations.Comment #16
mile23Comment #17
neetu morwani commentedComment #18
neetu morwani commentedComment #19
neetu morwani commentedComment #21
googletorp commentedRerolled patch from #12
Comment #23
mile23#2407195: Move attachment processing to services and per-type response subclasses adds a unit test for BubbleableMetadata::merge(), which leads to a reroll conflict with the patch in #21.
Let's remove the unit tests for BubbleableMetadata from the patch, which are broken and a bit out of scope here anyway.
Comment #24
googletorp commentedComment #28
mile23We want to use
BubbleableMetadata::mergeAttachments()like it says in the deprecation message.Comment #29
ianthomas_ukComment #30
mile23Nice, thanks.
Setting to RTBC. If the tests fail, it'll be NW, but I doubt it.
Still needs a change record and a beta evaluation.
Comment #31
mile23Comment #32
mile23Added change record: https://www.drupal.org/node/2561493
Comment #34
jeroentRandom test failures?
Comment #36
ianthomas_ukLooks like the bot was out of disk space to me
Comment #39
googletorp commentedSeems like something weird happened with test bot. Since tests are passing I'm moving issue back to RTBC.
Comment #40
mile23+1 RTBC. Patch applies and the testbot seems happy, plus there aren't any calls to
drupal_merge_attached()remaining.Comment #41
mile23Comment #42
alexpottCommitted 2ca39a4 and pushed to 8.0.x. Thanks!