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.
Problem/Motivation
Tracker has the following controllers:
- TrackerPage
- TrackerUserRecent
- TrackerUserTab
but they are all just including tracker.pages.inc.
Proposed resolution
Adds a single new controller that contains the code and also some code improvements, DI and so on.
Remaining tasks
User interface changes
API changes
The old controllers and functions are deprecated. Technically, none of them ever were an API, they are internal per BC policy.
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#53 | 3015650-53-interdiff.txt | 5.34 KB | kim.pepper |
#53 | 3015650-53.patch | 21 KB | kim.pepper |
#49 | 3015650-49-interdiff.txt | 2.55 KB | kim.pepper |
#49 | 3015650-49.patch | 16.31 KB | kim.pepper |
#46 | 3015650-46-interdiff.txt | 1.83 KB | kim.pepper |
Comments
Comment #2
kim.pepperTagging for DrupalSouth
Comment #3
jibranWe can modernize the module file as well maybe in followup.
Comment #4
andypostMeanwhile it is time to brainstorm on tracker fate #1261120-12: Deprecate Tracker module in D10 and move to contrib in D11
Comment #5
andypostComment #6
Eric115 CreditAttribution: Eric115 at PreviousNext commentedComment #7
andypostjust to make ball roll
Comment #8
kim.pepperWe can inject these.
We can use PagerSelectExtender::class instead.
Missing docs.
Comment #9
kim.pepperWe should typehint this with
/** @var \Drupal\node\NodeInterface[] $nodes */
Should we be using get() and set() for these properties?
Comment #10
vacho CreditAttribution: vacho at Skilld commentedSolving point 1 from #9 comment.
Comment #11
Berdirhttps://api.drupal.org/api/drupal/core%21modules%21comment%21comment.mod... also loads comment statistics and adds it to the comment field. Makes me wonder if the code here is even still necessary?
urlInfo() is deprecated, should use toUrl()
I'm trying to understand why we really still need those old classes?
Could we maybe rename TrackerBase to TrackerController and actually deprecate the child classes, and update the routes to call the parent directly?
Comment #12
kim.pepperI also added change record for removing tracker.pages.inc.
Comment #14
kim.pepperReverting changes to core/modules/migrate_drupal/tests/fixtures/drupal7.php
Comment #16
BerdirCouldn't we keep it public, rename $account to $user and then point the routes directly to buildContent()? They should be able to deal with an optional argument that's only set on one route just fine?
should probably inject the entity type manager and load through the storage handler?
Comment #17
kim.pepperYep. Sounds good.
Comment #19
kim.pepperNeed to rename the 'account' param to 'user' as @Berdir suggested above.
Comment #20
jibranNeeds rebase.
Comment #21
kim.pepperRe-roll of #19
Comment #22
jibranWe have ETM injected already let's add nodeType storage property and get rid of this call.
$type = $this->nodeTypeStorage->load($node->bundle());
Comment #23
jibranSorry one more nit.
We have injected nodeStorage so no need to use ETM method.
Comment #24
jibranAnd one more nit.
We can have
$owner
Comment #25
kim.pepperComment #26
andypostA bit of clean-up and address #22-23
Comment #27
jibranThis is a nice cleanup but unfortunately, there are some redundant changes. :)
These are not the same thing.
The removed line will show, 'Page' or 'Article' and the added line will show 'Content Type'.
This is a redundant change.
Comment #29
andypostreroll and fix #27.1 - reverted back to
node_get_type_label()
is not deprecated and has todo to implement access from entity to bundle entity, so I did not injected node type storage - it supposed to be retrieved from$node
#27.2 is not redundant - controllers should not cache the entity definition (value object) inside properties
Comment #30
BerdirMaybe we could store this comment metadata in $tracker_data instead of assigning to to arbitrary top-level node properties, which relies on __get() magic. We could also explicitly initialize those two array keys in the loop with $tracker_data += [...]; to avoid those if/else cases.
We should also add @deprecated and @trigger_error to the old functions in tracker.pages? No need to call the controller from them, just leave the old code.
Comment #31
andypost@Berdir good point about #30.1 but .2 makes no sense because trigger_error() added to class - so it someone using it as parent should get message
Comment #32
andypostOTOH whole class needs deprecation to be caught by sniffers
Comment #33
Berdir"Old functions in tracker.pages" (.inc).
The controllers are just wrappers around the old functions in the include file, I'm saying we should deprecate them too.
Comment #34
andypostPatch deprecates old function
btw This statistics loaded in
comment_entity_storage_load()
so this properties probably just adjusted hereComment #35
Berdir> btw This statistics loaded in comment_entity_storage_load() so this properties probably just adjusted here
Yeah, but while not very obvious, that hook stores the information on the comment fields, of which there might be multiple. I assume this code was rewritten then to still store that information as summary of all comment fields that might exist on the node. So it's similar to that but actually not the same anyway and we could also store it in our own data array.
Comment #36
BerdirOverlooked that $tracker_data is a list of objects too, so the initialize-with-defaults parts doesn't work but I still prefer to not put stuff onto the node entities like that, and combined with the ?? operator, we can clean it up a bit further (the current code relies on __get('last_comment_timestamp') not triggering a notice.
Comment #37
mikelutzComment #38
mikelutzI think moving the logic is fine, and I'm fine to move it without unnecessary refactoring. A couple nits.
While technically being used as a base class to the deprecated clases, this isn't really a base class, it is THE tracker controller.
I'm guessing you started planning to make an abstract base class that the other three could extend and changed you r mind.
I'm supposed to make you write legacy tests to prove these errors are triggered, but even I confess I don't see the value..
Can we deprecate this whole file a la https://www.drupal.org/core/deprecation#how-file in addition to the function?
Comment #39
Berdir1. & 2. Fixed the comments.
3. & 4. Yeah, per https://www.drupal.org/core/d8-bc-policy, both controllers and and specifically also "Locations of procedural code" mentions that filenames and location of functions in them is internal. We've started being way more strict than that page recently, specifically on constructors, but I also think that this is enough. The chance that someone used these very specific controllers/functions that aren't reusable in any way is basically zero.
Comment #40
BerdirComment #41
mikelutzI guess my argument on adding the error to the file is specifically that
require_once(\Drupal::service('module_handler').getModule('tracker').getPath() . "/tracker.pages.inc");
will work in D8 with no error triggered and throw a fatal in D9. That all having been said, we still have to load all the legacy include files in D8 and I assume we are intending to remove them for D9, So so be it. Perhaps we should update https://www.drupal.org/core/deprecation#how-file to say simply to deprecate all the functions, and not say we need to trigger the error in the file itself.Comment #42
larowlanshould we be using getDisplayName here instead - the docs of getAccountName say
I think we need a deprecation test for this
Comment #43
larowlanhttp://grep.xnddx.ru/search?text=tracker_page&filename= shows its just portfolio_theme, which had core committed in an earlier version
Comment #44
xjmComment #45
andypostNW for #42.2 at least
Comment #46
kim.pepperFixes for #42.
Comment #47
jibranBack to RTBC as #42 has been addressed.
Comment #48
dpiChanges look good,
Minor feedback:
The title only applies to one of the routes, I suggest renaming the method, or removing it entirely: setting the title in controller instead, with
['#title']
in the render array.Falling under "code improvements" / modernization:
Many of these merge calls and other '#cache' usage can be converted to: creating a
\Drupal\Core\Cache\CacheableMetadata
object at the top, adding all cache metadata in between, thenapply
-ing just before the render array is returned. Code would be simpler to read.Comment #49
kim.pepperThanks @dpi!
Re: 'Minor feedback:'
Not entirely sure how we determine whether to set the title or not from inside
buildContent($user = NULL)
. There are two routes that pass a$user
but only one of them sets a title.Re: "code improvements" / modernization:
Good idea! I've not spent much time with this, so let me know if this works.
Comment #50
dpiAfter looking at how titles are determined/set, it appears the way it is implemented may be more straightforward.
Thanks for the cacheable metadata object, looks good!
Comment #51
alexpottOne of the dangers of not having tracker_page() call out to the new controller is that this test isn't really enough. If we committed this change we'd have no coverage of the code in tracker_page() beyond that it returns a value that's not empty. Beyond this the change looks great - nice to see this finally cleaned up.
Comment #52
alexpottThis could be
return \Drupal::classResolver(TrackerController::class)->buildContent($account);
and then everything is using the same code.Comment #53
kim.pepperAh yes! Good idea.
Let's see what the bot says. :-)
Comment #54
jibranYeah, this looks better now.
Comment #55
alexpottCommitted 3ed7550 and pushed to 8.8.x. Thanks!
So this change to use display name is actually a bug fix. It's been discussed on #2629286: Use getDisplayName() for user names consistently - an issue where progress has been hard. Given that and the fact that there is no logic in either ::getTitle() method I think we should proceed here.
Comment #57
alexpottFixing the issue title to be closer to what has occurred.