Files: 

Comments

Cottser’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

Title: system.module - Convert PHPTemplate templates to Twig » system.module - Convert theme_ functions to Twig

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions rather than templates. See #1898454: system.module - Convert PHPTemplate templates to Twig for template conversion.

c4rl’s picture

Issue summary: View changes

Updated issue summary.

gnuget’s picture

Status: Active » Needs review
FileSize
35.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system_theme_functions_to_Twig-1987410-1.patch. Unable to apply patch. See the log in the details link for more information. View

Patch attached

minneapolisdan’s picture

Status: Needs review » Needs work
Issue tags: +Needs profiling, +Twig

The last submitted patch, system_theme_functions_to_Twig-1987410-1.patch, failed testing.

gnuget’s picture

Assigned: Unassigned » gnuget

This patchs needs a re-roll i guess... i'm going to work on it.

gnuget’s picture

Assigned: gnuget » Unassigned
Status: Needs work » Needs review
FileSize
35.1 KB
PASSED: [[SimpleTest]]: [MySQL] 55,274 pass(es). View

Here a rerolled version.

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Thanks @gnuget.

Here is a few nitpiks.

+++ b/core/modules/system/templates/admin-block-content.html.twig
@@ -0,0 +1,29 @@
+ * Available variables:
+ * - content: An array containing information about the block. Each element
+ *   of the array represents an administrative menu item, and must at least
+ *   contain the keys 'title', 'href', and 'localized_options', which are

Array shouldn't be in the description, change to 'A list'

+++ b/core/modules/system/templates/admin-block.html.twig
@@ -0,0 +1,26 @@
+{% if block.show %}
+<div class="admin-panel">
+  {% if block.title %}<h3>{{ block.title }}</h3>{% endif %}
+  {% if block.content %}<div class="body">{{ block.content }}</div>
+  {% elseif block.description %}<div class="description">{{ block.description }}</div>{% endif %}
+</div>

Indenting on this should be in line with other twig indentation.
@see http://twig.sensiolabs.org/doc/coding_standards.html

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -0,0 +1,58 @@
+ * Each theme_groups[group] contains an array of theme groups.
+ *
+ * Each group in theme_groups[group] contains:
+ * - group.attributes: Element attributes specific to this group.
+ * - group.title: Title for the theme group.
+ * - group.state: State of the theme group.
+ * - group.themes: An array of themes within that group.
+ *
+ * Each group.themes[theme] contains an array of themes.
+ *
+ * Each theme in group.themes[theme] contains:
+ * - theme.attributes: Element attributes specific to this theme.
+ * - theme.screenshot: Render of theme screenshot.
+ * - theme.description: Description of the theme.
+ * - theme.name: Name of theme.
+ * - theme.version: Verions number of theme.
+ * - theme.notes: Identifies what context this theme is being used.
+ *   eg. (default theme, admin theme)
+ * - theme.compatibility: Description of any incompatibility issues,
+ *   if the theme is compatible, provides a list of links.

These sub items should be indented without the name.
ie.
- attributes
not group.attributes.
@see https://drupal.org/node/1823416

IshaDakota’s picture

Status: Needs work » Needs review
FileSize
3.39 KB
34.67 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.module. View

Re-roll and nitpicks from #7

interdiff from after re-roll.

Status: Needs review » Needs work

The last submitted patch, system_theme_functions_to_Twig-1987410-8.patch, failed testing.

IshaDakota’s picture

Status: Needs work » Needs review

botched that re-roll. Attached re-roll only to test before adding the "nitpicks."

IshaDakota’s picture

FileSize
36.11 KB
PASSED: [[SimpleTest]]: [MySQL] 57,395 pass(es). View

with patch :)

joelpittet’s picture

Thank you @IshaDakota:)
FYI, there is a bunch more nitpicks and performance improvements on this patch as it's fairly big. Mostly to do with attributes but I'll wait till the patch turns green before I write them up.

IshaDakota’s picture

OK @joelpittet - I'll wait for a write up before submitting a new patch.

joelpittet’s picture

Ok here they come:) @IshaDakota if you have any questions I should be around on IRC (#drupal-twig) if you have any questions or leave the questions here.

+++ b/core/modules/system/system.admin.inc
@@ -11,6 +11,7 @@
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 use Drupal\Core\Datetime\DrupalDateTime;
+use Drupal\Core\Template\Attribute;
 use Drupal\system\Form\ModulesInstallConfirmForm;

This is a task in itself but we don't need to instantiate Attribute for anything in this patch because they should all be lazy loaded through #1982024: Lazy-load Attribute objects later in the rendering process only if needed

This would mean removing all the new Attribute() calls form this patch and leaving them as arrays.

+++ b/core/modules/system/system.module
@@ -152,46 +157,49 @@ function system_theme() {
     ),
     'admin_page' => array(
-      'variables' => array('blocks' => NULL),
+      'variables' => array('blocks' => array()),

None of the variables in system_theme() need to be converted to an empty array from NULL. I have a good feeling that was a twig bug that was fixed here: #1975462: Allow and test for NULL and integer 0 values in Twig templates.

+++ b/core/modules/system/system.module
@@ -3515,30 +3523,38 @@ function system_timezone($abbreviation = '', $offset = -1, $is_daylight_saving_t
 
 /**
- * Returns HTML for the Powered by Drupal text.
+ * Prepares variables for the Powered by Drupal template.
+ *
+ * Default template: system-powered-by.html.twig.
+ *
+ * This doesn't do anything extra to prepare the template, but is given simply
+ * for example purposes.
  *
  * @ingroup themeable
  */
-function theme_system_powered_by() {
-  return '<span>' . t('Powered by <a href="@poweredby">Drupal</a>', array('@poweredby' => 'http://drupal.org')) . '</span>';
+function template_preprocess_system_powered_by(&$variables) {
 }

if this is an empty preprocess function, it can be deleted.

+++ b/core/modules/system/templates/admin-block-content.html.twig
@@ -0,0 +1,29 @@
+{% if content %}
+<dl{{ attributes }}>
+  {% for item in content %}
+    <dt>{{ item.link }}</dt>
+    {% if item.description %}
+      <dd>{{ item.description }}</dd>
+    {% endif %}
+  {% endfor %}
+</dl>

dl needs one more indent.

+++ b/core/modules/system/templates/admin-block.html.twig
@@ -0,0 +1,26 @@
+{% if block.show %}
+<div class="admin-panel">
+  {% if block.title %}<h3>{{ block.title }}</h3>{% endif %}
+  {% if block.content %}<div class="body">{{ block.content }}</div>
+  {% elseif block.description %}<div class="description">{{ block.description }}</div>{% endif %}
+</div>

contents of if block nees one more indent and if statements shouldn't be on oneline.

+++ b/core/modules/system/templates/admin-page.html.twig
@@ -0,0 +1,25 @@
+ * - containers: An array of administrative blocks keyed by position: left or right.

80 charachters and 'An array of' should be 'A list of'.

+++ b/core/modules/system/templates/status-report.html.twig
@@ -0,0 +1,40 @@
+ * - requirements: contains multiple requirement instances. Each requirement
+ *     has .title, .value and .description.
+ * - severity: The severity of error. Contains .title and .class.

the indent and formatting of the requirements properties is incorrect. @see https://drupal.org/node/1823416

+++ b/core/modules/system/templates/system-admin-index.html.twig
@@ -0,0 +1,26 @@
+ @todo: remove this file once http://drupal.org/node/1842232 is resolved.

Remove the todo, the issue should be good enough. We can't commit with @todos.

+++ b/core/modules/system/templates/system-modules-details.html.twig
@@ -0,0 +1,15 @@
+ * - content: file form element html.

Capital F for the sentence.

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -0,0 +1,58 @@
+ * Each theme in group.themes[theme] contains:
+ * - theme.attributes: Element attributes specific to this theme.
+ * - theme.screenshot: Render of theme screenshot.
+ * - theme.description: Description of the theme.
+ * - theme.name: Name of theme.
+ * - theme.version: Verions number of theme.
+ * - theme.notes: Identifies what context this theme is being used.
+ *   eg. (default theme, admin theme)

The main item 'group' and 'theme' and all their sub items need to be formatted according to the twig standards here: https://drupal.org/node/1823416

IshaDakota’s picture

Assigned: Unassigned » IshaDakota
Status: Needs review » Needs work
IshaDakota’s picture

Assigned: IshaDakota » Unassigned
Status: Needs work » Needs review
FileSize
11.78 KB
35.07 KB
FAILED: [[SimpleTest]]: [MySQL] 57,565 pass(es), 0 fail(s), and 424 exception(s). View

OK, I think maybe I got everything from #7 and #14, thanks to @joelpittet in IRC.

Status: Needs review » Needs work

The last submitted patch, system_theme_functions_to_Twig-1987410-16.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
34.62 KB
FAILED: [[SimpleTest]]: [MySQL] 52,747 pass(es), 143 fail(s), and 59 exception(s). View
7.75 KB

Thank you @IshaDakota!

There is still quite a bit to do on this patch as I skimmed through so we need a fresh pair of eyes I bet.

I was wrong on some of those array() to NULL. Here's a patch that inches this a bit further.

Issue tags: +Novice, +Needs profiling, +Twig

The last submitted patch, 1987410-18-twig-system-theme.patch, failed testing.

Status: Needs review » Needs work
Issue tags: -Novice, -Needs profiling, -Twig

The last submitted patch, 1987410-18-twig-system-theme.patch, failed testing.

joelpittet’s picture

FileSize
4.86 KB
34.65 KB
PASSED: [[SimpleTest]]: [MySQL] 57,287 pass(es). View

Ok @IshaDakota! I was a little heavy handed on the Attributes note but it's still much better (even though it's red:)

This may still not pass, but again, I promise it's in the correct direction.

So the Attributes here that I had to leave in as Attribute objects are not $variables['attributes'] that you changed to arrays. Those ones are cool, and also not $whatever['#attributes'] which is a renderable array that will get themed. It's only the ones that are $whatever['attributes]. It's subtle, confusing and important:)

@IshaDakota, checkout how I cleaned up the twig files with those |keys nonesense and split out attributes.class. There are a number of other templates that need the same cleanup. And whatever errors I left, if any:)

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: +@todo clean-up

activate testbot! and tag, also added a few new @todos that need some filling out in the twig file.

joelpittet’s picture

Status: Needs review » Needs work

See #21 for notes on what further needs doing.

IshaDakota’s picture

FileSize
915 bytes
34.43 KB
PASSED: [[SimpleTest]]: [MySQL] 58,336 pass(es). View

...and re-rolled.

-function system_admin_index() removed in #1987812: Convert system_admin_index() to a new style controller
-div container and class added to "no-screenshot" in #1861702: Add a responsive grid to the Appearance page. Resolved as such:

---- a/core/modules/system/templates/system-themes-page.html.twig
+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -43,7 +43,9 @@
           {% if theme.screenshot %}
             {{ theme.screenshot }}
           {% else %}
-            <div class="no-screenshot">{{ "no screenshot"|t }}</div>
+            <div class="no-screenshot">
+              <div class="no-screenshot__text">{{ "no screenshot"|t }}</div>
+            </div>
           {% endif %}
           <div class="theme-info">
             <h3>{{ theme.name }} {{ theme.version }} {{ theme.notes }}</h3>
IshaDakota’s picture

Status: Needs work » Needs review
IshaDakota’s picture

@joelpittet: Is this what you had in mind for for cleaning up the twig files? These were the only two cases I could find (in addition to the 2 you fixed).

--- a/core/modules/system/templates/admin-page.html.twig
+++ b/core/modules/system/templates/admin-page.html.twig
@@ -18,8 +18,8 @@
   {{ system_compact_link }}
   {% for position, container in containers %}
     <div class="{{ position }} clearfix">
-      {% for i in container.blocks|keys %}
-        {{ container.blocks[i] }}
+      {% for container.block in container.blocks %}
+        {{ container.block }}
       {% endfor %}
     </div>
   {% endfor %}

and

--- a/core/modules/system/templates/system-admin-index.html.twig
+++ b/core/modules/system/templates/system-admin-index.html.twig
@@ -17,8 +17,8 @@
   {{ system_compact_link }}
   {% for position, container in containers %}
     <div class="{{ position }} clearfix">
-      {% for i in container.blocks|keys %}
-        {{ container.blocks[i] }}
+      {% for container.block in container.blocks %}
+        {{ container.block }}
       {% endfor %}
     </div>
   {% endfor %}
joelpittet’s picture

@IshaDakota Very close and in the right direction.

The first variable in the {% for ... is something you 'make up'. While container.block may work it's a bit confusing, the thing I was thinking of making may also be a bit confusing.

+      {% for block in container.blocks %}
+        {{ block.block }}

Which reads: For each block in container's blocks... print block's block.
The reason this is confusing, IMO, isn't my change but the property name block. So I'd prefer:

+      {% for block in container.blocks %}
+        {{ block.content }}

Which reads: For each block in container's blocks... print block's content.

This would mean changing a variable in the preprocces some place but by the looks of things you should be able to get away with this:

+      {% for block in container.blocks %}
+        {{ block }}

Which would be even better, so maybe give that a try and see if the admin page still does what it should.

IshaDakota’s picture

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

OK. I'll test that out.

IshaDakota’s picture

FileSize
308.42 KB
306.95 KB

The following seems to work in both the admin and admin index pages (see screenshots):

<div class="admin clearfix">
  {{ system_compact_link }}
  {% for position, container in containers %}
    <div class="{{ position }} clearfix">
      {% for block in container.blocks %}
        {{ block }}
      {% endfor %}
    </div>
  {% endfor %}
</div>

Admin Index Page:
admin-index.png

Admin Config Page:
admin-page.png

I'll check out those @todos in the twig files.

joelpittet’s picture

Issue tags: +Needs manual testing

Great news @IshaDakota! Could you add the patch with that up?

This patch is getting pretty close, would like to get another set of eyes to review after that patch. Then we can move on to manual testing and profiling. (pre-emptive tagging.)

IshaDakota’s picture

Assigned: IshaDakota » Unassigned
Status: Needs work » Needs review
FileSize
1.18 KB
34.4 KB
PASSED: [[SimpleTest]]: [MySQL] 57,633 pass(es). View

This is is the patch with #29.

There are still @todos in the status-report.html.twig file. Are those just to fill out the descriptions in the docblock? Or does it refer to the code? To me it looks OK.

Status: Needs review » Needs work
Issue tags: -Novice, -Needs manual testing, -Needs profiling, -Twig, -@todo clean-up

The last submitted patch, 1987410-31-twig-system-theme.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs manual testing, +Needs profiling, +Twig, +@todo clean-up

#31: 1987410-31-twig-system-theme.patch queued for re-testing.

joelpittet’s picture

@IshaDakota, thank you and you are right, yeah, just to fill out the descriptions of what those variable are.

Below are another round of nitpikery:

+++ b/core/modules/system/system.admin.inc
@@ -1354,100 +1335,86 @@ function theme_admin_block_content($variables) {
+ * @todo Remove this once http://drupal.org/node/1842232 is resolved.

Remove @todo, the issue it self can deal with it's removal

+++ b/core/modules/system/system.module
@@ -3785,6 +3788,7 @@ function theme_system_config_form($variables) {
  * @ingroup themeable
+ * @todo Move this out of system.module and back into theme.inc.

This should have it's own issue and not be a @todo. (may already exist, have a search before creating a new issue)

+++ b/core/modules/system/templates/admin-block-content.html.twig
@@ -0,0 +1,29 @@
+ *   passed to l(). A 'description' key may also be provided.
+ * - attributes: Remaining HTML attributes to be aded to the element.

Remove 'Remaining '.

+++ b/core/modules/system/templates/admin-block.html.twig
@@ -0,0 +1,31 @@
+ * - block: An array of information about the block, including:
+ *   - show: A boolean flag indicating if the block should be displayed.
+ *   - title: The block title.

Should sa 'A list' and not refernce 'An array' @see https://drupal.org/node/1823416#datatypes

+++ b/core/modules/system/templates/status-report.html.twig
@@ -0,0 +1,46 @@
+ *   Each requirement contains:
+ *   - title: @todo.
+ *   - value: @todo.
+ *   - description: @todo.
+ * - severity: The severity of error.
+ *   Contains:
+ *   - title: @todo.
+ *   - class: @todo.

These todo's need to be filed out describing what these variables contain.

+++ b/core/modules/system/templates/system-admin-index.html.twig
@@ -0,0 +1,25 @@
+ * - system_compact_link: Themed link to toggle compact view.
+ * - container: Container for admin blocks.

Need to add 'position' and 'block' to the available variables here.

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -0,0 +1,60 @@
+ * - attributes: HTML element attributes.
+ * - theme_groups: An array of theme groups.
...
+ * - state: State of the theme group.
+ * - themes: An array of themes within that group.

Data types in twig dockblocks again @see https://drupal.org/node/1823416#datatypes

thedavidmeister’s picture

Status: Needs review » Needs work
eromero1’s picture

Assigned: Unassigned » eromero1

dibs

sbudker1’s picture

Assigned: eromero1 » sbudker1

dibs!

sbudker1’s picture

FileSize
44.34 KB
FAILED: [[SimpleTest]]: [MySQL] 58,014 pass(es), 0 fail(s), and 24 exception(s). View

first patch failed here is a re roll!

sbudker1’s picture

Status: Needs work » Needs review

test bot go

Status: Needs review » Needs work

The last submitted patch, twig-7563225-38.patch, failed testing.

joelpittet’s picture

Assigned: sbudker1 » Unassigned
Status: Needs work » Needs review
FileSize
34.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,097 pass(es). View

Re-roll again.

joelpittet’s picture

Issue tags: -Novice, -@todo clean-up
FileSize
8.81 KB
35.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,007 pass(es). View

Doc cleanups from #34

eromero1’s picture

Status: Needs review » Reviewed & tested by the community

I installed the patch, and checked all of the fixed subjects noted at the top under "Theme function name". Everything ran smoothly and it appears as though the patch worked swimmingly.

Cottser’s picture

Status: Reviewed & tested by the community » Needs review

Thanks for testing @eromero1, this still needs profiling before we can RTBC it though. Did you perform manual testing (markup comparison) or just a visual run-through? If you can write up step-by-step instructions for how you tested each theme function that would be extremely helpful not only for manual testing but for profiling as well.

jenlampton’s picture

Also, be aware that this is going to need a reroll after #1833932: Convert theme_system_compact_link() into a #type link

jenlampton’s picture

Issue summary: View changes

List of functions

Eric_A’s picture

I looked at the theme_status_report() part and noticed two things in the patch from #42:
This patch converts direct calls to theme() for the status_report hook to calls to drupal_render(). (Why?). Also, this is already happening in #2009674: Replace theme() with drupal_render() in system module. See also #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().
This patch changes the status_report theme hook to take variables rather than a render element. (Why?) As it happens this has already happened in #2041283: theme_status_report() is severely broken, so the patch will need a re-roll anyways.

Eric_A’s picture

Also, theme_status_report() only prints the icon and rows when the #type key is empty, if I'm not mistaken. The template nor the new preprocess function take care of this now, it seems. Related: #2013258: Simplify render for theme_status_report().

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Thanks for taking a look @Eric_A. Regarding your first point, if you look at the time stamps you can see the initial work on this issue predates all of the issues mentioned in #46. We've known for quite a while that we need to convert to render arrays.

If this needs a reroll, it should be set to needs work and tagged :)

pplantinga’s picture

Status: Needs work » Needs review
FileSize
34.5 KB
PASSED: [[SimpleTest]]: [MySQL] 57,615 pass(es). View

Reroll.

Eric_A’s picture

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

Needs work per #47 and #46.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
4.55 KB
31.08 KB
FAILED: [[SimpleTest]]: [MySQL] 57,456 pass(es), 362 fail(s), and 49 exception(s). View

Reverted unnecessary theme() to drupal_render() changes, and removed two templates that only had {{ variable }} in them (they're unnecessary and slow). As for #47, that issue is outside of scope for this issue, let's let #2013258: Simplify render for theme_status_report() take care of it.

Status: Needs review » Needs work

The last submitted patch, 1987410-51-twig-system-theme.patch, failed testing.

Eric_A’s picture

In #51 the code for the two templates was reverted but not the code for the preprocessors and theme functions. Also, #1939090: Convert theme_tablesort_indicator() to Twig had gone in already.

#2013258: Simplify render for theme_status_report() and #2010982: Replace theme() with drupal_render() in system module for system_status() are is in, too.

Eric_A’s picture

Sorry, #2010982: Replace theme() with drupal_render() in system module for system_status() is not in and not relevant here. Edited my earlier comment.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
33.75 KB
PASSED: [[SimpleTest]]: [MySQL] 57,896 pass(es). View

Ok I think there may have been some miscommunication here... #51 revert went a bit too far.

I took #49 which needed another re-roll. There aren't any theme to drupal_render switches that I can see.

@Eric_A could you try again with your review?

joelpittet’s picture

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Time for another reroll.

Cottser’s picture

The chart in the issue summary should be updated with the status of the current patch as well.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
32.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,435 pass(es). View

Looks like some things got dropped in favour of the routes, I like it:)

Here's the re-roll.

joelpittet’s picture

FileSize
32.32 KB
PASSED: [[SimpleTest]]: [MySQL] 59,094 pass(es). View

Re-roll

pplantinga’s picture

FileSize
32.33 KB
PASSED: [[SimpleTest]]: [MySQL] 58,347 pass(es). View

re-roll

pplantinga’s picture

Issue summary: View changes

updaed

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

jeanfei’s picture

FileSize
32.64 KB
PASSED: [[SimpleTest]]: [MySQL] 58,649 pass(es). View

Re-roll the #61.

jeanfei’s picture

Status: Needs work » Needs review

Forget to change status to 'need review'.

mcrittenden’s picture

Issue tags: -Needs reroll

Tags

joelpittet’s picture

Status: Needs review » Needs work

This is missing the removal of theme_system_modules_details And hopefully the template is not {{ content }} as it is now, if so try to remove the template all together.

rteijeiro’s picture

Assigned: Unassigned » rteijeiro

I will try to fix it :)

rteijeiro’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Assigned: rteijeiro » Unassigned
Issue tags: -Needs issue summary update

never mind updated issue summary.
theme_system_modules_details will be removed in #1938930: Convert theme_system_modules_details() to #type table

joelpittet’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

thedavidmeister’s picture

didn't mean to add that tag back on :(

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Issue summary: View changes
Issue tags: +Needs reroll

Needs a re-roll, patch doesn't apply.

longwave’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
32.73 KB
PASSED: [[SimpleTest]]: [MySQL] 59,114 pass(es). View

Rerolled #63. Minor comment cleanup, and effectively reverted #1143922: Undefined index: value in theme_status_report() as explicitly checking for 'value' is not needed now we have Twig.

longwave’s picture

FileSize
32.72 KB
PASSED: [[SimpleTest]]: [MySQL] 59,031 pass(es). View

Also changed .element-invisible to .visually-hidden.

joelpittet’s picture

Nice catch on the .visually-hidden. I didn't even see that conversion get in. For anybody looking that change record is here: https://drupal.org/node/2022859

Cottser’s picture

Issue summary: View changes
Issue tags: -Needs manual testing, -Needs profiling

Updating the issue summary in preparation for splitting this up by function and making this a meta instead of an all-or-nothing issue.

Cottser’s picture

Issue summary: View changes

Crossing out the theme functions that have actually been removed already.

Cottser’s picture

Title: system.module - Convert theme_ functions to Twig » [meta] system.module - Convert theme_ functions to Twig
Issue summary: View changes
Status: Needs review » Active
Issue tags: -Core Review Bonus
Parent issue: » #1757550: [Meta] Convert core theme functions to Twig templates
Related issues: +#1898454: system.module - Convert PHPTemplate templates to Twig, +#1938930: Convert theme_system_modules_details() to #type table, +#1833932: Convert theme_system_compact_link() into a #type link

Converting to a meta.

joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
Cottser’s picture

Status: Active » Fixed

Woo, just one postponed issue left so I'm going to make that a child of #1757550: [Meta] Convert core theme functions to Twig templates instead. Great work everyone, making this and form.inc a meta really helped get this done! :D

Status: Fixed » Closed (fixed)

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