Proposed commit message:

Issue #1842326 by c4rl, sun, joelpittet, pakmanlh, jjcarrion | jenlampton: Merge _theme_table_cell() into theme_table().

One of the principles we arrived at during the last Drupal Twig sprint was that HTML elements like UL and TABLE should not be divided into smaller parts. A table is only whole if it contains cells, and a table cell can not stand on it's own. We want Drupal's theme system to match the mental model of front-end developers, and semantically match HTML.

_theme_table_cell() is just a shortcut to write a few lines of HTML (and consequently an example of when DRY becomes an anti-pattern).

Proposed solution: theme_table() refactors calls to _theme_table_cell() to simply mirror its functionality in theme_table(). Forum module also has a call to be refactored in a similar fashion. (Sidenote: Forum module theme functions need some heavy revision, let's save that discussion for other issues).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jenlampton’s picture

Issue summary: View changes

cleanup

jenlampton’s picture

Title: Merge _theme_table_cell() into theme_table » Make a renderable object for Table (and move _theme_table_cell() into a __toString method)

On closer consideration, this is a good candidate for a renderable object :)

c4rl’s picture

Title: Make a renderable object for Table (and move _theme_table_cell() into a __toString method) » Remove theme('table_cell')

Table cells are always part of a larger structure (a table) and do not need to be themed independently. theme_table_cell just isn't necessary.

Related: #1842160: Consider appropriate usage of theme() from within Twig templates

steveoliver’s picture

c4rl’s picture

Title: Remove theme('table_cell') » Merge _theme_table_cell() into theme_table
Status: Active » Needs review
FileSize
7.42 KB

Okay the original title makes the most sense. On further inspection, this is just a helper function. The attached patch removes this function and should make it easier to proceed with the Twig theme_table conversion.

Status: Needs review » Needs work

The last submitted patch, drupal-1842326-4.patch, failed testing.

c4rl’s picture

Status: Needs work » Needs review
FileSize
709 bytes
7.4 KB

I see the issue, there's an empty class attribute on td tags.

c4rl’s picture

Issue summary: View changes

clarify? iz tired

alexrayu’s picture

#6 fixed the empty td classes for me. The patch applied well and the form rendered correct.

thedavidmeister’s picture

Status: Needs review » Needs work

The patch no longer applies, unfortunately.

error: patch failed: core/modules/forum/forum.module:6
error: core/modules/forum/forum.module: patch does not apply

thedavidmeister’s picture

Issue summary: View changes

Expanded summary.

joelpittet’s picture

Issue summary: View changes
Issue tags: +triage, +Novice

Darn I didn't see this issue. I've duplicated it over at #2216407: Remove _theme_table_cell().. The forum bits were removed in #2092343: Consolidate forum.module and remove call to _theme_table_cell() within template_preprocess_forum_topic_list().

Could someone double check the pieces in this and see if anything is needed further at #2216407: Remove _theme_table_cell().?

sun’s picture

Title: Merge _theme_table_cell() into theme_table » Merge _theme_table_cell() into theme_table()
Assigned: Unassigned » sun
Status: Needs work » Needs review
Issue tags: -triage, -Novice
FileSize
12.02 KB
11.06 KB

I worked on this, and cherry-picked code from both the patch here as well as @joelpittet's patch in #2216407: Remove _theme_table_cell(). (duplicate now)

  1. - #2216407 by joelpittet, pakmanlh, jjcarrion: Remove _theme_table_cell().
  2. Take over array/non-array code path approach from #1842326: Merge _theme_table_cell() into theme_table() for performance.
  3. Convert TableTest into DUTB.
joelpittet’s picture

@sun Thank you for refactoring and still keeping some of the variables and logic we put work into the other patch!
I also like how you took the $cell['attributes'] and $cell['data'] off the $cell into their local variables because there is less juggling that way:)

Hopefully this goes green!

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

This removes both _theme_table_cell() and tablesort_cell() functions which puts the markup logic and addition of extra classes in context of where they are being built instead of passing the context into helper functions that seemed to become more complex th and td needed different conditions for handling their display.

This adds new tests and makes them faster (UnitTest now vs SimpleTest before).

Did a quick manual test using simply test me on the /admin/reports/dblog page which has the sort indicators and a fairly large table and the markup is identical.

It passes previous tests.

I also reviewed the code as well to see if there were any glaring mistakes or oversights that I could see and none came to mind.

RTBC++

star-szr’s picture

Issue summary: View changes
Related issues: +#2216407: Remove _theme_table_cell().

Adding proposed commit message to pull in the folks who worked on #2216407: Remove _theme_table_cell()..

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit 25991e9 on 8.x by catch:
    Issue #1842326 by c4rl, sun, joelpittet, pakmanlh, jjcarrion |...
sun’s picture

Status: Fixed » Closed (fixed)

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