Problem/Motivation

Follow-up to #2329505-17: Convert theme_task_list to Twig template

Proposed resolution

Decide on name.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Manually test the patch Novice Instructions

User interface changes

n/a

API changes

The task_list theme hook has been changed to maintenance_task_list.

Before

$task_list = array(
  '#theme' => 'task_list',
  '#items' => $tasks,
  '#active' => $active,
);

After

$task_list = array(
  '#theme' => 'maintenance_task_list',
  '#items' => $tasks,
  '#active' => $active,
);

Comments

Cottser’s picture

joshi.rohit100’s picture

Status: Active » Needs review
FileSize
1.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,970 pass(es). View

Patch uploaded. Please review.

dawehner’s picture

https://www.drupal.org/documentation/git/configure has some good example how to configure git to show renames in diffs.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing

Thanks for the patch @joshi.rohit100! Apparently we don't have any test coverage for this, drupal_common_theme() needs to be updated…

joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
2.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,071 pass(es). View

updated patch.

Cottser’s picture

Status: Needs review » Needs work

Sorry, I should have been clearer. The actual theme hook, task_list (and usages of it) should be renamed. We don't usually have the templates named differently from the theme hook.

LewisNyman’s picture

I'm not sure of the logic behind this change? From a theming point of view, why specify that a task list should only be used for maintenance tasks? Couldn't they be used in other situations that we have not yet uncovered? It doesn't fit with the idea of "reusable design components irrespective of content" that we have embraced in our CSS standards.

Cottser’s picture

I think the ideal end result is #1222254: Remove theme_task_list() and call theme('item_list__tasks') instead., but if you look at the template it has a hardcoded "Installation tasks" right in there. So definitely not reusable right now, whatever it's called :)

tuutti’s picture

Status: Needs work » Needs review
FileSize
2.65 KB
4.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,183 pass(es). View
thamas’s picture

Issue summary: View changes
thamas’s picture

Issue tags: +Amsterdam2014
Cottser’s picture

Status: Needs review » Needs work
+++ b/core/includes/install.core.inc
@@ -766,7 +766,7 @@ function install_tasks($install_state) {
- * theme_task_list().
+ * theme_maintenance_task_list().

@@ -775,7 +775,7 @@ function install_tasks($install_state) {
- * @see theme_task_list()
+ * @see theme_maintenance_task_list()

While we're updating these we might as well change them to maintenance-task-list.html.twig because neither of these theme functions exist, pre- or post-patch.

Other than that this is looking good! Thanks @tuutti (and @thamas!).

jjcarrion’s picture

Assigned: Unassigned » jjcarrion

I'll try this one

jjcarrion’s picture

Assigned: jjcarrion » Unassigned

Finally I hadn't time yesterday to see this, I left it for the sprint and I'll take it again if nobody resolve it.

rootwork’s picture

FileSize
3.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,872 pass(es). View
920 bytes

Updated per #12.

rootwork’s picture

Status: Needs work » Needs review
pieterjd’s picture

As a novice at the drupalcon Amsterdam sprint workshop, I have applied the patch.

  • The output of git status shows the file has correctly been renamed.
  • The changes in core/includes/install.core.inc, core/includes/theme.inc and core/modules/system/src/Controller/DbUpdateController.php now reference the new filename/template
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -Needs manual testing

thank you @pieterjd and @rootwork.

Cottser’s picture

Issue summary: View changes

Adding the API change to the issue summary. Thanks everyone!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 63a2332 and pushed to 8.0.x. Thanks!

diff --git a/core/includes/theme.inc b/core/includes/theme.inc
index 681a27e..ef49f54 100644
--- a/core/includes/theme.inc
+++ b/core/includes/theme.inc
@@ -1544,7 +1544,7 @@ function template_preprocess_container(&$variables) {
 /**
  * Prepares variables for maintenance task list templates.
  *
- * Default template: task-list.html.twig.
+ * Default template: maintenance-task-list.html.twig.
  *
  * @param array $variables
  *   An associative array containing:

Fixed on commit.

  • alexpott committed 63a2332 on 8.0.x
    Issue #2335003 by joshi.rohit100, rootwork, tuutti | joelpittet: Rename...
Cottser’s picture

Thank you @alexpott!

LewisNyman’s picture

Status: Fixed » Needs work
FileSize
519.45 KB

I'm not sure if it was this issue, but it seems suspect that HEAD is looking like this:

Cottser’s picture

Not at a computer right now to check but I suspect that is #1842140: Remove title and wrapper div from item-list.html.twig.

joelpittet’s picture

I'd suspect the issue Cottser mentioned as well.

LewisNyman’s picture

Status: Needs work » Fixed

Ah I see, thanks

Status: Fixed » Closed (fixed)

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