Replace calls to theme('item_list') with calls to theme('links') for links, when a heading or wrapper div is warranted

I'm guessing a separate issue should be filed for converting the calls to theme() to drupal_render()?

Here's a possible patch.

A quick question though, the css will be converted and '#title' be removed with #1842140: Remove title and wrapper div from item-list.html.twig, right?

Okay... let's give this another shot.

#6: theme-item-list-to-links-2032645-6.patch queued for re-testing.

Apparently we are now escaping a string that we weren't before, and that was throwing off a test.

Here's the version with the test updated.

remove pager

+++ b/core/modules/toolbar/tests/modules/toolbar_test/toolbar_test.module
@@ -29,13 +29,17 @@ function toolbar_test_toolbar() {
+        '#heading' => array(
+          'text' => t('Test tray'),
+          'level' => 'h2',
+          'class' => array('visually-hidden'),
         ),
-        '#prefix' => '<h2 class="visually-hidden">' . t('Test tray') . '</h2>',
         '#attributes' => array(
           'class' => array('menu'),

class as a top level argument, rather than being #attributes, was the only thing that jumped out of me when reviewing this diff.

As a follow-up to this issue, since there already is a @todo in core for it(theme_links(), theme.inc:1706), #header['class'] should be replaced with #header['attributes']['class'].

Re: #4, I do believe the title, wrapper, CSS are supposed to happen in the other issue(s).

jenlampton: I'm not totally understanding the proposed resolution, but it seems the scope of this issue is fully addressed by this latest patch in #10.

If so, +1 to RTBC.

up

It looks like after 6 months of sitting here, the code needs to be re-rolled.

Re-rolling patch from comment 14.

Looks good to me.

This all looks like a great clean-up! One quick thing...

+++ b/core/modules/statistics/lib/Drupal/statistics/Tests/StatisticsReportsTest.php
@@ -51,7 +51,7 @@ function testPopularContentBlock() {
-    $this->assertText("Today's", "Found today's popular content.");
+    $this->assertText(check_plain("Today's"), "Found today's popular content.");

That's a bit weird and afaik should not be necessary? Or at least I can't figure out why it would've passed before and not now.

Since this is part of a title that used to be just spit out as plain HTML, it wasn't checked to see if it was html-ready.

Old output: Today's

New output: Today's

I think the new output is better HTML, but correct me if I'm wrong...

Trying to reroll

Patch rerolled!

rerolled.

Rerolled.

Re Rolled!

Patch applies cleanly.

Drupal\statistics\Tests\StatisticsReportsTest was failing due to fatal error, since the previous patch was calling check_plain, which is now String::checkPlain().

Plenty of tests to fix stil though.

Here is the fixed #35 patch : the theme_links waited for an "url" key and not a "href" key.
I've also fixed the title which display the comment count on each popular node link.

Fixed just a couple of nitpicks. It's RTBC for me.

Toolbar BEFORE

Toolbar AFTER

I am at DrupalCon Barcelona mentored sprint. And will try to do this reroll (together with jnnck).
Documentation for reroll https://www.drupal.org/patch/reroll

We rerolled the patch from #39.
Let's see what the testbot says.

I am reviewing this issue at dc barcelona

I removed a still existing '#title' => NULL and removed the changes in the StatisticReportsTest which breaks the current tests.
As webchick mentioned in #21, I don't think we have to checkplain there.

If we look in the list in the issue summary, I am not sure what todo about the #prefix elements...

This is the correct interdiff

1. +++ b/core/authorize.php
   @@ -146,7 +146,10 @@ function authorize_access_allowed(Request $request) {
          '#theme' => 'item_list',
   

   Shouldn't this be changed to '#theme' => 'links'?

2. +++ b/core/modules/statistics/src/Tests/StatisticsTestBase.php
   @@ -19,7 +19,7 @@
   -  public static $modules = array('node', 'block', 'ban', 'statistics');
   +  public static $modules = array('node', 'block', 'ban', 'statistics', 'comment');
   
   +++ b/core/modules/statistics/statistics.module
   @@ -105,6 +105,11 @@ function statistics_title_list($dbfield, $dbrows) {
   +    if (Drupal::moduleHandler()->moduleExists('comment')) {
   +      $query->leftJoin('comment_entity_statistics', 'ces', 'n.nid = ces.entity_id');
   +      $query->fields('ces', array('comment_count'));
   +    }
   

   These changes were introduced in #38, but I'm not sure they're in scope of this issue?

3. The change to core/includes/theme.maintenance.inc described in the issue summary has not been included in this patch.
4. The change to core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php may no longer be necessary, but we may need to investigate where that was changed and update the issue summary accordingly.
5. The change to core/modules/views/views.theme.inc may no longer be necessary, but we may need to investigate where that was changed and update the issue summary accordingly.
6. The file core/update.phpno longer exists in HEAD, but we may need to investigate where that was changed and update the issue summary accordingly.

Handled the comments above. 8.0.x HEAD has a few other instances for theme_item_list with #title as well.

Uploading the updated patch here.

Patch created with core folder as the base. Uploading the correct patch again.

It's great to see progress on this =)

+++ b/core/authorize.php
@@ -146,7 +146,10 @@ function authorize_access_allowed(Request $request) {
+    $heading = array(
+      'text' => t('Next steps'),
+      'level' => 'h3',
+    );

+++ b/core/modules/node/node.module
@@ -187,16 +187,37 @@ function node_entity_view_display_alter(EntityViewDisplayInterface $display, $co
+  $links = array();
...
+    ) : array();
+    $links[] = array(
+      'title' => $row->title,
+      'url' => new Url('entity.node.canonical', ['node' => $row->nid]),
+      'attributes' => $options,
+    );
...
+    $heading = array(
+      'text' => $title,
+      'level' => 'h3',
+    );
+    return array(
+      '#theme' => 'links__node',
+      '#links' => $links,
+      '#heading' => $heading
+    );

Let's do these in short array syntax.

Did the array() that I could, removed the return that was left in there and this should be good for a review.

+++ b/core/modules/node/node.module
@@ -186,19 +186,37 @@ function node_entity_view_display_alter(EntityViewDisplayInterface $display, $co
+  if ($has_rows) {
+    $heading = [
+      'text' => $title,
+      'level' => 'h3',
+    ];
+    return [
+      '#theme' => 'links__node',
+      '#links' => $links,
+      '#heading' => $heading,
+    ];
+  }
+  else {
+    return FALSE;

Bit of a non-issue, but we could avoid using if else here by returning FALSE early if !$has_rows... makes for easier reading

Other than this, interdiff and patch looks good to me. Good catch @joelpittet on removing that leftover return btw.

Next step, fix the failing tests :)

Addressing #69