Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Followup #2999721: [META] Deprecate the legacy include files before Drupal 9:
Problem/Motivation
Convert tablesort.inc
functions to a static class.
Proposed resolution
Implement table_sort
service and deprecate all tablesort.inc
provided functions
Remaining tasks
Convert content into static classDeprecate functions in tablesort.incReplace deprecated functions with the static method callsCreate and fill CR to highlite deprecation of the tablesort.inc fileAdd legacy test for deprecated functionsPrepare all external dependencies to static calls (translation, render, request)
User interface changes
None.
API changes
tablesort_* functions will be deprecated and removed at Drupal 9 release.
Will be introduced a new static TableSort class.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#33 | 3001201-33.patch | 25.59 KB | andypost |
#33 | interdiff.txt | 10.8 KB | andypost |
#26 | 3001201-26.patch | 27.14 KB | voleger |
Comments
Comment #2
volegerComment #3
andypostall this methods depends on request object so while refactoring this to service it makes sense to add request as argument to all this methods
Looking at the code I thing the request object should be passed to init() method and use fallback to request stack if no request object passed
Comment #4
volegerAddressed #3
Comment #5
goodboyNo need else statement in this case.
Comment #6
volegerAddressed #5
Comment #7
volegerPatch cleanup
Comment #8
andypostAdded draft CR https://www.drupal.org/node/3009182 and it needs deprecated tests
Patch does more clean-up of new implementation and I bet it needs follow-up to remove protected methods in
\Drupal\Core\Database\Query\TableSortExtender
and polish its docsImplementation
\Drupal\Core\Utility\TableSort::getOrder()
coiul;d be polished more to have less then 10 Cyclomatic Complexity.Also I think
\Drupal\Core\Utility\TableSortInterface::getQueryParameters()
method should not be a part of new interface - probably convert it to inline implementationthe only usage
Comment #9
andypostI went ahead and did clean-up cos this methods just duplicates service's ones
Comment #10
volegerLittle refactor of
request
helper method.getQueryParameters
moved out of an interface. Wondering should this method be a protected method as it used only inside the service?Added legacy tests.
getOrder
still require attention regarding simplifying it.Comment #11
volegerAfter discussion in the #contribute drupal-slack channel, we considered that that functions can be moved to the static class instead of a service. But this requires few changes. See issue summary updates.
Comment #12
volegerComment #13
volegerComment #14
volegerComment #16
andypostNow it looks better but now the most cruel thing left - naming!
I'd renaned init to more understandable getFromRequest()or getContext()
Also getQueryParameters() probably should be removed or marked as @internal
Comment #19
volegerUpdated docs
Renamed init method to getContextFromRequest
Marked getQueryParameters as internal.
Fixed issue with missed argument from last few patches
Comment #20
volegerComment #22
volegermissed namespace use
Comment #24
volegerMissed method replacement
Comment #26
volegerLegacy tests fixes
Comment #27
volegerFinally, all changes were refactored since new scope in #11
Comment #28
andypostCR updated & looks good to go
Comment #29
larowlanThis looks great! couple of minor comments on the final deprecation message and a follow up request
there is no container here, its static, can we update these comments?
Can we get a follow up to allow extenders to implement container injection?
Comment #30
andypostInteresting point!
@larowlan any reason to make injection?
The only reason is to acess request stack if request not passed but surely this class looks more like final to me
Comment #31
andypostI missed renderer dependency for header image render in
header()
methodComment #32
andypostFollow-up filed #3014607: Allow container injection for TableSortExtender
Comment #33
andypostRework of deprecation messages - no reason to provide examples because this functions examples itself
Comment #34
voleger#33 adressed #29 and followup filled in #32
So +1 for RTBC
Comment #35
larowlanAdding review credit for @goodboy
Comment #37
larowlanCommitted 52224e3 and pushed to 8.7.x. Thanks!
Published the change record
Great work folks, more includes down!