tablesort.inc is basically the traditional tablesorting functions that we've had for as long as I can remember, plus the same code duplicated in object-form to work with DBTNG. Example TableSort::getSort() and tablesort_get_sort(). This breaks the DRY principal. The object-form could probably be rewritten to re-use the functional version:

  protected function getSort() {
    return tablesort_get_sort($this->header);
  }
CommentFileSizeAuthor
#3 985836-3_tablesort_function_deduplication.patch1.99 KBjrchamp

Comments

Crell’s picture

Crell commented

Actually, I think the procedural code was just never removed. It shouldn't be used, period. Ever.

I don't know if we can still remove it at this point, though, with RC imminent. :-(

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented

theme_table() as well as everything from the entity system to forums calls the procedural code.

jrchamp’s picture

jrchamp commented
Status: Active » Needs review
StatusFileSize
new 985836-3_tablesort_function_deduplication.patch1.99 KB

I've made the appropriate modifications in a patch. It still depends on #839556: Fix isset regression in tablesort, add tests, and cleanup theme_process_registry(). I'm setting this issue to needs review so that the test bot will pick it up. This issue is still being discussed as to whether this is the correct decision.

moshe weitzman’s picture

moshe weitzman commented

well, the patch looks good. who could complain about removing dupe code. bot likes it too.

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented

I like it. Unless @Crell opposes I think we can mark RTBC.

Crell’s picture

Crell commented
Status: Needs review » Needs work

*expletive deleted*. I really thought we'd gotten rid of the procedural/global rats' nest.

We really can't do anything else besides this in Drupal 7, though. Not in RC state. I agree that code duplication is non-good. We should be sure to revisit it for Drupal 8, though.

Therefore, dalin, can you reroll with some @todo markers to deprecate and remove the old version in Drupal 8 and clean up tablesort generally? Then we can commit that, and refile this issue to Drupal 8 to clean up tablesort. (Pager needs to be gutted as well, but that's another issue.)

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Status: Needs work » Needs review

@Crell I did some deeper investigating and I'm not sure there's any value in adding @todos. Reason being that the purpose of adding a @todo would be for developers to see it and think "oh I shouldn't use this, I should use the object instead to make it easier to upgrade to D8". However I don't think it's possible to replace any of the procedural functions with an object equivalent. For example theme_table calls some of the procedural functions to theme the table headers etc. It can't use the TableSort class because it isn't doing anything with queries. And it doesn't make sense to replicate the issue queue as @todos in the code.

jrchamp’s picture

jrchamp commented

Well you could @deprecated it instead of @todo, but I realize that doesn't answer the theme_table situation.

Crell’s picture

Crell commented
Status: Needs review » Reviewed & tested by the community

Sigh. Ah crap. OK, then I guess #3 is good to go as is. (All of Drupal 7 is potentially @deprecated for Drupal 8. It's how we work. :-) )

Thanks, dalin and jrchamp.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

This looks very nice, but #3 says it depends on #839556: Fix isset regression in tablesort, add tests, and cleanup theme_process_registry(). Is that true?

jrchamp’s picture

jrchamp commented

Only in the sense that there is a bugfix in the patch on #839556: Fix isset regression in tablesort, add tests, and cleanup theme_process_registry(). That small bugfix can be rolled into this patch if that is preferred and the other patch can be rerolled without the bugfix.

Here's the small change that would ensure the object form is not altered by using the functional form:

- $first = current($headers);
+ $first = reset($headers);

I would prefer if the other patch went in first because the patch on this issue is much easier to reroll and is more of a deduplication patch than a bugfix patch.

dawehner’s picture

dawehner
Primary language German
commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
dries’s picture

dries commented

I just committed #839556: Fix isset regression in tablesort, add tests, and cleanup theme_process_registry() to 8.x. Not sure if that means we need to re-roll this patch. I'll try applying the patch in this issue later.

jrchamp’s picture

jrchamp commented

I apologize for the confusion, as I'm a bit new to git. This patch was merged with #839556: Fix isset regression in tablesort, add tests, and cleanup theme_process_registry() which I was testing on my install. As such, these issues were fixed by yesterday's commits.

Crell’s picture

Crell commented

Uh, so is this now committed to D8 then?

bfroehle’s picture

bfroehle commented
Status: Reviewed & tested by the community » Fixed

Yes, this has been committed to D8. And also D7.

aspilicious’s picture

aspilicious commented
Issue tags: -Needs backport to D7

untagging

Status: Fixed » Closed (fixed)

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