Problem/Motivation

ViewsUIController::reportFields() calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code.
  • If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.

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.No new user-provided text is added here, sanitization is handled elsewhere.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

Manual testing steps

Do these steps both with HEAD and with the patch applied:

  1. Clean install of Drupal 8.
  2. Create at least two views, displaying fields, that use the same fields.
  3. Visit the "Used in Views" field list page at admin/reports/fields/views-fields
  4. Compare the output above in HEAD and with the patch applied. Specifically, review the "Used In" column.

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#23 2501937-23.patch2.69 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,840 pass(es). View
#23 interdiff-21-23.txt1.94 KBstefan.r
#22 2501937-21.png64.12 KBpjonckiere
#22 2501937-head21.png66.32 KBpjonckiere
#21 interdiff-15-21.txt3.58 KBstefan.r
#21 2501937-21.patch3.94 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,850 pass(es). View
#17 with-patch.png18.16 KBstefan.r
#15 2501937-15.patch843 bytesstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,657 pass(es). View
#14 2501937-12.png76.8 KBpjonckiere
#14 2501937-head.png72.94 KBpjonckiere
#12 2501937-9.patch862 bytesstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,654 pass(es). View
#8 2501937-8.patch853 bytesstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,643 pass(es). View
#8 interdiff-1-8.txt799 bytesstefan.r
#1 remove_safemarkup_set-2501937-1.patch884 bytesCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,707 pass(es). View

Comments

Cottser’s picture

Status: Active » Needs review
FileSize
884 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,707 pass(es). View

It would probably be nice to have SafeMarkup::implode (or ::join) again (#1825952: Turn on twig autoescape by default) for cases like this rather than this roundabout way.

Cottser’s picture

Status: Needs review » Postponed
cilefen’s picture

Cottser’s picture

Issue tags: +VDC
akalata’s picture

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

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

Title: Remove SafeMarkup::set in ViewsUIController::reportFields() » [PP-1] Remove SafeMarkup::set in ViewsUIController::reportFields()
Issue summary: View changes
stefan.r’s picture

FileSize
799 bytes
853 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,643 pass(es). View
Wim Leers’s picture

Title: [PP-1] Remove SafeMarkup::set in ViewsUIController::reportFields() » Remove SafeMarkup::set in ViewsUIController::reportFields()
Status: Postponed » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect!

stefan.r’s picture

Status: Reviewed & tested by the community » Needs work

This needs to be updated still

stefan.r’s picture

Status: Needs work » Needs review
FileSize
862 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,654 pass(es). View
stefan.r’s picture

Issue tags: +Needs manual testing
pjonckiere’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs manual testing
FileSize
72.94 KB
76.8 KB

The patch in #12 doesn't work, or at least broke something:

stefan.r’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
843 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,657 pass(es). View

Ah yes, that was wrong

pjonckiere’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Manual testing gives me the same result as #14.

And given that #12 came up green, we are missing coverage.

stefan.r’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
18.16 KB

@pjonckiere did you test #15? Against HEAD? Because it works fine for me:

with patch

I'm not sure about this needing automated tests, the item_list already has test coverage so asserting for the correct output of that theme function (ie. a <ul class="comma-list"> etc.) in every single caller seems a bit redundant? We also can't assert for the comma list as it's CSS based, so this would need to be tested manually.

If anything asserting that the field list is not empty would be a nice to have for the future?

pjonckiere’s picture

Status: Needs review » Needs work

Yes. The IS specifies to look at the "Used in Views" tab. Based on your screenshot, I think you are looking at the "Entities" tab?

I was more thinking of a "is the output there" kind of test. But fair enough.

pjonckiere’s picture

Fwiw, I see following in my source:

<td #theme="item_list" #items="<a href=&quot;/en/admin/structure/views/view/duplicate_of_list_text_view_test&quot;>duplicate_of_list_text_view_test</a> <a href=&quot;/en/admin/structure/views/view/list_text_view_test&quot;>list_text_view_test</a>" #context="comma-list"></td>
stefan.r’s picture

Ah, that makes sense, thanks!

stefan.r’s picture

Status: Needs work » Needs review
FileSize
3.94 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,850 pass(es). View
3.58 KB

Added that test and this should fix the issue from #18

pjonckiere’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
66.32 KB
64.12 KB

Now it's working, but a space got lost (cf screenies). I would think that a list_type of "comma-list" would do that for you, but that doesn't seem to be the case.

Other than that, the fix and the test look good to me!

stefan.r’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
1.94 KB
2.69 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,840 pass(es). View

I just manually tested myself and it didn't have the CSS issue (Firefox). Do you have the same problem in the comma separated item lists on admin/structure/views? Which browser was this?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/views_ui/src/Tests/ReportFieldsTest.php
@@ -0,0 +1,60 @@
+    $field_storage = FieldStorageConfig::create([
+      'field_name' => 'field_test',
+      'type' => 'integer',
+      'entity_type' => 'entity_test',
+    ]);
+    $field_storage->save();
...
+    $field = FieldConfig::create([
+      'field_name' => 'field_test',
+      'entity_type' => 'entity_test',
+      'bundle' => 'entity_test',
+    ]);
+    $field->save();

Nit: no need to create this variable. Just do:

Thing::create([
  …
])->save();

(You only want to assign it to a variable if you're going to use the variable.)

Can be fixed on commit, or not at all :) Doesn't really matter.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9f85d56 and pushed to 8.0.x. Thanks!

Didn't fix the unused variables... it's a test :)

  • alexpott committed 9f85d56 on 8.0.x
    Issue #2501937 by stefan.r, Cottser, pjonckiere, akalata: Remove...

Status: Fixed » Closed (fixed)

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