Problem/Motivation
Converting tracker listing to Drupal view.
Steps to reproduce
NA - not a bug.
Proposed resolution
Clean up current code. Convert the tracker listing into a view instead.
Remaining tasks
- Update patch with view which was taken out starting at #75
- Verified desired fields for view
- Update hook for importing new view
- Review current code base to make sure nothing was calling the tracker routes
- Code review
User interface changes
- There will be an additional view now.
- There will be a new tab on the user page for "Activity"
API changes
Removing tracker routes and menu link if that counts
Data model changes
Release notes snippet
Original report by damiankloip
Opposite to #1853572: Remove the old default tracker view, we need to create a new tracker view when #1829734: Expose tracker data to views is committed.
Comment | File | Size | Author |
---|---|---|---|
#98 | 1941830-98-WIP.patch | 36.4 KB | smustgrave |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedComment #2
xjmComment #3
xjmActually, let's be bolder. There's no reason to maintain the legacy listing since tracker is a feature module.
Comment #4
ekes CreditAttribution: ekes commentedChanging the filter to use the tracker_user table alters
explain
results to:Where it was PRIMARY and DEPENDENT SUBQUERY before.
Looking at making a patch.
Comment #5
ekes CreditAttribution: ekes commentedAttached patch which still needs work. Three known issues. I don't know if anyone has any thoughts:-
user/%/track
page needs an additional check on view the entity. in the present tracker the check is both 'access content' and view user. So that's an additional 'permission to view user' on the 'Tracker - User: ...' contextual filter (which seems less obvious than thetaxonomy/term/%
case); or #2029509: Add a generic entity argument validation plugin. with the ability to have both view content permission, and access user entity.tracker/%current_user
. There istracker/%user
generally (it's the same as user/%/track - so it could actually be removed - permission note above also valid here). It would be easy to make another pagetracker/my
as a tab where the argument is specified as current user. If following what is already in core is most important this should be done, one more page display - nothing really. It really is duplicated user/%own_user/track though - even in core now.Comment #6
xjmNice, thanks @ekes!
Comment #8
xjmComment #9
oadaeh CreditAttribution: oadaeh commentedThis is just an FYI that I am working on this issue to upgrade the patch to the current state of D8.
Comment #10
oadaeh CreditAttribution: oadaeh commentedHere's my attempt so far.
Here are the issues I know about:
* When showing the count of new comments, I get a WSOD and an error similar to this in the Apache logs: "PHP Fatal error: Call to undefined method stdClass::id() in /var/www/vcs/git/drupal/drupal/8.x/core/modules/comment/lib/Drupal/comment/Plugin/views/field/NodeNewComments.php on line 130, referer: http://localhost/drupalsandboxes/drupal-8.x/"
* When removing the tracker.routing.yml file, I got Symfony errors trying to access anything in user other than user/$uid/track, and so I've removed the controllers, since they aren't being used and removing them doesn't appear to cause problems, but I've left the routing and local_task YAML files.
* Local tests failed on a couple of things that were clearly working. I'm hoping those tests won't fail here.
Comment #12
oadaeh CreditAttribution: oadaeh commentedHere's a more improved view that more closely resembles the tables in the D7 version. The tests still fail, but I'm not sure how to correct them. I've tweaked the view and the tests so they better match each other, but Simpletest still doesn't see the results correctly.
Comment #14
killua99 CreditAttribution: killua99 commentedFirst a re-roll cause the patch doesn't apply to the last 8.x
So my interdiff is quite hard to reproduce.
Comment #15
oadaeh CreditAttribution: oadaeh commentedLet's at least see what the test bot has to say.
Comment #17
killua99 CreditAttribution: killua99 commented@oadaeh, that patch is a big fail. I did a wrong stuff with git and did commit the new file and did a mess.
If I have change today I'll fix it, and I did some research and I guess I did find the problem. (for the test stuff)
Comment #18
dawehner.
Comment #19
amitgoyal CreditAttribution: amitgoyal commentedReroll of #14.
Comment #21
oadaeh CreditAttribution: oadaeh commentedThe patches in #14 and #19 are incomplete.
Comment #22
oadaeh CreditAttribution: oadaeh commentedComment #23
oadaeh CreditAttribution: oadaeh commentedComment #24
dawehnerNote: converting the tracker page to a view would fix IMHO a security issue of not checking the access to those base fields.
Comment #25
larowlan+1 to the security hardening here - bumping it to major for that reason
Comment #26
oadaeh CreditAttribution: oadaeh commentedAs an FYI, this will have to be restarted from scratch, as the most recent patch is so far out of date, it doesn't even pretend to apply. I started it on Sprint day, but I had too many other distractions and didn't make very much headway.
Comment #27
pcambraI gave the reroll a try and updated the view. Let's get the ball rolling again :)
Comment #29
pcambraSome fixes, I guess that if we're not providing a legacy mode, the routing.yml file should be removed.
There are a bunch of tests accessing "activity" still, I guess they should go to tracker instead.
Comment #30
pcambraOoops, interdiff.
Comment #32
pcambraSome progress but still far fetched.
I've removed the routes and the links, I think that should probably happen on the view.
In hook_help there's this but tracker.page is not a route, what's the preferred option for arbitrary internal urls? (does views generate a route?) the Change record doesn't seem to mention it https://www.drupal.org/node/2346779
Comment #34
fgmLatest patch no longer even applied. Rerolled to check how it fares now.
Comment #35
dawehnerThis is the only access checking we do ... we need so a test to ensure that it works.
Maybe I'm stupid but we seem to not care about the level of access control ...
Comment #38
fgmAssigning to self to avoid someone else doing duplicate work.
Comment #39
fgmThis seems to work locally, do tests pass ?
@dawehner : all displays have access control based on
perm: access content
.Comment #41
fgmSome (all ?) fails were due to views' format changes. This should fail less.
Comment #42
andypostLooks the issue depends on related #2086125: Last read comment field/filter/argument uses still the node.changed instead of node_field_data.changed column
Comment #44
fgmMore fixes.
Comment #46
fgmMuch better, and it confirms andypost's remark about the comment statistics issue.
Comment #47
fgmThis should fix all the fails on the NodeAccessTest...
Comment #49
fgmThis restores the tabs, so should fixe some more errors.
Comment #51
fgmThis removes most exceptions and some of the errors locally. What does it give here ?
Comment #53
fgmNo time for more right now, but this should remove one fail.
Comment #55
fgmJust to be sure, part of one test removed : the case when comment module is there (it's a dependency for tracker) but comments are disabled.
This should pass, and is to make sure the problem is isolated.
Comment #57
fgmOne more of the test functions should now pass.
Comment #59
fgmOne more complete test should now be passing: the regex for new nodes had been wrong all the time.
Comment #61
fgmExpecting one less fail.
Comment #63
fgmOne less fail indeed, but as Drupalcon is over, I will not be able to continue on this in the next few days, so unassigning myself for now.
Comment #66
andypostSuppose that should go into 8.1
Comment #67
gaydabura CreditAttribution: gaydabura as a volunteer and at Skilld commentedComment #69
xjmThis issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.
Comment #70
gaydabura CreditAttribution: gaydabura as a volunteer and at Skilld commentedComment #73
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedHad a go at re-rolling this but it may be worth just starting from a new patch at this point. Adding the tag for visibility.
Comment #74
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedComment #75
pk188 CreditAttribution: pk188 at OpenSense Labs commentedComment #77
pk188 CreditAttribution: pk188 at OpenSense Labs commentedComment #84
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedPatch re-rolled, Please review.
Comment #86
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commentedRe-rolling the patch against 9.1.x-dev.
Comment #92
smustgrave CreditAttribution: smustgrave at Mobomo commentedrerolled for 9.5
Comment #93
smustgrave CreditAttribution: smustgrave at Mobomo commentedphpcs fix
Comment #94
smustgrave CreditAttribution: smustgrave at Mobomo commentedAnother phpcs
Comment #96
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo I believe all patches after #75 should be ignored. Seems after 75 the view itself was removed and if that wasn't noticed then these patches probably need more work.
Know the help text in tracker.module isn't functional because it's pointing to a route that is now being deleted.
The view itself probably needs to be recreated
Will need an install hook to import this new view.
Am I forgetting anything?
Comment #97
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #98
smustgrave CreditAttribution: smustgrave at Mobomo commentedUpdated the issue summary
Deleted TrackerController as that doesn't appear to be needed with tracker.routing being deleted.
Scanned the core repo looking for tracker routes and believe they've all been updated.
Updated the view to be more simple. Removed page_3 as it appeared ot be a duplicate of page_2
Just need to write an update hook
Attaching a work in progress patch for any early review.
Comment #100
quietone CreditAttribution: quietone at PreviousNext commentedThis extension is deprecated and scheduled for removal in Drupal 11.
This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.
It will be moved to the contributed extension once the Drupal 11 branch is open.