The Umami demo profile defines two profile-specific blocks via custom code: "Umami disclaimer" and "Umami Footer promo".

It is unusual for an install profile to define its own blocks like that. It is also unnecessary; Drupal 8 has fieldable custom blocks, which can easily be used here (the two blocks don't have any special behavior, just some text fields and such).

Switching to custom blocks would allow the install profile to follow best practices, and it would also replace lots of custom code with configuration, which is good for the overall stability of the profile. Furthermore, the "Umami Recipes Banner" block already uses fieldable custom blocks, so changing the other two blocks over makes the code more consistent.

In addition to the general improvements noted above, this change would lead to several specific improvements also:

  1. It fixes several bugs with the "Find out more" link, including a security issue (see #2938904: "Find out more" link isn't validated or properly displayed, leading to bugs including cross-site scripting).
  2. It allows the "Quick Edit" feature to be used on the block content.
  3. In general, it allows the blocks to be much more configurable, using Drupal's standard field UI.
CommentFileSizeAuthor
#16 Screen Shot 2018-02-01 at 3.53.34 AM.png514.22 KBnavneet0693
#16 Edit custom block Umami disclaimer I drupal 8.6.png357.78 KBnavneet0693
#16 Edit custom block Umami footer promo I drupal 8_6.png260.63 KBnavneet0693
#14 umami-blocks-2938900-14.patch38.34 KBDavid_Rothstein
#11 umami-blocks-2938900-11.patch38.34 KBDavid_Rothstein
#8 interdiff.txt1014 bytesDavid_Rothstein
#8 umami-blocks-2938900-8.patch37.74 KBDavid_Rothstein
#2 umami-blocks-2938900-2.patch37.77 KBDavid_Rothstein
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein created an issue. See original summary.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Active » Needs review
FileSize
umami-blocks-2938900-2.patch37.77 KB

Here is a patch.

Although the patch is large in terms of file size, it is actually very straightforward - as noted above, most of what it does is just delete custom code and move it over to config files instead.

The visual appearance of the blocks should not change at all (comparing a site installed before this patch to one installed after).

Note: Another possible improvement here would be to make the "3 issue bundle" image associated with the footer promo block into an uploaded image field (similar to how the "Umami Recipes Banner" does it), rather than hardcoded in the theme. Currently, however, the patch does not do this.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Issue summary: View changes
Related issues: +#2938904: "Find out more" link isn't validated or properly displayed, leading to bugs including cross-site scripting

Added link to the security issue that this patch fixes to the issue summary, plus several minor edits.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Issue tags: +Umami beta blocker

I am tentatively marking this as a beta blocker, for two reasons:

  1. It fixes #2938904: "Find out more" link isn't validated or properly displayed, leading to bugs including cross-site scripting (which is definitely a beta blocker) and is almost certainly the best, most robust fix for that issue.
  2. The amount of custom code that it deletes and moves into configuration is probably too large of a change to do after beta; for example, this would cause issues for any site that already had the profile installed.

Also, note that after this patch, the install profile essentially has no custom runtime code remaining, which would really mean it can do whatever it wants to do in the future (functionality-wise) without worrying about breaking anything for anyone.

Status: Needs review » Needs work

The last submitted patch, 2: umami-blocks-2938900-2.patch, failed testing. View results

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented

Those test failures look fixable - I will look into them a little later.

markconroy’s picture

markconroy CreditAttribution: markconroy as a volunteer and at Annertech commented

David,

Thanks for this work, I'll try get some time later on this play with it.

Hopefully some of the backenders on the project can do the same.

Looks like it's some great work.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Needs work » Needs review
FileSize
umami-blocks-2938900-8.patch37.74 KB
interdiff.txt1014 bytes

Thanks!

Meanwhile, the attached patch should fix the tests (it looks like there was a minor mistake during the configuration export that caused the tests to fail).

navneet0693’s picture

navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commented
Status: Needs review » Reviewed & tested by the community

It works like charm! I manually tested it.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

@navneet0693 is there a chance you took screenshots of before after?

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Reviewed & tested by the community » Needs review
FileSize
umami-blocks-2938900-11.patch38.34 KB

The patch no longer applied (due to #2938170: Enable ALT text for Banner Block image field. being committed).

Here's a reroll - the only conflict was in the tests (core/profiles/demo_umami/modules/demo_umami_content/tests/src/Functional/UninstallDefaultContentTest.php). Hopefully can be a quick re-RTBC.

Regarding screenshots, there is supposed to be no change to the visual appearance before/after this patch.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Regarding screenshots, there is supposed to be no change to the visual appearance before/after this patch.

Agree, was just asking if #9 had screenshots to verify

JayKandari’s picture

JayKandari
Primary language Hindi
CreditAttribution: JayKandari as a volunteer and at QED42 commented
Status: Needs review » Needs work

Patch #11 didn't apply correctly with 

git apply umami-blocks-2938900-11.patch 
error: patch failed: core/profiles/demo_umami/themes/umami/css/components/blocks/banner/banner.css:39
error: core/profiles/demo_umami/themes/umami/css/components/blocks/banner/banner.css: patch does not apply

.
However I tried to apply it via patch -p1 < umami-blocks-2938900-11.patch it patches everything except the following one file. 

patching file core/profiles/demo_umami/themes/umami/css/components/blocks/banner/banner.css
Hunk #2 FAILED at 39.
1 out of 5 hunks FAILED -- saving rejects to file core/profiles/demo_umami/themes/umami/css/components/blocks/banner/banner.css.rej

Patch Review:

  • umami_disclaimer & umami_footer_promo have been converted to default custom blocks and properly imported in InstallHelper::importBlockContent()
  • Earlier user was able to enter javascript (eg: "javascript:alert('xss');") in link field of footer promo block. Now resolved.

Apart from the patch applying, this seems a great solution. In our previous OOTB meetings, we have had a discussion regarding blocks, as to keep these blocks as custom or the default custom blocks provided by core.

The actual design of Umami homepage has few more blocks. They can also be implemented in a similar way. There was some previous work doe around this. Not sure if this is relevant or not.

Marking this as "needs work" - just for not cleanly applying patch. else its good for RTBC. :) Cheers !!
Thanks!

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Needs work » Needs review
FileSize
umami-blocks-2938900-14.patch38.34 KB

Thanks for the review.

I rerolled the patch to fix the conflict with #2939940: Update stylelint rules color-hex-case to be consistent with Drupal's CSS standards (there was no change to the actual patch, just the surrounding code).

markconroy’s picture

markconroy CreditAttribution: markconroy as a volunteer and at Annertech commented
Status: Needs review » Reviewed & tested by the community

We live tested this on the weekly call today and are happy to RTBC it. Thanks a lot @david_rothstein for all your work on this.

navneet0693’s picture

navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commented
FileSize
Edit custom block Umami footer promo I drupal 8_6.png260.63 KB
Edit custom block Umami disclaimer I drupal 8.6.png357.78 KB
Screen Shot 2018-02-01 at 3.53.34 AM.png514.22 KB

@larowlan Apologies for responding to your comment. Attaching screenshots.

Only local images are allowed.

Only local images are allowed.

Only local images are allowed.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

adding @JayKandari to issue credits

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed as b1e477e and pushed to 8.6.x

Thanks

  • larowlan committed b1e477e on 8.6.x 
    Issue #2938900 by David_Rothstein, navneet0693, JayKandari: Replace...
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented

Note: Another possible improvement here would be to make the "3 issue bundle" image associated with the footer promo block into an uploaded image field (similar to how the "Umami Recipes Banner" does it), rather than hardcoded in the theme.

I wrote a patch for this at #2942007: Move the footer promo block image to a field, for better configurability and accessibility.

  • alexpott committed 327ca9f on 8.5.x authored by larowlan 
    Issue #2938900 by David_Rothstein, navneet0693, JayKandari: Replace...

Status: Fixed » Closed (fixed)

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