Remove SafeMarkup::set in ViewListBuilder

Let's use array() so the code inside the method uses the same style.

I think this is a good example of converting string concatenation into a render array.

It would be good to add to the ViewListBuilderTest unit test to check that paths and titles are properly escaped and not double escaped.

I'm going to review this.

Can we fake the test out somehow and trigger it to make sure it works? Maybe by ... turning auto escape off?

so reading the patch, the changes make sense, but I was unable to get the test to fail.

first I tried getting the test to fail with *most* of the fix by taking out the SafeMarkup::escape()... but it passes.
This might indicate (as @cilefen said in irc)
1) render array #markup is intrinsically safe because of some other thing 2) something else in this system fixes it

then I tried the test out on head, hoping first that it would pass, and then I could make a change to get it to fail, but it is failing... cause of some other reason. I attached the changes I did trying to get the test to work on head.

None of these "patches" should be committed, just attaching them so people can see exactly what I did.

@YesCT - here's a change on top of the patch that makes the test fail

oops, silly me - fat finger in the editor.

Reposting Chris' patch from #11, since that's what we actually want considered for commit.

I looked at that patch again. Approach makes sense to me, and we showed that the test is a good test.

I didn't test this manually. If we want to, we should figure out steps to reproduce to get to a page that uses this output.

Yes please. :)

The output affected by this patch is the views UI views listing at /admin/structure/views

With the patch the output looks correct (see 1st screenshot) - paths without % are linked, while those with are plain text.

I added in some extra text to each #prefix in the render array to confirm this code affects that page.

I also cloned a default View edited the path to have & in it. It shows up correctly in the UI and checking the HTML source verifies that it's correctly singly-escaped.

UI

HTML

Can we have test coverage and/or manual testing for a case with multiple page displays and paths? The unit test only seems to cover a single entry.

@xjm - the overview page I tested manually did have some with multiple paths. I don't know that more manual testing is needed.

Okay, just automated tests then. :)

Can we have test coverage and/or manual testing for a case with multiple page displays and paths? The unit test only seems to cover a single entry.

I thought to just do that, but then well, what the unit test is doing is this line of code:

    $row = $view_list_builder->buildRow($view);

    $this->assertEquals($row['data']['path']['data'][0]['#markup'], '/test_page', 'The path of the page display is not added.');
    $this->assertEquals($row['data']['path']['data'][1]['#markup'], '/<object>malformed_path</object>', 'The malformed path of the page display is not escaped.');

which then returns $data, which has multiple page displays and so paths, which we test. I think the justifies the RTBC

Hmmm. I really think it should have an integration test.

Hmmm. I really think it should have an integration test.

That is actually a 100% fair point. Safemarkup fixes should have integration tests ... unit tests are for logic checking, escaping happens on the application level.

Can we get with and without test patches?

Thank you @cilefen! Back to RTBC.

1. +++ b/core/modules/views_ui/src/ViewListBuilder.php
   @@ -81,11 +81,14 @@ public function load() {
        foreach ($this->getDisplayPaths($view) as $display_path) {
   -      $display_paths .= $separator . SafeMarkup::escape($display_path);
   -      $separator = ', ';
   +      $path_data[] = array(
   +        '#prefix' => $prefix,
   +        '#markup' => SafeMarkup::escape($display_path),
   +      );
   +      $prefix = ', ';
        }
   

   The output of getDisplayPaths is already escaped so this is not actually doing anything afaics. The mindset of all SafeMarkup patches needs to first ask the question is this really necessary - is it already escaped - or can we leave the render system to filter or escape it later.

2. +++ b/core/modules/views_ui/tests/src/Unit/ViewListBuilderTest.php
   @@ -141,8 +147,14 @@ public function testBuildRowEntityList() {
   +    $this->assertEquals($row['data']['path']['data'][0]['#markup'], '/test_page', 'The path of the page display is not added.');
   +    $this->assertEquals($row['data']['path']['data'][1]['#markup'], '/<object>malformed_path</object>', 'The malformed path of the page display is not escaped.');
   

   PHPUnit assertions should always be the expected value first plus the message should be what you are testing not an error message.

@alexpott nice! I'd rather not have to do that ugly looking pattern to comma separate some strings.

+1

+++ b/core/modules/views_ui/src/Tests/DisplayPathTest.php
@@ -58,6 +59,21 @@ protected function doBasicPathUITest() {
+    $this->drupalPostForm('admin/structure/views/nojs/display/test_view/page_3/path', array('path' => '<script>alert("hello");</script>'), t('Apply'));
+    $this->drupalPostForm('admin/structure/views/view/test_view', array(), t('Save'));
+    $this->drupalGet('admin/structure/views');
+    $this->assertRaw('<a href="' . $base_path . '%3Cobject%3Emalformed_path%3C/object%3E">/&lt;object&gt;malformed_path&lt;/object&gt;</a>, <a href="' . $base_path . '%3Cscript%3Ealert%28%22hello%22%29%3B%3C/script%3E">/&lt;script&gt;alert(&quot;hello&quot;);&lt;/script&gt;</a></td>', 'Two paths were filtered for XSS.');

After discussing with @xjm, realised that actually if if the <script> containing path has a % then it would be admin filtered away and wrong. We need to investigate using a twig template to comma join the safe strings that displayPaths is generating.

Probably need a renderer and not sure if mocking that is possible... but I did the same test that was done before.

Thanks @joelpittet.

I think this still needs testing and test coverage for the % case, as well as combinations of that and the linked path case.

The safe_join/inline template is an improvement. I do think the comma-separated item list template would be more semantic and help us in a bunch of places.

I'm a bit gun-shy on unit test still but here's an attempt to add to what @alexpott wrote. Hopefully this will do the job?

I did with and without text after the placeholder in a local test, both show the same result so I expect this will do the job.

1. +++ b/core/modules/views_ui/src/Tests/DisplayPathTest.php
   @@ -60,6 +61,21 @@ protected function doBasicPathUITest() {
   +    $this->drupalPostForm(NULL, array(), 'Add Page');
   +    $this->drupalPostForm('admin/structure/views/nojs/display/test_view/page_2/path', array('path' => '<object>malformed_path</object>'), t('Apply'));
   +    $this->drupalPostForm(NULL, array(), 'Add Page');
   +    $this->drupalPostForm('admin/structure/views/nojs/display/test_view/page_3/path', array('path' => '<script>alert("hello");</script>'), t('Apply'));
   

   Can we add another test so we test escaping the path where a link in not generated by ie. when it includes a placeholder like %1. See ViewListBuilder::getDisplayPaths().

2. +++ b/core/modules/views_ui/src/Tests/DisplayPathTest.php
   @@ -60,6 +61,21 @@ protected function doBasicPathUITest() {
   +    $this->assertRaw('<a href="' . $base_path . '%3Cobject%3Emalformed_path%3C/object%3E">/&lt;object&gt;malformed_path&lt;/object&gt;</a>, <a href="' . $base_path . '%3Cscript%3Ealert%28%22hello%22%29%3B%3C/script%3E">/&lt;script&gt;alert(&quot;hello&quot;);&lt;/script&gt;</a></td>', 'Two paths were filtered for XSS.');
   

   Let's use ->assertEscaped() as this is escaped and not filtered text.

3. +++ b/core/modules/views_ui/tests/src/Unit/ViewListBuilderTest.php
   @@ -89,7 +89,10 @@ public function testBuildRowEntityList() {
   +        $this->returnValue('placeholder_page/%')));
   

   Let's put some html in here.

[Edit:deleted comment on wrong issue - thanks @cilefen!]

working on this

addresses #50

Re-RTBC from #44 with added test coverage for script tags with % as requested by #46 #48 and #50

Also thanks @larowlan for figuring that out.

+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -103,7 +97,13 @@ public function buildRow(EntityInterface $view) {
+        'path' => array(
+          'data' => array(
+            '#type' => 'inline_template',
+            '#template' => '{{ display_paths|safe_join(", ") }}',
+            '#context' => array('display_paths' => $this->getDisplayPaths($view)),
+          ),
+        ),

I thought we agreed to add a real template for this since lots of places need this. Or maybe rather than a template we could add a static function to InlineTemplate to return a template to do this?

@alexpott yes, you are correct. Here is more-or-less what we discussed. Though my SUGGESTION-foo is lacking in the theme layer. I can't get the system suggestion to pickup! the seven one picks up like a charm so this should look fine for testing... Need help from @Cottser or @lauriii may know this trickery.

+++ b/core/modules/views_ui/tests/src/Unit/ViewListBuilderTest.php
@@ -141,8 +156,16 @@ public function testBuildRowEntityList() {
+    $display_paths = $row['data']['path']['data']['#context']['display_paths'];

Note to the next patch/patcher:
['#context']['display_paths'] just needs to be ['#items'] now for that test to pass.

+++ b/core/themes/seven/templates/item-list--inline.html.twig
@@ -0,0 +1,33 @@
+      {{ loop.first ? '' : ', ' }}{{ item.value }}

Is there a specific reason why loop.last cannot be used? It would make this a bit more easier to understand when someone is over viewing the template

@lauriii, nope just change the order, doesn't matter to me.

Main problem is we need to figure out why the system template won't pickup the suggestion.

Maybe it was picking up the suggestion, and the test hadn't been updated for the new template?

From system_theme():

    // Normally theme suggestion templates are only picked up when they are in
    // themes. We explicitly define theme suggestions here so that the block
    // templates in core/modules/system/templates are picked up.

So, let's do the same here! ... Though I'm still trying to figure out how.

The suggestion should be item_list__inline. __ is the default delimiter. If you want -- you have specify it with the third parameter.

Oh wait, ignore that. I see, that was dumb. Hmm. It isn't picking up the file at all? How can this be tested?

Easiest manual testing location I've found is in the Views UI, the views list page (admin/structure/views) where it lists multiple paths (Path column).

joelpittet found that he had to add core/themes/seven/templates/item-list--inline.html.twig for Seven in order to test this manually.

The attached path removes this additional template file (since things ideally should work without it), and adds the item_list__inline declaration to drupal_common_theme(). It also makes laurii's preferred tweak from #61.

The screenshots are as follows, all showing the Path column for the core Files view:

• #64 (with the template in Seven)
  
  
• #64 (with the template removed)
  
• #68 (template in /core/modules/system being used, but values not printed
  
• adding "myterm" to the /core/modules/system template, proving that it is being called
  

I got stuck on trying to get phpstorm and xdebug talking to each other to figure out what was going on with the values coming from ViewListBuilder.

1. +++ b/core/includes/theme.inc
   @@ -1726,6 +1726,9 @@ function drupal_common_theme() {
   +    'item_list__inline' => array(
   +      'variables' => array('items' => array(), 'title' => '', 'list_type' => 'inline', 'attributes' => array(), 'empty' => NULL, 'context' => array()),
   +    ),
   

   I think we need an explicit test for this template.

2. +++ b/core/modules/system/templates/item-list--inline.html.twig
   @@ -0,0 +1,33 @@
   +  {%- if title is not empty -%}
   +    <h3>{{ title }}</h3>
   +  {%- endif -%}
   

   are we sure we want to support titling in an "inline" item list?

I think removing the title does make sense.

I couldn't find anyplace where a template itself is tested -- @alexpott did you mean test the template suggestions? I see a few examples of that in core.

@alexpott re #71if we extend from item list we should keep the title I think, if we go to a new template likely not needed to have a title, but they should be as close as possible to eachother or they really don't need to share the same namespace.

@akalata We shouldn't have to make that change but thanks for showing that. I have a feeling the preprocess is not working for the suggestions, that may be completely another issue that we still have hanging around.(@lauriii or @Cottser may no about this) if it is indeed the case. Here is the issue #939462: Specific preprocess functions for theme hook suggestions are not invoked

Are we trying to shoehorn ourselves into the item_list template too much? I think we might be.

1. Core-based template suggestions still have issues what we're adding workarounds for; meanwhile, there are currently 4 SafeMarkup::set() issues postponed on this one.
2. A new template could be extended to support the unsolved patterns in #2501975: Determine how to update code that currently joins strings in SafeMarkup::set():
   

   1. A determinate (but possibly conditional) series of translated strings, joined together with whitespace (spaces, break tags, paragraph tags)

   as well as
   

   An indeterminate, imploded list of translated or sanitized strings that is a list of items.

3. This new, inline_list template would accept a configurable separator string, where the current item_list template only allows ul/ol. This matches up with established UI patterns; for instance, the "multiple field settings" option in the Views UI.
4. It makes more sense to remove the title for a completely new, rather than extended, template.

Posting progress on implementation switch before working on tests.

Let's try this. Ignoring #75.

Updating per @joelpittet's review in-person at MWDS:

1. list_type is not a variable in the inline_list template
2. use Twig's for/else/endfor syntax for adding the conditional empty text

So excited about the use-case for my favourite front-end syntax-sugar in twig the for...else loop!

+++ b/core/modules/system/templates/inline-list.html.twig
@@ -21,9 +20,6 @@
+
 

Minor nit to remove the double line-break at end of file on next roll. Thanks for the fixes and trying that out @akalata!

1. +++ b/core/includes/theme.inc
   @@ -1066,6 +1067,73 @@ function template_preprocess_item_list(&$variables) {
   + *
   + * @param array $variables
   + *   An associative array containing:
   + *   - items: An array of items to be displayed in the list. Each item can be
   + *     either a string or a render array. If #type, #theme, or #markup
   + *     properties are not specified for child render arrays, they will be
   + *     inherited from the parent list, allowing callers to specify larger
   + *     nested lists without having to explicitly specify and repeat the
   + *     render properties for all nested child lists.
   + *   - separator: A string to separate list items.
   + *
   ...
   +function template_preprocess_inline_list(&$variables) {
   

   I don't actually get this sub items render array, so I was wondering whether it would be a good idea to give some examples with @code @endcode

2. +++ b/core/modules/system/system.module
   @@ -256,6 +256,14 @@ function system_theme_suggestions_html(array $variables) {
   +function system_theme_suggestions_item_list(array $variables) {
   +  $types = ['ul', 'ol', 'inline'];
   +  return theme_get_suggestions($types, 'item_list');
   

   MH, just wondering: doesn't the type ul vs. ol depends on the kind of data you list, so should this be the responsibility of the caller? It feels like ordered vs. unordered list is semantically different. I was starting to think about that because I wondered why ul should be preferred over ol. So what about something like $variables['#list_type'] or so

3. +++ b/core/modules/system/system.module
   index 0000000..9d7a63b
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/system/templates/inline-list.html.twig
   
   +++ b/core/modules/system/templates/inline-list.html.twig
   +++ b/core/modules/system/templates/inline-list.html.twig
   @@ -0,0 +1,25 @@
   
   @@ -0,0 +1,25 @@
   +{#
   

   We seem to not have a classy overrides with the attributes involved?

4. +++ b/core/modules/views_ui/src/ViewListBuilder.php
   @@ -113,7 +107,12 @@ public function buildRow(EntityInterface $view) {
   +            '#theme' => 'inline_list',
   +            '#items' => $this->getDisplayPaths($view),
   

   I really like this kind of API

80.1 - I'm not sure, that was part of the item-list template that I copied.

80.2 - This is actually from an earlier patch and is no longer needed.

80.3 - Oooh, good catch! Though now I'm very tempted to remove the "wrapper" attribute entirely, since it makes the Classy template less streamlined.

After more discussion with Joel, we agreed to remove both types of attributes, which means that we do not need to add the preprocess function or Classy template.

I will work on a test now.

But we still need a preprocess function to sanitize the separator value...

1. +++ b/core/includes/theme.inc
   @@ -1065,6 +1066,24 @@ function template_preprocess_item_list(&$variables) {
    
   +
   ...
   +
   +
   

   Can remove one new line before and after this function.

2. +++ b/core/includes/theme.inc
   @@ -1065,6 +1066,24 @@ function template_preprocess_item_list(&$variables) {
   + *   An associative array containing:
   + *   - items: An array of items to be displayed in the list.
   

   Could you add separator to the list of items contained in the $variables?

3. +++ b/core/includes/theme.inc
   @@ -1065,6 +1066,24 @@ function template_preprocess_item_list(&$variables) {
   +  // Since the separator may be user-specified, it must be sanitized.
   +  $variables['separator'] = SafeString::create(Xss::filterAdmin($variables['separator']));
   

   Because we escape all variables in the template this ensures we prevent Xss attacks but also allows certain tags through. May need to re-work that to include why we are marking values as safe.

4. +++ b/core/modules/system/src/Tests/Theme/FunctionsTest.php
   @@ -168,6 +168,50 @@ function testItemList() {
   +    $variables['items'] = array('Un', 'Deux', 'Trois');
   +    $expected = 'Un, Deux, Trois';
   

   Moar bunnies are needed here.

Re 84.2, I also added $variables['empty'] for completeness' sake.

1. +++ b/core/includes/theme.inc
   @@ -1066,6 +1067,25 @@ function template_preprocess_item_list(&$variables) {
   + *   - separator: A string to separate list items.
   + *   - empty: A message to display when there are no items.
   

   Thanks for adding both:)

2. +++ b/core/includes/theme.inc
   @@ -1726,6 +1746,9 @@ function drupal_common_theme() {
   +      'variables' => array('items' => array(), 'separator' => ', ', 'attributes' => array(), 'empty' => NULL, 'context' => array()),
   

   That context variable is throwing me a bit. Anybody know where this came from or what it's for?

3. +++ b/core/modules/system/src/Tests/Theme/FunctionsTest.php
   @@ -168,6 +168,50 @@ function testItemList() {
   +    /**
   

   2 spaces back.

4. +++ b/core/modules/system/templates/inline-list.html.twig
   @@ -0,0 +1,21 @@
   + * - items: An array of items.
   + * - separator: A string to separate list items.
   + * - empty: A message to display when there are no items. Allowed value is a
   + *   string or render array.
   

   We have some fun rules for these variables:) We don't actually say the 'type' of data. For array, we say 'list'.
   https://www.drupal.org/node/1823416#datatypes

Thanks for the continued reviews!

86.2 - No clue. :) Just following what was already established in core.

Fixing a typo and removing the context variable (which was added for a specific use case and not globally, per @joelpittet's sleuthing).

+++ b/core/modules/system/src/Tests/Theme/FunctionsTest.php
@@ -168,6 +168,50 @@ function testItemList() {
+    // Verify that a non-default separator is rendered
...
+    // Verify that HTML separators are properly rendered
...
+    // Verify that the separator is sanitized

Periods at end of the lines are the last nits I could find, they could be fixed on commit:)

I think this is good to go, has ample tests from multiple angles (the safemarkup and the new theme template).

Great improvements since my last review!! Great work, thank you!

+++ b/core/includes/theme.inc
@@ -1066,6 +1067,25 @@ function template_preprocess_item_list(&$variables) {
+  $variables['separator'] = SafeString::create(Xss::filterAdmin($variables['separator']));

Maybe we should only permit a list of specific tags rather than the entire admin list. This is because the admin list includes elements that are block level elements like p and div. These elements don't make much sense here. In fact is there any element other than br that makes sense?

I like that idea @dawehner & @cilefen. Anybody that needs more can write a preprocess and add whatever they want.

+++ b/core/modules/system/src/Tests/Theme/FunctionsTest.php
@@ -168,6 +168,50 @@ function testItemList() {
+    // Verify that empty text is not displayed when there are list items.
+    $variables = array();
+    $variables['empty'] = 'No items found.';
+    $variables['items'] = array('Rabbit', 'rabbit', 'rabbit');
+    $expected = 'Rabbit, rabbit, rabbit';
+    $this->assertThemeOutput('inline_list', $variables, $expected, '%callback does not print empty text when there are list items.');

this is asserting the item is displayed.

this is not asserting that the empty text is not displayed.

Hm. I guess it might be. Looking up assertThemeOutput() seems to be asserting all the themed output is equal to expected. (Not what I assumed which was that it was asserting the themed output contained the expected string.)

so this is fine.

Looks like a nice solution, thanks all.

So just a word on performance... this orders of magnitude slower than implode. Doing a small array of 20 items 10 times with renderer primed.

Render array in 0.021458864212036 seconds
implode() in 0.0000090599060058594 seconds

If you prime the render with the template you get...

Render array in 0.000084877014160156 seconds
implode() in 0.0000090599060058594 seconds

So the good thing is that only the first is costly - loading PHP code etc... but basically doing this seems to add 20ms on any page that uses it. Th.at that maybe on a webserver with opcaching enabled the startup cost will be smaller.

Tripple post! Nice

Well yeah, for sure this is no suprise here at ALL.

I think 20 ms for something so simple is pretty much unacceptable? Why would Twig be this slow for something so simple? Is it a case that isn't optimized well in Twig, because of the inline template?

Tentatively blocking on @Fabianx' feedback.

So as I suspected with opcache this gets a bit better.

Render array in 0.0040850639343262 seconds
Implode in 0.000007.8678131103516 seconds

Still 4ms - maybe this should be a theme function and not twig by default.

This is not an inline template, it's a regular template which goes through the whole theme system. Renderer, preprocess etc.

Just a thought but would it make sense to use the safe join filter instead of a for loop? Might be worth a trial/comparison for perf.

@Cottser but we can still use a theme function instead right - like we do for other performance related templates?

We are really trying to go to templates for a consistant dx when looking for marking up variables. Didn't win everywhere, but still trying.

Only two theme conversions blocked on performance: theme_indentation and the views fields

If we're leaning towards a theme function I'd probably rather we just used an inline template (or maybe better yet, safe join in the template if feasible) if that brings the overhead down to something deemed more reasonable.

While we're very surprised that a single, simple Twig template costs us 4 ms (on the first use), that pretty much implies every other Twig template takes >= 4ms.

So… is this really that big of a deal? And did we even know about this "4 ms/unique template" rule-of-thumb?

We've been going off a rough less than 1% regression(mostly wall time) if it's not admin facing and with scenario that is common for that template when doing our performance tests with xhprof.

The worst offender was the small but repeated templates or many calls to twigs attribute.

Worst performance that got in that we profiled was not twig but #type table conversion on the simple test table of 30-40% regression but it got rammed through...

Twigs mega patch was ~10% which is what triggered the performance gate applied to all conversions, then reduced to non-admin conversions.

Twigs c extension will speed up twigs attribute() calls, which iirc is all it does. And also why safe_join would be faster as cottser suggested

We discussed this on IRC and likely an inline template or a safe join won't make any difference in terms of performance. Maybe we could go with a theme hook?

Didn't mean to remove the item list empty tests, just the inline list ones.

+++ b/core/includes/theme.inc
@@ -1083,6 +1083,32 @@ function theme_indentation($variables) {
+  return implode($variables['separator'], $variables['items']);

Just wondering, do we have to do an Xss::filterAdmin() on the imploded string, even if the glue and the string are already safe?

re #113 nope we don't.

So the theme function does not seem to make much difference to the compiled template. Doing the same test against #112

Render array in 0.0037128925323486 seconds
Implode in 0.0000069141387939453E-6 seconds

I'm inclined to think we should stick with twig template and accept the cost. It also makes me wonder about the profiling of theme_indentation vs the twig template.

What about adding a comment explaining that you better don't use it, but rather implode directly in a template?

So 3.7ms always for a theme hook vs 20ms first and once the renderer is primed .8ms for a twig template?

Why is it still slow with a theme hook, have we profiled this to see if there are there any particularly inefficient/slow functions calls during rendering?

@stefan.r the 20ms was without opcode cache. I didn't bother profiling the theme hook without opcode cache.

So 4ms with Twig template, 3.7 ms with theme function. So basically the same. That's very interesting. I too am surprised that a simple theme function is basically equally fast/slow.

Really looking forward to @Fabianx' opinion about this :)

Apparently the 20ms included some theme initialization which we do on every request anyway, doing Drupal::service('renderer')->renderPlain( ['#theme' => 'inline_list', '#items' => $items]); 20 times in a row without opcache I get:

• theme_inline_list(): ~4ms
• Twig template, if using other twig templates in the same request: ~6ms
• Twig template, if we had already loaded the item list template earlier in the request: ~5ms
• Twig template, if there are no other Twig templates loaded yet: ~25ms
• Twig template, if theme not yet initialized through ThemeManager::initTheme(): ~50ms

When I enable opcache these actually all go up by a bit so something may be wrong on my setup :)

Discussed with @alexpott and @stefan.r. The scenarios we will profile are:

Scenario A

• Install Standard.
• Visit admin/structure/views.

Scenario B

• Install Standard.
• Create 50 views, each with 2-4 page displays.
• Visit admin/structure/views

Patches to profile

• HEAD
• Patch from #93: https://www.drupal.org/files/issues/remove_safemarkup_set-2505931-93.patch
• Patch from #112: https://www.drupal.org/files/issues/2505931-111.patch

Twig template, if theme not yet initialized through ThemeManager::initTheme(): ~50ms

... I would recommend to init the theme before the benchmark code.
The theme manager potentially has to initialize the entity system (due to access checking for example), which then is really costly.

Alright, discussed with Moshe and tlittlefield, and Moshe's recommendation was that this is not worth profiling unless we put it on something like nodes, comments, etc. So we should go ahead with whatever approach has the best themer and developer experience.

re-uploading #93 as that seems to be what was preferred (and RTBC'ed) earlier on

And just for future reference, the benchmark script used in #120

Alright — sorry for the distraction!

Thanks, all.

Nice, this looks like a great solution. Thanks also for looking into the performance concerns. Couple smallish points of feedback:

1. +++ b/core/includes/theme.inc
   @@ -1066,6 +1067,25 @@ function template_preprocess_item_list(&$variables) {
   + *   - separator: A string to separate list items.
   ...
   +  // Since the separator may be user-specified, it must be filtered to permit
   +  // some HTML (such as <br />) to pass through.
   +  $variables['separator'] = SafeString::create(Xss::filter($variables['separator'], ['br']));
   
   @@ -1726,6 +1746,9 @@ function drupal_common_theme() {
   +    'inline_list' => array(
   +      'variables' => array('items' => array(), 'separator' => ', ', 'attributes' => array(), 'empty' => NULL),
   +    ),
   

   The comment says "some HTML (such as <br />)" which would imply that it's not just <br />... but it is just <br />.

   Also, <br /> tags seem inherently not inline to me. They're also less semantic than lists or commas. There are also no usecases in core. So I'm not clear on why we need to support that.

   Finally, I'm also hesitant as to whether we need to support multiple separators at all. All of the four issues in core use commas and it seems like the majority usecase. Could we at least default to a comma if the separator is not provided? Edit: It looks like we are actually providing the comma as a default; can we document that in the preprocess docblock?

2. +++ b/core/includes/theme.inc
   @@ -1066,6 +1067,25 @@ function template_preprocess_item_list(&$variables) {
   + * @see https://www.drupal.org/node/1842756
   

   This is... odd. We don't usually link change records from API docs since change records are not permanent documentation. I also don't understand what relevance this change record has when visiting it. At a minimum, we should explain WHY the developer might want to read this link, but do we need it at all?

Regarding the @see, that must have sneaked in while people copypasted the preprocess for theme_item_list() (ie _item_ list)

I agree <br /> is not inline, and people should use lists for that in most cases, so we can probably remove that until there is an actual use case.

Hm that preprocess is wrong, it just needs to Html::escape() or filter if unsafe

+++ b/core/includes/theme.inc
@@ -1730,6 +1746,9 @@ function drupal_common_theme() {
+      'variables' => array('items' => array(), 'separator' => ', ', 'attributes' => array(), 'empty' => NULL),

+++ b/core/modules/system/templates/inline-list.html.twig
@@ -0,0 +1,20 @@
+ * - empty: A message to display when there are no items.
...
+{%- else -%}
+  {{- empty -}}
+{%- endfor -%}

I don't think we should be supporting empty text.

Given that we're not going to be supporting br (which seems fine). Why don't we go the whole way and just limit this to comma lists and then we'll never have to filter the or escape the comma because it'll be hardcoded?

Fine with not supporting empty, just thought what a great opportunity to show off that front end syntax-sugar. Mostly because the use-cases may be slim to none.

I'm a bit concerned about not doing the <br> tags though because we have a use-case or two for that even in core on the list. <br> is not a block HTML tag, it's an inline tag, it just breaks the line. I can see how that can get look like it's not inline, but it is. Here's a nice little note on that:
http://stackoverflow.com/questions/1369234/are-hr-and-br-inline-or-block...
http://htmlhelp.com/reference/html40/inline.html

The one I was refering to that exists in core is #2550985: Remove SafeMarkup::set in _batch_test_finished_helper() but I was thinking ahead to help consolidate views UI @see screenshot in #74

@joelpittet there was a patch in #134, did you mean to set this to NW?

As to your point about "inline-ness", I can understand that "inline" applies to br elements, but I'm not sure that "list" does necessarily? It's not exactly semantic markup to use it for a list, nor is it very accessible, and I wonder if core has any valid use cases for this at all? #2550985: Remove SafeMarkup::set in _batch_test_finished_helper() looks like it should be using <ul>.

Even having looked at #74, I can't really come up with a use case for contrib modules to want to use another separator for an inline list (other than <br />), but for those cases they should almost always be using a <ul> instead anyway. If anything we could make another template for those cases later? Things that are separated by <p> or spaces don't really sound like inline lists to me.

As to the patch in #134, I agree with @xjm and @alexpott that hardcoding the separator as a comma makes sense as it would cover most use cases (at least for core). Which means we could probably also rename it to comma_list.

@stefan.r yes, not a fan of removing the separator, nor the new name for the theme function.

Re #137 for a list:

.
list: a number of connected items or names written or printed consecutively, typically one below the other.

I think that applies to what we are trying to make here.

I'd be okay with supporting non-markup separators and letting Twig escape them, defaulting to , . Or with doing it in CSS, actually. I really would prefer not to add <br /> to the scope though because it's not good practice.

I'd rather not do this template if it's only usefulness is to do what implode() does but without the extension.

I can can concede the <br> removal as it removes the preprocess anyways and the only use-case currently is a poorly written test. Also if anybody wants they can extend their preprocess in their theme to support other things very easily.

I'd like to keep 'inline_list' as the name, can that work?

Let's!

So just to summarize, #147 is essentially the same as #93 but without the empty message and without linebreak support...

Thank you, re-RTBC, hope (most) everybody agrees.

Yup, I'm good with that too.

The more I think about the more I think it is absolutely absurd to have a themeable comma separated list in core and that when it is used will add at least a couple of 2ms to a response. Do we need ever to customise how a comma separated list looks? Also we have a performant safe alternative in #2554073-12: Allow #markup to be an array of strings and join them safely that is only 4 times as slow as an implode + escaping whereas this is about 17 times as slow.

I would have expected this to use a <ul> with inline styling.

Having the list themed as a literal comma separated list seems like it would be an issue for RTL languages.

See for example http://www.oxwall.org/forum/topic/10218 and http://blogs.transparent.com/arabic/punctuation-in-arabic/

If we use a ul with inline CSS styling (like this very old alistapart post: http://alistapart.com/article/taminglists), then an RTL stylesheet should be able to handle it.

With the comma being in the actual markup we print, afaik that's not possible at all.

So I also disagree with the new theme hook, although I'm not sure #2554073: Allow #markup to be an array of strings and join them safely is actually right for this case either.

stefan_r pointed out in irc why the non-semantic list is bad for screenreaders: http://www.idpf.org/accessibility/guidelines/content/xhtml/lists.php - comes down to being able to skip easily backwards and forwards through the list. Combined with my RTL concern (which I'm not 100% sure is right, but seems valid as far as I understand it), using a semantic list feels right here.

So I think we can do this:

Use 'item_list' - no extra template.

Add 'inline' and 'comma-separated' classes - we already have some CSS for inline lists (to support theme_links which also uses a semantic list for inline links), we'd need a new bit of CSS for the comma-separated bit.

+1 to this, this relieves us of the need for a new twig template for what can be done in CSS in a much more accessible way.

As to the performance hit vs faster options, it's probably 1ms/2ms max in most scenarios considering the benchmarks in #2554073: Allow #markup to be an array of strings and join them safely. It was also pointed out that if we re-use the item list template, once that template is primed all other calls are cheaper.

+1. This seems like a well-balanced approach. Front-end folks are happy because it's a consistent (Twig-based) approach, accessibility people are happy because it's fully accessible.

In IRC, the following URL was also surfaced: https://stackoverflow.com/questions/1517220/how-to-style-unordered-lists...

Regarding configurable separators (#74.3): in those cases maybe we can re-use the pattern from this issue and make the CSS class configurable instead. A comma class will likely cover 90%+ of use cases, for the other ones we can add new classes as needed?

Looks great!

+++ b/core/modules/system/css/components/links.theme.css
@@ -13,6 +13,12 @@ ul.inline li {
+ul.comma-list li {
...
+ul.comma-list li:last-child:after {

I think (but am not sure!) we want ul.inline.comma-list here instead?

(Because only ul.inlines are eligible to become comma lists.)

To me this looks the same before and after, maybe someone can confirm by going to admin/structure/views and comparing with and without the patch in #160:

With patch:

Without patch:

Removing units from margin

Added RTL support:

+++ b/core/modules/system/css/components/links.theme.css
@@ -13,16 +13,23 @@
 }
-ul.comma-list.inline li {
+[dir="rtl"] .item-list ul.comma-list.inline {
   padding-left: 0;
   padding-right: 0;
   margin-left: 0;
   margin-right: 0;
 }

Hrm, why only for RTL? I think this is missing the non-RTL selector?

+++ b/core/modules/system/css/components/links.theme.css
@@ -13,6 +13,25 @@ ul.inline li {
+[dir="rtl"] .item-list ul.comma-list.inline {

This looks overly specific. Shouldn't need the ul or two class names to do this.

I don't think the links css file is the best place for this. Wouldn't it be better in item-list?

...especially, if you are specifying ".item-list" but that doesn't seem necessary either.

It's overriding the rtl selector only, ltr doesnt need it on the ul

Yes! This is what I expected when I first proposed the issue; glad to see it is indeed possible. Thank you!

Edit: I have no idea regarding the proper placement of the CSS under our new standards.

+++ b/core/modules/system/css/components/links.theme.css
@@ -13,6 +13,25 @@ ul.inline li {
+.item-list  ul.comma-list.inline li {

Two spaces here, should be one.

I don't think the links css file is the best place for this. Wouldn't it be better in item-list?

Good point!

...especially, if you are specifying ".item-list" but that doesn't seem necessary either.

Well, it's currently explicitly designed to work for #theme => item_list, but I think you're right, it doesn't need that. It'd be perfectly valid for somebody to create markup without #theme => item_list — as long as it's an <ul> with both the inline and comma-list classes, it should work.

So, disregard the previous point, we can keep it in this CSS file, but should remove the .item-list parts of the selectors.

Shouldn't need the ul or two class names to do this.

The comma-list class only makes sense on inline <ul>s, so this does make sense. But perhaps it should be inline--comma-list, for BEM compliance?
We do want ul to be present in the selector, for consistency with ul.inline.

To be discussed in a follow-up: maybe we shouldn't be hardcoding the classes in the calling code, we can do that in the preprocess. I'd rather see something like '#list_type' => 'comma'.

Removed the item-list selector as it was not needed. I think the item-list.theme.css is actually better suited than the links.css one, but ideally this should be in its own file. Maybe follow-up material :)

As to what to call the classes, I could live with inline--comma-list if we had #169, but now that calling render arrays have to hardcode the classes and spell them out, comma-list may be better.

@WimLeers / @davidhernandez it would be good to hear if we're good in terms of CSS now.

So really this logically (ha!) should be a variant of the item list, which was in the patch at one point. Having a item-list--comma template somewhere, because this is a variation of the item list itself, not a variation of the contents of the list. But, whatever, that would just be a change to satisfy some css/bem dogma. I'm not suggestion we bother for such small use case.

I think .item-list__comma-list should be fine. It's attacking the subelement of the component, instead of a variation of it. I don't see any need for the word "inline". Isn't that kind of by definition of a comma list? I also still don't see a reason for the ul. The ul isn't providing any more semantic meaning. You can try it, but it shouldn't matter, and it is better to be less specific. Even if someone changed it to an ol or nested divs or something, so what. They'd have to adjust it anyway.

"inline" was just taking advantage of the existing inline class, but we don't need it necessarily, nor do we need the ul target.

As to whether we had a variation of item-list in the patch at some point --- we didn't, we just had a comma-list twig template (#134). Also it's better for this not to be a variant for performance reasons, we'd rather reuse a template already used on the same request (see benchmarks in #2554073: Allow #markup to be an array of strings and join them safely).

@davidhernandez told me he'd roll a new patch so I'm leaving this issue be this for now :)

I've made the unfortunate mistake of ...thinking about it.

1) The CSS should be in a separate file, not item-list.css. Probably item-list.module.css. The reason being that the .theme file will get moved to Classy and the more I think about it this the comma list is functional, not stylistic. This should work for core and live in core. The CSS should be overridable, but considered functional, so it needs to stay in core.
2) We hate when class names are added in php. We should pass the list type to the template and produce the class in the template. This makes it possible to create other list types without any modification. It also makes it overridable from the template without modification in preprocess or someplace else.

I'm going make the CSS changes and post that, in case everyone hates the idea and doesn't want to make any template changes. Buuut ... while looking I think I found something to make this easier. I'm going to check that first. Also, I think I found some visual regression in Bartik that we might want to deal with.

I was looking to change this to use a variable for the template and realized Cottser is a genius! item_list already has a generic context variable which was needed to customize the search results item_list, so I reused it. Forward thinking reusability win! \o/

The CSS is actually a bit of a conundrum. This can't really be done right without making some out of scope changes which I don't want to do and muddy up this issue. So I left the CSS in the item-list.theme.css file. We can try to fix it later when the files are moved to Classy. There were also a couple of visual regressions in Bartik and with some of the RTL. I checked all themes LTR and RTL and it looked ok to me. I also ditched the inline class. It is mostly designed for things like tabs and requires doing more overridding than necessary so why bother? All we need it for is the display: inline. The same with the uls. I can't leave them out because of Seven's styling, but need the CSS to be in Classy first before cleaning that up. Again out of scope.

@davidhernandez nice work!

+++ b/core/modules/system/templates/item-list.html.twig
@@ -12,12 +12,17 @@
+ *   - list_style: The custom list style.

+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -111,7 +111,7 @@ public function buildRow(EntityInterface $view) {
+            '#context' => ['list_style' => 'comma-list'],

this is indented by 1 space only, shouldn't it be 2?

Also, rather than reuse the context variable isn't it better DX to create a new variable so we can do something shorter, ie. ['#list_type' => 'comma'] as opposed to ['#context' => ['list_style' => 'comma-list']] -- the latter mixes underscores and dashes and sounds very presentation/CSS-specific to be putting in the calling code (which will often be not be written by themers)

@davidhernandez++

this is indented by 1 space only, shouldn't it be 2?

I don't see this?

Also, rather than reuse the context variable isn't it better DX to create a new variable so we can do something shorter, ie. ['#list_type' => 'comma'] as opposed to ['#context' => ['list_style' => 'comma-list']]

But David said that it's designed to work this way:

I was looking to change this to use a variable for the template and realized Cottser is a genius! item_list already has a generic context variable which was needed to customize the search results item_list, so I reused it. Forward thinking reusability win! \o/

So, given that this works in the way that the Twig people designed it to work, I think any discussion regarding improving that design should be done in a follow-up. This now complies with the latest front-end (Twig & CSS) standards, which is what this issue was stuck on.

Hence: RTBC :)

Awesome work everyone!

Fair enough, RTBC++ then, assuming this has been manually tested?

The indentation can be fixed on commit:

+++ b/core/modules/system/templates/item-list.html.twig
@@ -12,12 +12,17 @@
+ * - context: A list of contextual data associated with the list. May contain:
+ *   - list_style: The custom list style.

We've definitely had a good discussion on this one. We've arrived at the best solution wrt to semantics and accessibility. Additionally the item list is a really common template and performance cost of re-using a template already used on the page is minimal so this will be as performant as it can be. Nice work everyone.

Committed c7ca868 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/views_ui/src/Tests/DisplayPathTest.php b/core/modules/views_ui/src/Tests/DisplayPathTest.php
index 62c1f2f..1515b2f 100644
--- a/core/modules/views_ui/src/Tests/DisplayPathTest.php
+++ b/core/modules/views_ui/src/Tests/DisplayPathTest.php
@@ -64,7 +64,6 @@ protected function doBasicPathUITest() {
    * Tests that View paths are properly filtered for XSS.
    */
   public function doPathXssFilterTest() {
-    global $base_path;
     $this->drupalGet('admin/structure/views/view/test_view');
     $this->drupalPostForm(NULL, array(), 'Add Page');
     $this->drupalPostForm('admin/structure/views/nojs/display/test_view/page_2/path', array('path' => '<object>malformed_path</object>'), t('Apply'));

Fixed on commit - global variable unused.

I don't think this has been pushed yet?

Re #177, I just manually tested and it looked great!

Really cool to see how this came together, IMO a great example of how pushing each other to find a better solution can be really fruitful! Thanks all.

Really cool to see how this came together, IMO a great example of how pushing each other to find a better solution can be really fruitful! Thanks all.

Absolutely!

Special thanks to @davidhernandez on this one :)

#context is intended for miscellaneous template things, so putting something a little more frontend friendly in there should be ok. I agree having it separated as its own variable would be better if this was intended for anything else., but the list style isn't data or state or really anything other than template context.

I thought about making the value more consistent with other things we do in the backend, but that would probably mean using a space. And then using the clean_class filter on it in the template. Given how much discussion has gone into the performance, I didn't want to introduce another function call just for this.

I'll make a comment on the issue we have open to move the item list CSS to Classy regarding some of the CSS refactoring.