Problem/Motivation

Working on some rendering stuff, I noticed that Bartik's color.inc creates a render array like this: $js_attached['#attached']['drupalSettings'] It then feeds it to drupal_process_attached(), which immediately unsets 'drupalSettings' and then proceeds to waste some cycles doing nothing with the array.

Proposed resolution

Figure out what this is trying to actually accomplish and then make it do that.

Remaining tasks

User interface changes

API changes

Data model changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because Bartik a) Isn't attaching JS properly, b) Is about to break when drupal_process_attached() is deprecated.
Issue priority Normal because it's not the end of the world.
Unfrozen changes Unfrozen because it adds tests to reinforce the currently-expected behavior in addition to fixing the bug.
Prioritized changes Prioritized because we might be deprecating drupal_process_attached() out of existence.
Disruption Disruptive in the sense that it makes Bartik's preview work correctly.
CommentFileSizeAuthor
#9 2548991-9.patch2.65 KBwim leers
#5 Screen Shot 2015-09-04 at 17.14.37.png448.23 KBwim leers
#5 Screen Shot 2015-09-04 at 17.14.46.png270.1 KBwim leers
#4 2548991-4.patch1.6 KBwim leers

Comments

Mile23 created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented 
      // Change the logo to be the real one.
      if (!this.logoChanged) {
        $('.color-preview .color-preview-logo img').attr('src', drupalSettings.color.logo);
        this.logoChanged = true;
      }
      // Remove the logo if the setting is toggled off.
      if (drupalSettings.color.logo === null) {
        $('div').remove('.color-preview-logo');
      }

Looks is used

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Needs manual testing
Related issues: +#2552865: Deprecate drupal_process_attached() for 8.0.0 and either throw an Exception or make it work via the RenderContext

Actually needs to check that logo displayed at color-preview

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2548991-4.patch1.6 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Issue tags: -Needs manual testing
StatusFileSize
new Screen Shot 2015-09-04 at 17.14.46.png270.1 KB
new Screen Shot 2015-09-04 at 17.14.37.png448.23 KB

I manually tested this.

I uploaded a custom logo for Bartik. Results of customizing the colors of Bartik below.

With HEAD:

With patch:

Status: Needs review » Needs work

The last submitted patch, 4: 2548991-4.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented

nice trick :)
maybe make this key optional?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Issue tags: +Needs tests

So we do have some test coverage! Let's fix it there then.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Bartik's color.inc uses drupal_process_attached() improperly. » Bartik's color.inc uses drupal_process_attached() improperly
Assigned: wim leers » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2548991-9.patch2.65 KB

There.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/color/src/Tests/ColorTest.php
@@ -177,4 +177,21 @@ function testValidColor() {
+  function testLogoSettingOverride() {
+    $this->drupalLogin($this->bigUser);
+    $edit = array(
+      'default_logo' => FALSE,
+      'logo_path' => 'core/misc/druplicon.png',
+    );
+    $this->drupalPostForm('admin/appearance/settings', $edit, t('Save configuration'));
+
+    // Ensure that the overridden logo is present in Bartik, which is colorable.
+    $this->drupalGet('admin/appearance/settings/bartik');
+    $this->assertIdentical($GLOBALS['base_url'] . '/' . 'core/misc/druplicon.png', $this->getDrupalSettings()['color']['logo']);
+  }

1. Nice to have a test, but this only tests that the setting was saved, not that the image shows up on the page. Since the image preview seems to be handled by JS though, we can't really do that without something like selenium.

2. Also, core/misc/druplicon.png is an image of the druplicon. As it turns out, when default_logo is TRUE, Bartik shows core/themes/bartik/logo.svg, which is.... ALSO an image of the druplicon. :-) Which made manual repro kind of weird.

Not sure whether to RTBC or NW, because we can't really address 1, and 2 isn't a blocker. So RTBC it is.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Thanks for the review!

Nice to have a test, but this only tests that the setting was saved, not that the image shows up on the page. Since the image preview seems to be handled by JS though, we can't really do that without something like selenium.

We test that the drupalSettings that are necessary for the image to show up are present. Which is NOT working in HEAD. And indeed, the only way to ensure that the image is also visually there (i.e. that the JS isn't broken), is to run an actual browser that can run JS. So, yep, this is as good as it gets :)

ALSO an image of the druplicon. :-) Which made manual repro kind of weird.

:P The PNG does not get colored automatically! That's a big, noticeable difference :)

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Title: Bartik's color.inc uses drupal_process_attached() improperly » Remove Bartik's erroneous use of drupal_process_attached(), add tests.
Issue summary: View changes

Added beta eval, updated issue summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 3cd0943 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 3cd0943 on 8.0.x 
    Issue #2548991 by Wim Leers, Mile23: Remove Bartik's erroneous use of...

Status: Fixed » Closed (fixed)

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