Remove SafeMarkup::set in ViewsUIController::reportFields() [#2501937]

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

~~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'
~~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.
~~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:

Clean install of Drupal 8.
Create at least two views, displaying fields, that use the same fields.
Visit the "Used in Views" field list page at admin/reports/fields/views-fields
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

Comment	File	Size	Author
#23	2501937-23.patch	2.69 KB	stefan.r
#23	8.0.x: PHP 5.5 & MySQL 5.5 13,507 pass
#23	interdiff-21-23.txt	1.94 KB	stefan.r
#22	2501937-21.png	64.12 KB	Anonymous (not verified)
#22	2501937-head21.png	66.32 KB	Anonymous (not verified)
#21	interdiff-15-21.txt	3.58 KB	stefan.r
#21	2501937-21.patch	3.94 KB	stefan.r
#21	8.0.x: PHP 5.5 & MySQL 5.5 13,508 pass
#17	with-patch.png	18.16 KB	stefan.r
#15	2501937-15.patch	843 bytes	stefan.r
#15	8.0.x: PHP 5.5 & MySQL 5.5 13,337 pass
#14	2501937-12.png	76.8 KB	Anonymous (not verified)
#14	2501937-head.png	72.94 KB	Anonymous (not verified)
#12	2501937-9.patch	862 bytes	stefan.r
#12	8.0.x: PHP 5.5 & MySQL 5.5 13,337 pass
#8	2501937-8.patch	853 bytes	stefan.r
#8	8.0.x: PHP 5.5 & MySQL 5.5 13,337 pass
#8	interdiff-1-8.txt	799 bytes	stefan.r
#1	remove_safemarkup_set-2501937-1.patch	884 bytes	star-szr
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

star-szr

he/him

English

CreditAttribution: star-szr as a volunteer commented 7 June 2015 at 13:39

Status:

Active

» Needs review

File	Size
remove_safemarkup_set-2501937-1.patch	884 bytes

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.

Comment #2

star-szr

he/him

English

CreditAttribution: star-szr as a volunteer commented 7 June 2015 at 17:45

Status:

Needs review

» Postponed

Postponed on some further discussion on #2501975: Determine how to update code that currently joins strings in SafeMarkup::set() for now.

Comment #3

cilefen CreditAttribution: cilefen commented 14 June 2015 at 19:45

Comment #4

star-szr

he/him

English

CreditAttribution: star-szr as a volunteer commented 14 June 2015 at 23:48

Issue tags:

+VDC

Comment #5

akalata CreditAttribution: akalata commented 13 August 2015 at 17:57

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.

Comment #6

akalata CreditAttribution: akalata commented 13 August 2015 at 20:19

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.

Comment #7

akalata CreditAttribution: akalata commented 15 August 2015 at 17:45

Title:	Remove SafeMarkup::set in ViewsUIController::reportFields()	» [PP-1] Remove SafeMarkup::set in ViewsUIController::reportFields()
Issue summary:	View changes

Comment #8

stefan.r CreditAttribution: stefan.r commented 20 August 2015 at 20:18

File	Size
interdiff-1-8.txt	799 bytes
2501937-8.patch	853 bytes
8.0.x: PHP 5.5 & MySQL 5.5 13,337 pass

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

Comment #9

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 21 August 2015 at 09:03

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

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

Comment #10

dawehner

German

CreditAttribution: dawehner as a volunteer commented 21 August 2015 at 09:16

Status:

Needs review

» Reviewed & tested by the community

Perfect!

Comment #11

stefan.r CreditAttribution: stefan.r commented 21 August 2015 at 09:31

Status:

Reviewed & tested by the community

» Needs work

This needs to be updated still

Comment #12

stefan.r CreditAttribution: stefan.r commented 21 August 2015 at 09:33

Status:

Needs work

» Needs review

File	Size
2501937-9.patch	862 bytes
8.0.x: PHP 5.5 & MySQL 5.5 13,337 pass

Comment #13

stefan.r CreditAttribution: stefan.r commented 21 August 2015 at 09:33

Issue tags:

+Needs manual testing

Comment #14

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented 21 August 2015 at 09:51

Issue summary:	View changes
Status:	Needs review	» Needs work
Issue tags:	-Needs manual testing

File	Size
2501937-head.png	72.94 KB
2501937-12.png	76.8 KB

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

File	Size
2501937-15.patch	843 bytes
8.0.x: PHP 5.5 & MySQL 5.5 13,337 pass

Comment #16

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented 21 August 2015 at 10:46

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.

Comment #17

stefan.r CreditAttribution: stefan.r commented 21 August 2015 at 11:05

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

File	Size
with-patch.png	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?

Comment #18

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented 21 August 2015 at 11:12

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.

Comment #19

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented 21 August 2015 at 11:17

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>

Comment #20

stefan.r CreditAttribution: stefan.r commented 21 August 2015 at 11:23

Ah, that makes sense, thanks!

Comment #21

stefan.r CreditAttribution: stefan.r commented 21 August 2015 at 14:58

Status:

Needs work

» Needs review

File	Size
2501937-21.patch	3.94 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,508 pass
interdiff-15-21.txt	3.58 KB

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

Comment #22

Anonymous (not verified) CreditAttribution: Anonymous at XIO commented 21 August 2015 at 15:15

Issue summary:	View changes
Status:	Needs review	» Needs work

File	Size
2501937-head21.png	66.32 KB
2501937-21.png	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!

Comment #23

stefan.r CreditAttribution: stefan.r commented 21 August 2015 at 15:32

Issue summary:	View changes
Status:	Needs work	» Needs review
Issue tags:	-Needs manual testing

File	Size
interdiff-21-23.txt	1.94 KB
2501937-23.patch	2.69 KB
8.0.x: PHP 5.5 & MySQL 5.5 13,507 pass

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?

Comment #24

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 21 August 2015 at 15:44

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.

Comment #25

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 21 August 2015 at 16:49

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 :)

Comment #26

21 August 2015 at 16:49

alexpott committed 9f85d56 on 8.0.x

Issue #2501937 by stefan.r, Cottser, pjonckiere, akalata: Remove...

Comment #27

4 September 2015 at 16:54

Status:

Fixed

» Closed (fixed)

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

Remove SafeMarkup::set in ViewsUIController::reportFields()

Problem/Motivation

Proposed resolution

Remaining tasks

Manual testing steps

User interface changes

API changes

Comments

Parent issue

Related issues