Issue #2152227 by JeroenT, joelpittet, steinmb, steveoliver, hussainweb, shanethehat, jenlampton, kpa, AnythonyR, EVIIILJ, kgoel, Cottser, dsdeiz, hanpersand: Convert theme_tableselect() to #theme table__tableselect

Task

Convert theme_tableselect() to #theme table__tableselect.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

  1. Add an article and add a number of comments and visit.
  2. Visit /admin/content/comment
Files: 
CommentFileSizeAuthor
#34 2152227-tableselect-34.patch4.33 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,407 pass(es). View
#32 Kaleidoscope_–_tableselect-before-empty_txt___tableselect-after-empty_txt.png35.41 KBjoelpittet
#25 interdiff.txt2.8 KBCottser
#25 2152227-25.patch4.28 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2152227-25.patch. Unable to apply patch. See the log in the details link for more information. View
#23 interdiff.txt2.24 KBJeroenT
#23 2152227-tableselect-suggestion-23.patch5.56 KBJeroenT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,290 pass(es). View
#18 2152227-tableselect-suggestion-17.patch7.12 KBJeroenT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,270 pass(es). View
#8 commentsmarkup-comparison.png316.14 KBjoelpittet
#8 Comments--tableselect-before-patch.png48.07 KBjoelpittet
#8 Comments--tableselect-after-patch.png49.79 KBjoelpittet
#8 comment-before-patch.txt22.2 KBjoelpittet
#8 comment-after-patch.txt22.34 KBjoelpittet
#7 interdiff.txt1.22 KBjoelpittet
#7 2152227-tableselect-suggestion-7.patch4.43 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,944 pass(es). View
#6 2152227-6.patch4.45 KBsteinmb
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,847 pass(es). View
#5 interdiff.txt2.84 KBJeroenT
#5 2152227-5.patch4.51 KBJeroenT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,169 pass(es). View
#2 2152227-2.patch3.04 KBJeroenT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,345 pass(es). View

Comments

Cottser’s picture

Issue summary: View changes

Adding a commit message to the issue summary so the folks who already worked on #1898480: [meta] form.inc - Convert theme_ functions to Twig and in the Twig sandbox get credit.

JeroenT’s picture

Status: Active » Needs review
FileSize
3.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,345 pass(es). View

Converted the theme_tableselect to table__tableselect.

Patch attached

joelpittet’s picture

Awesome @JeroenT could you add a couple URLs to review this change to the issue summary's Steps to test? Maybe a couple paths that use table select?

I'll do a manual review for you;)

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs steps to reproduce

Added steps to the Issue Summary. Did a quick test and The responsive JS and table class were missing from the comments admin table select.

"core\/misc\/tableresponsive.js"
<table class="responsive-enabled">

May need
'#responsive' => TRUE, and possibly some other type=>table default properties.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
4.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,169 pass(es). View
2.84 KB

@JoelPittet,

Thank you for the review. I added the responsive + added some missing classes to the table.

steinmb’s picture

Assigned: Unassigned » steinmb
FileSize
4.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,847 pass(es). View

PSR-4 fix

joelpittet’s picture

Assigned: steinmb » Unassigned
FileSize
4.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,944 pass(es). View
1.22 KB

This looks good, just tried it out again and reviewed. I'm just posting those changes in this patch. Let me know if you have any concerns with the changes?

  1. +++ b/core/includes/form.inc
    @@ -1632,14 +1634,8 @@ function form_process_container($element, &$form_state) {
    +function form_pre_render_tableselect(array $element) {
    

    No other pre_renders type hint the $element so I'd rather not in this case unless you want to open an issue to do them all at once?

  2. +++ b/core/modules/comment/src/Form/CommentAdminOverview.php
    @@ -202,15 +202,22 @@ public function buildForm(array $form, array &$form_state, $type = 'new') {
    +          'data' => drupal_render($username),
    

    You don't need to call drupal_render here. Just data => $username.

    Save the flattening.

joelpittet’s picture

thanks @steinmb and @JeroenT

Here's a manual testing. It's looking good, this patch fixes a responsive bug in rendering the comments form. See screenshots.

JeroenT’s picture

@joelpittet,

The responsive fixes come from RESPONSIVE_PRIORITY_MEDIUM and RESPONSIVE_PRIORITY_LOW:

	@@ -202,15 +202,22 @@ public function buildForm(array $form, array &$form_state, $type = 'new') {
             '#title' => $comment->getSubject(),
           ) + $comment_permalink->toRenderArray(),
         ),
-        'author' => drupal_render($username),
+        'author' => array(
+          'data' => $username,
+          'class' => array(RESPONSIVE_PRIORITY_MEDIUM),
+        ),
         'posted_in' => array(
           'data' => array(
             '#type' => 'link',
             '#title' => $commented_entity->label(),
             '#access' => $commented_entity->access('view'),
           ) + $commented_entity->urlInfo()->toRenderArray(),
+          'class' => array(RESPONSIVE_PRIORITY_LOW),
+        ),
+        'changed' => array(
+          'data' => $this->date->format($comment->getChangedTime(), 'short'),
+          'class' => array(RESPONSIVE_PRIORITY_LOW),
         ),
-        'changed' => $this->date->format($comment->getChangedTime(), 'short'),
       );
       $comment_uri_options = $comment->urlInfo()->getOptions();
       $links = array();
joelpittet’s picture

Thanks, yeah this is ready RTBC in my opinion, just checking if Cottser agrees to remove the profiling task as it's usually just a Admin UI item and we aren't using it in many places nor are we converting to twig.

Cottser’s picture

Status: Needs review » Needs work

This should be faster because it wouldn't be going through the whole theme stack. I can profile it if we want to double check.

+++ b/core/includes/form.inc
@@ -1594,7 +1594,9 @@ function form_process_container($element, &$form_state) {
  * @param $variables

@@ -1632,14 +1634,8 @@ function form_process_container($element, &$form_state) {
+function form_pre_render_tableselect($element) {

@param should be $element here.

Other than that looks good at a glance.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
4.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,065 pass(es). View
19 bytes

I made changes to the comment of the function form_pre_render_tableselect.

Patch attached.

JeroenT’s picture

FileSize
694 bytes

Wrong interdiff, this is the right one.

Cottser’s picture

Status: Needs review » Needs work

Thanks @JeroenT!

+++ b/core/includes/form.inc
@@ -1593,11 +1593,12 @@ function form_process_container($element, &$form_state) {
+ * @param $element
+ *     An associative array containing the properties and children of
  *     the tableselect element. Properties used: #header, #options, #empty,
  *     and #js_select. The #options property is an array of selection options;
  *     each array element of #options is an array of properties. These
@@ -1631,14 +1632,8 @@ function form_process_container($element, &$form_state) {

@@ -1631,14 +1632,8 @@ function form_process_container($element, &$form_state) {
  *       '#empty' => t('No content available.'),
  *     );
  *     @endcode

Cool, now everything nested underneath @param needs to be outdented by 2 spaces though :)

JeroenT’s picture

Status: Needs work » Needs review
FileSize
6.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,063 pass(es). View
2.87 KB

I outdented everything underneath @param.

Patch attached.

Cottser’s picture

Status: Needs review » Needs work

Thanks! I missed that in the previous patch the @param didn't have a data type specified, we should add that since we're changing that line: https://drupal.org/node/1354#param

Also the @code section needs to be indented by 2 spaces so that it stays on the same level as the @param info.

Cottser’s picture

Issue summary: View changes

Updating suggested commit message.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
7.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,270 pass(es). View
2.54 KB

@Cottser, I hope this is the last update to the comments ;)

I also changed the comment of form_process_tableselect.

Patch attached.

Status: Needs review » Needs work

The last submitted patch, 18: 2152227-tableselect-suggestion-17.patch, failed testing.

JeroenT’s picture

Cottser’s picture

We shouldn't touch form_process_tableselect() if we weren't changing that hunk already (and it doesn't look like we were). Scope is a delicate thing :)

Cottser’s picture

The indenting on the @code is not right yet, it's the @code...@endcode (and its contents) that needed to be indented, not just the contents. Sorry for any miscommunication there.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
5.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,290 pass(es). View
2.24 KB

Ok, I think I get it now.

I also removed the changes to the comment of form_process_tableselect. I thought i could also change this comment because it's also about the tableselect.

Patch attached.

Cottser’s picture

Status: Needs review » Needs work

Thanks again @JeroenT!

Looking pretty good here, I'm going to dig in deeper and do some profiling and testing. In the meantime, here are some minor documentation/coding standards items to look at:

  1. +++ b/core/includes/form.inc
    @@ -1596,18 +1596,19 @@ function form_process_container($element, &$form_state) {
    + *   table row's HTML attributes; see theme_table(). An example of per-row
    

    This should probably say "see table.html.twig" instead of "see theme_table()" because theme_table() is gone now.

  2. +++ b/core/includes/form.inc
    @@ -1789,7 +1784,6 @@ function form_process_tableselect($element) {
    - * @see theme_tableselect()
    

    Should this reference point to form_pre_render_tableselect() now instead of just being removed?

  3. +++ b/core/modules/system/system.module
    @@ -541,10 +541,13 @@ function system_element_info() {
    +    '#pre_render' => array('drupal_pre_render_table','form_pre_render_tableselect'),
    

    Minor: Missing a space after the first array item per https://drupal.org/coding-standards#array

Cottser’s picture

Status: Needs work » Needs review
Related issues: +#1876714: Remove #type 'tableselect'
FileSize
4.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2152227-25.patch. Unable to apply patch. See the log in the details link for more information. View
2.8 KB

Looks like this patch may have gotten a bit eager trying to fix this bug: #2296859: [regression] Responsive classes not always added to table rows

With the attached patch (which also takes care of #24) those changes are rolled back and the markup is identical except for two changes:

  • The table used for testing (/admin/content/comment) now has an ID of edit-comments added.
  • tableresponsive.js is before tableselect.js in the source.

Also poking around it looks like #type table supports tableselect and it looks like the plan was to remove this anyway, so ultimately this might be going away but it's still a decent cleanup from my perspective. See #1876714: Remove #type 'tableselect'

Status: Needs review » Needs work

The last submitted patch, 25: 2152227-25.patch, failed testing.

Cottser queued 25: 2152227-25.patch for re-testing.

Cottser’s picture

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

Looks like a slight performance hit but nothing too crazy. I profiled admin/content/comment with 50 comments, render cache disabled.

=== 8.x..8.x compared (53ccf06f3ae60..53ccf19d00e96):

ct  : 184,993|184,993|0|0.0%
wt  : 688,806|687,494|-1,312|-0.2%
cpu : 673,843|672,816|-1,027|-0.2%
mu  : 43,500,432|43,500,432|0|0.0%
pmu : 43,527,872|43,527,872|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53ccf06f3ae60&...

=== 8.x..tableselect-twig-2152227-25 compared (53ccf06f3ae60..53ccf1bb00d4b):

ct  : 184,993|185,672|679|0.4%
wt  : 688,806|690,321|1,515|0.2%
cpu : 673,843|675,479|1,636|0.2%
mu  : 43,500,432|43,506,848|6,416|0.0%
pmu : 43,527,872|43,517,136|-10,736|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53ccf06f3ae60&...

Status: Needs review » Needs work

The last submitted patch, 25: 2152227-25.patch, failed testing.

JeroenT queued 25: 2152227-25.patch for re-testing.

JeroenT’s picture

Status: Needs work » Needs review

Marking as needs review.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
35.41 KB

Did a manual test, and it works great. Only diff was it now has an ID and before it didn't. This is due to the addtion of drupal_pre_render_table I believe.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2152227-25.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,407 pass(es). View

Re-rolled, something must have just changed slightly as I just reviewed it.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 28ef6b2 and pushed to 8.0.x. Thanks!

  • alexpott committed 28ef6b2 on 8.0.x
    Issue #2152227 by JeroenT, joelpittet, steinmb, steveoliver, hussainweb...
alexpott’s picture

Title: Convert theme_tableselect() to #theme table__tableselect » Change record: Convert theme_tableselect() to #theme table__tableselect
Status: Fixed » Active
Issue tags: +Needs change record

Oops - we need a change record for this. If this isn't done in a couple of days I'll have to make this a beta blocker :(

penyaskito’s picture

Wanted to do this change record, and looked at the parent for possibly existing ones or using one as a template. Found that #2152205: Convert theme_date() to #theme input__date didn't have one. Maybe also needs one?

joelpittet’s picture

Status: Active » Needs review
Issue tags: -Needs change record

Drafted a change record, not sure it's really nessasary but maybe it's helpful?

In D7 we still had the #type=>tableselect, this just gets rid of the extra theme function and replaces it with a table template suggestion.
https://www.drupal.org/node/2322315

alexpott’s picture

Title: Change record: Convert theme_tableselect() to #theme table__tableselect » Convert theme_tableselect() to #theme table__tableselect
Status: Needs review » Fixed

Actually all I think we need to do is add this issue to this CR https://www.drupal.org/node/1876710 since what we people need to know to do is this:

+++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
@@ -249,12 +250,13 @@ public function buildForm(array $form, array &$form_state) {
-        '#type' => 'tableselect',
+        '#type' => 'table',
+        '#tableselect' => TRUE,

Sorry for the distraction.

alexpott’s picture

I think https://www.drupal.org/node/2322315 does not contain the necessary detail if someone was actually using theme_tableselect().

Cottser’s picture

Ultimately I'd like to think table__tableselect will be going away as well in favour of what @alexpott showed in #40 which is using #type table plus #tableselect = TRUE everywhere:

#1876714: Remove #type 'tableselect'

This is a step in the right direction either way though.

Status: Fixed » Closed (fixed)

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