Problem/Motivation

core/modules/field_ui/src/FieldStorageConfigListBuilder calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123 Approach should be the same as #2505931: Remove SafeMarkup::set in ViewListBuilder (currently adding '#theme' => 'inline_list'
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it. The function concatenates known-safe strings and does not need to be individually sanitized.

Manual testing steps

  1. Install Drupal 8 using the Standard profile.
  2. Log in as the admin user
  3. Visit admin/reports/fields
  4. Compare the "Used In" column of the Field List report, specifically for the body field which should be a comma-separated list

User interface changes

N/A

API changes

N/A

CommentFileSizeAuthor
#18 2501971.18.patch1.11 KBalexpott
#18 13-18-interdiff.txt645 bytesalexpott
#15 2501971-13.png220.63 KBAnonymous (not verified)
#15 2501971-head.png238.51 KBAnonymous (not verified)
#13 2501971-9.patch1.1 KBstefan.r
#13 2501971-9.patch1.1 KBstefan.r
#8 2501971-8.patch1.05 KBstefan.r
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tetranz’s picture

tetranz CreditAttribution: tetranz as a volunteer commented
Related issues: +#2501975: Determine how to update code that currently joins strings in SafeMarkup::set()
tetranz’s picture

tetranz CreditAttribution: tetranz as a volunteer commented
Priority: Normal » Major
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Status: Active » Postponed

Postponing on the decision from #2501975: Determine how to update code that currently joins strings in SafeMarkup::set()

tetranz’s picture

tetranz CreditAttribution: tetranz as a volunteer commented
Assigned: tetranz » Unassigned

Unassigning myself for now until there is a clear direction for 2501975 and I have more time.

akalata’s picture

akalata CreditAttribution: akalata commented
Status: Postponed » Active

No longer blocked, though the discussion in #2501975: Determine how to update code that currently joins strings in SafeMarkup::set() might be valuable to identify the best solution in this case.

akalata’s picture

akalata CreditAttribution: akalata commented
Status: Active » Postponed
Related issues: +#2505931: Remove SafeMarkup::set in ViewListBuilder

Postponed for real on #2505931: Remove SafeMarkup::set in ViewListBuilder. Whatever pattern we find works there can most likely be used here as well.

akalata’s picture

akalata CreditAttribution: akalata commented
Title: Remove or document SafeMarkup::set in FieldStorageConfigListBuilder » [PP-1] Remove or document SafeMarkup::set in FieldStorageConfigListBuilder
Issue summary: View changes
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
FileSize
2501971-8.patch1.05 KB

This may work once #2505931-170: Remove SafeMarkup::set in ViewListBuilder is in.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: [PP-1] Remove or document SafeMarkup::set in FieldStorageConfigListBuilder » Remove or document SafeMarkup::set in FieldStorageConfigListBuilder
Status: Postponed » Needs review

#2505931: Remove SafeMarkup::set in ViewListBuilder just landed :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/field_ui/src/FieldStorageConfigListBuilder.php
@@ -119,13 +118,10 @@ public function buildRow(EntityInterface $field_storage) {
+    $row['data']['usage'] = [
+      '#theme' => 'inline_list',
+      '#items' => $usage,
+    ];

This is also not performance critical ... so having a render array is perfect here.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2501971-8.patch, failed testing.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

This needs to be updated still

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
2501971-9.patch1.1 KB
2501971-9.patch1.1 KB
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Title: Remove or document SafeMarkup::set in FieldStorageConfigListBuilder » Remove SafeMarkup::set in FieldStorageConfigListBuilder
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented
Status: Needs review » Needs work
Issue tags: -Needs manual testing
FileSize
2501971-head.png238.51 KB
2501971-13.png220.63 KB

This doesn't seem to work:

The last submitted patch, 13: 2501971-9.patch, failed testing.

The last submitted patch, 13: 2501971-9.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
13-18-interdiff.txt645 bytes
2501971.18.patch1.11 KB

Let's fix it then. Got to love how #type => 'table' works :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

Got to love how #type => 'table' works :)

Yeah…

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

Yep, manually tested in admin/reports/fields and this seems to work great.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented

FYI, if you put a 'data' key that is #theme => table's preprocess thing, #type => table treats the nested keys as elements.

*star rainbow*

https://www.drupal.org/node/1876710

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Diff looks much nicer.

Committed/pushed to 8.0.x, thanks!

  • catch committed 8540c2f on 8.0.x 
    Issue #2501971 by stefan.r, alexpott: Remove SafeMarkup::set in...

Status: Fixed » Closed (fixed)

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