Problem/Motivation

ViewListBuilder::buildRow() 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
  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.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

Manual testing steps (for XSS and double escaping)

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

  1. Clean install of Drupal 8.
  2. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  3. If there is any user or calling code input in the string, submit
    alert('XSS');

    and ensure that it is sanitized.

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#174 interdiff-2505931-170to174.txt4.33 KBdavidhernandez
#174 remove_safemarkup_set-2505931-174.patch9.8 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,655 pass(es). View
#170 interdiff-163-170.txt1.26 KBstefan.r
#170 2505931-170.patch7.18 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,655 pass(es). View
#163 rtl.png21.96 KBstefan.r
#163 2505931-163.patch7.26 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,649 pass(es). View
#163 interdiff-162-163.txt866 bytesstefan.r
#162 2505931-162.patch7.05 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,654 pass(es). View
#162 interdiff-159-162.txt450 bytesstefan.r
#161 list-with-patch.png32.53 KBstefan.r
#161 list-without-patch.png33.36 KBstefan.r
#160 interdiff-93-159.txt5.43 KBstefan.r
#160 2505931-159.patch7.05 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,631 pass(es). View
#158 2505931-158.patch6.92 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,638 pass(es). View
#158 interdiff-93-158.txt5.3 KBstefan.r
#147 2505931-145.patch9.21 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,238 pass(es). View
#147 interdiff-144-145.txt849 bytesstefan.r
#144 2505931-144.patch9.21 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,237 pass(es), 1 fail(s), and 0 exception(s). View
#144 interdiff-143-144.txt458 bytesstefan.r
#143 2505931-143.patch9.22 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,247 pass(es), 1 fail(s), and 0 exception(s). View
#143 interdiff-93-143.txt5.11 KBstefan.r
#134 interdiff-93-134.txt5.82 KBstefan.r
#134 2505931-134.patch8.7 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,249 pass(es). View
#129 2505931-127.patch11.23 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,241 pass(es), 2 fail(s), and 0 exception(s). View
#129 interdiff-93-127.txt1.22 KBstefan.r
#125 test.php_.txt1.18 KBstefan.r
#124 remove_safemarkup_set-2505931-93.patch11.21 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,250 pass(es). View
#112 2505931-111.patch9.74 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,241 pass(es). View
#112 interdiff-107-111.txt1.17 KBstefan.r
#110 2505931-107.patch10.7 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,218 pass(es). View
#110 interdiff-93-107.patch5.54 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch interdiff-93-107.patch. Unable to apply patch. See the log in the details link for more information. View
#93 remove_safemarkup_set-2505931-93.patch11.21 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,964 pass(es). View
#93 interdiff.txt2.12 KBcilefen
#88 interdiff-88-87.txt1.17 KBakalata
#88 2505931-88.patch11.2 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,279 pass(es). View
#87 interdiff-85-87.txt1.17 KBakalata
#87 2505931-87.patch11.22 KBakalata
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,237 pass(es), 5 fail(s), and 0 exception(s). View
#85 interdiff-83-85.txt2.24 KBakalata
#85 2505931-85.patch11.26 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,285 pass(es). View
#83 interdiff-81-83.txt7.53 KBakalata
#83 2505931-83.patch11.07 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,283 pass(es). View
#82 interdiff-81-82.txt5.1 KBakalata
#82 2505931-82.patch8.03 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,286 pass(es). View
#81 interdiff-2505931-78-81.txt1.69 KBakalata
#81 2505931-81.patch12.49 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,249 pass(es). View
#78 interdiff-77-78.txt1007 bytesakalata
#78 2505931-78.patch12.08 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,260 pass(es). View
#77 interdiff-70-77.txt6.58 KBakalata
#77 2505931-77.patch12.22 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,238 pass(es). View
#75 interdiff-70-75.txt13.52 KBakalata
#75 2505931-75.patch18.62 KBakalata
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2505931-75.patch. Unable to apply patch. See the log in the details link for more information. View
#74 multiple_field_settings.png57.01 KBakalata
#70 interdiff-68-70.txt478 bytesakalata
#70 remove_safemarkup_set-2505931-70.patch8.88 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,220 pass(es). View
#68 override-template.png8.33 KBakalata
#68 system-template-update.png7.23 KBakalata
#68 system-template.png13.48 KBakalata
#68 seven-template.png13.06 KBakalata
#68 interdiff.txt2.09 KBakalata
#68 remove_safemarkup_set-2505931-68.patch8.89 KBakalata
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,221 pass(es), 9 fail(s), and 0 exception(s). View

Comments

cilefen’s picture

Issue summary: View changes
cilefen’s picture

I am working on this.

cilefen’s picture

Status: Active » Needs review
FileSize
2.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,843 pass(es). View

Thank you to @pwolanin for describing the technique.

pwolanin’s picture

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

cilefen’s picture

colbol’s picture

FileSize
660 bytes
2.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,923 pass(es). View
pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

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.

cilefen’s picture

FileSize
2.67 KB
4.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,990 pass(es). View
cilefen’s picture

+++ b/core/modules/views_ui/tests/src/Unit/ViewListBuilderTest.php
@@ -7,6 +7,7 @@
+use Drupal\Component\Utility\SafeMarkup;

We do not need this use. I had tested against a call to SafeMarkup::escape() but I feel it is better to not rely on the same function for both the escaping and the assert.

cilefen’s picture

FileSize
497 bytes
4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,908 pass(es). View
YesCT’s picture

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?

YesCT’s picture

FileSize
1.13 KB
4.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,885 pass(es), 17 fail(s), and 5 exception(s). View
1.09 KB
2.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,981 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 13: 2505931.13.headtest.donotcommit.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Component/Utility/Xss.php. View
1.62 KB

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

Status: Needs review » Needs work

The last submitted patch, 15: 2505931-11-and-fail.patch, failed testing.

The last submitted patch, 13: 2505931.13.donotcommit.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,906 pass(es), 1 fail(s), and 0 exception(s). View
887 bytes

oops, silly me - fat finger in the editor.

Status: Needs review » Needs work

The last submitted patch, 18: 2505931-11-and-fail-really.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 98,094 pass(es). View

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

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

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.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

Yes please. :)

pwolanin’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
427.06 KB
48.68 KB
337.44 KB

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

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Needs manual testing

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.

cilefen’s picture

@xjm I realize the wisdom in this. It's a list builder.

pwolanin’s picture

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

xjm’s picture

Issue tags: -Needs manual testing

Okay, just automated tests then. :)

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

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

xjm’s picture

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

cilefen’s picture

FileSize
1.56 KB
5.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 98,121 pass(es), 1 fail(s), and 0 exception(s). View

The tests.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: remove_safemarkup_set-2505931-30.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
5.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 98,115 pass(es), 1 fail(s), and 0 exception(s). View

Whoops - I had some other test methods commented-out when I tested locally and there was an ordering problem.

cilefen’s picture

Whoops - I had some other test methods commented-out when I tested locally and there was an ordering problem.

dawehner’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 32: remove_safemarkup_set-2505931-32.patch, failed testing.

cilefen’s picture

#32 works for me locally. Advice is welcome.

cilefen’s picture

Re #36, the reason is simple - the expected output contains a site subpath.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
5.72 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,135 pass(es). View
davidhernandez’s picture

Can we get with and without test patches?

cilefen’s picture

FileSize
4.47 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,170 pass(es), 1 fail(s), and 0 exception(s). View
5.72 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,313 pass(es). View

The last submitted patch, 40: remove_safemarkup_set-2505931-38-TESTS.patch, failed testing.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @cilefen! Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.1 KB
5.39 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,696 pass(es). View
  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.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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

dawehner’s picture

+1

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
5.61 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,707 pass(es). View

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

xjm’s picture

Status: Needs review » Needs work

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
6.05 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,861 pass(es). View

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.

alexpott’s picture

Status: Needs review » Needs work
  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.

alexpott’s picture

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

cilefen’s picture

@alexpott is #51 on the correct issue?

larowlan’s picture

Assigned: Unassigned » larowlan

working on this

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
FileSize
3.68 KB
6.49 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,360 pass(es). View

addresses #50

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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

joelpittet’s picture

Also thanks @larowlan for figuring that out.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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?

joelpittet’s picture

FileSize
9.41 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,495 pass(es), 1 fail(s), and 0 exception(s). View

@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.

Status: Needs review » Needs work

The last submitted patch, 58: remove_safemarkup_set-2505931-58.patch, failed testing.

joelpittet’s picture

+++ 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.

lauriii’s picture

+++ 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

joelpittet’s picture

@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.

akalata’s picture

akalata’s picture

Status: Needs work » Needs review
FileSize
9.39 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,573 pass(es). View
728 bytes

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

akalata’s picture

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.

davidhernandez’s picture

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

davidhernandez’s picture

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

akalata’s picture

Issue summary: View changes
FileSize
8.89 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,221 pass(es), 9 fail(s), and 0 exception(s). View
2.09 KB
13.06 KB
13.48 KB
7.23 KB
8.33 KB

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.

Status: Needs review » Needs work

The last submitted patch, 68: remove_safemarkup_set-2505931-68.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
8.88 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,220 pass(es). View
478 bytes
alexpott’s picture

  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?

akalata’s picture

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.

joelpittet’s picture

@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

akalata’s picture

FileSize
57.01 KB

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.
akalata’s picture

Issue tags: -Needs subsystem maintainer review +Needs tests
FileSize
18.62 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2505931-75.patch. Unable to apply patch. See the log in the details link for more information. View
13.52 KB

Posting progress on implementation switch before working on tests.

Status: Needs review » Needs work

The last submitted patch, 75: 2505931-75.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
12.22 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,238 pass(es). View
6.58 KB

Let's try this. Ignoring #75.

akalata’s picture

FileSize
12.08 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,260 pass(es). View
1007 bytes

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
joelpittet’s picture

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!

dawehner’s picture

  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

akalata’s picture

FileSize
12.49 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,249 pass(es). View
1.69 KB

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.

akalata’s picture

FileSize
8.03 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,286 pass(es). View
5.1 KB

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.

akalata’s picture

Issue tags: -Needs tests
FileSize
11.07 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,283 pass(es). View
7.53 KB

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

joelpittet’s picture

Status: Needs review » Needs work
  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.

akalata’s picture

Status: Needs work » Needs review
FileSize
11.26 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,285 pass(es). View
2.24 KB

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

joelpittet’s picture

Status: Needs review » Needs work
  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

akalata’s picture

Status: Needs work » Needs review
FileSize
11.22 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,237 pass(es), 5 fail(s), and 0 exception(s). View
1.17 KB

Thanks for the continued reviews!

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

akalata’s picture

FileSize
11.2 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,279 pass(es). View
1.17 KB

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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).

The last submitted patch, 87: 2505931-87.patch, failed testing.

dawehner’s picture

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

alexpott’s picture

+++ 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?

cilefen’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.12 KB
11.21 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,964 pass(es). View
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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

YesCT’s picture

+++ 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.

Cottser’s picture

Looks like a nice solution, thanks all.

alexpott’s picture

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.

dawehner’s picture

Tripple post! Nice

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

Wim Leers’s picture

Assigned: Unassigned » Fabianx
Status: Reviewed & tested by the community » Needs review

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.

alexpott’s picture

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.

Cottser’s picture

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.

alexpott’s picture

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

joelpittet’s picture

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

Cottser’s picture

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.

Wim Leers’s picture

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?

joelpittet’s picture

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.

joelpittet’s picture

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

stefan.r’s picture

FileSize
5.54 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch interdiff-93-107.patch. Unable to apply patch. See the log in the details link for more information. View
10.7 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,218 pass(es). View

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?

The last submitted patch, 110: interdiff-93-107.patch, failed testing.

stefan.r’s picture

FileSize
1.17 KB
9.74 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,241 pass(es). View

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

stefan.r’s picture

+++ 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?

alexpott’s picture

re #113 nope we don't.

alexpott’s picture

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.

dawehner’s picture

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

stefan.r’s picture

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?

alexpott’s picture

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

Wim Leers’s picture

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

stefan.r’s picture

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

xjm’s picture

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

dawehner’s picture

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.

xjm’s picture

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.

stefan.r’s picture

FileSize
11.21 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,250 pass(es). View

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

stefan.r’s picture

FileSize
1.18 KB

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Alright — sorry for the distraction!

Cottser’s picture

Thanks, all.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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?

stefan.r’s picture

FileSize
1.22 KB
11.23 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,241 pass(es), 2 fail(s), and 0 exception(s). View

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.

stefan.r’s picture

Status: Needs work » Needs review
stefan.r’s picture

Assigned: Fabianx » Unassigned
Status: Needs review » Needs work

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

The last submitted patch, 129: 2505931-127.patch, failed testing.

alexpott’s picture

+++ 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?

stefan.r’s picture

Status: Needs work » Needs review
FileSize
8.7 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,249 pass(es). View
5.82 KB
joelpittet’s picture

Status: Needs review » Needs work

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

joelpittet’s picture

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

stefan.r’s picture

Status: Needs work » Needs review

@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.

joelpittet’s picture

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

joelpittet’s picture

Status: Needs review » Needs work

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.

xjm’s picture

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.

joelpittet’s picture

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?

stefan.r’s picture

Let's!

stefan.r’s picture

Status: Needs work » Needs review
FileSize
5.11 KB
9.22 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,247 pass(es), 1 fail(s), and 0 exception(s). View
stefan.r’s picture

FileSize
458 bytes
9.21 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,237 pass(es), 1 fail(s), and 0 exception(s). View

The last submitted patch, 143: 2505931-143.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 144: 2505931-144.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
849 bytes
9.21 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,238 pass(es). View
stefan.r’s picture

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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

Cottser’s picture

Yup, I'm good with that too.

alexpott’s picture

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.

catch’s picture

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’s picture

Status: Reviewed & tested by the community » Needs review
catch’s picture

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.

stefan.r’s picture

+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.

Wim Leers’s picture

+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...

stefan.r’s picture

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?

stefan.r’s picture

Issue tags: +Needs manual testing
FileSize
5.3 KB
6.92 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,638 pass(es). View
Wim Leers’s picture

Issue tags: +CSS

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

stefan.r’s picture

FileSize
7.05 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,631 pass(es). View
5.43 KB
stefan.r’s picture

Issue summary: View changes
Issue tags: -Needs manual testing
FileSize
33.36 KB
32.53 KB

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:

stefan.r’s picture

FileSize
450 bytes
7.05 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,654 pass(es). View

Removing units from margin

stefan.r’s picture

FileSize
866 bytes
7.26 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,649 pass(es). View
21.96 KB

Added RTL support:
rtl

Wim Leers’s picture

+++ 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?

davidhernandez’s picture

+++ 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.

stefan.r’s picture

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

xjm’s picture

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.

Wim Leers’s picture

Status: Needs review » Needs work
+++ 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.

stefan.r’s picture

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

stefan.r’s picture

Status: Needs work » Needs review
FileSize
7.18 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,655 pass(es). View
1.26 KB

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.

davidhernandez’s picture

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.

stefan.r’s picture

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

davidhernandez’s picture

Assigned: Unassigned » davidhernandez
Status: Needs review » Needs work

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.

davidhernandez’s picture

Assigned: davidhernandez » Unassigned
Status: Needs work » Needs review
FileSize
9.8 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,655 pass(es). View
4.33 KB

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.

stefan.r’s picture

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

@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!

stefan.r’s picture

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.
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

stefan.r’s picture

I don't think this has been pushed yet?

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

  • alexpott committed c7ca868 on 8.0.x
    Issue #2505931 by stefan.r, akalata, cilefen, pwolanin, joelpittet,...
Cottser’s picture

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.

Wim Leers’s picture

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

davidhernandez’s picture

#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.

Status: Fixed » Closed (fixed)

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