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.
Apparently our test coverage doesn't extend to node revision links, they were broken and then fixed again in #2322949: Implement generic entity link view field handlers.
Comment | File | Size | Author |
---|---|---|---|
#115 | 96-115-interdiff.txt | 5.02 KB | alexpott |
#115 | 1863898-115.patch | 10.17 KB | alexpott |
#100 | interdiff-1863898-100.txt | 10.74 KB | lokapujya |
#99 | interdiff-1863898-76-96.txt | 24.85 KB | lokapujya |
#96 | views_revision_link-1863898-96.patch | 10.38 KB | jeqq |
Comments
Comment #1
dawehner/me sighs
Not sure whether you (other person) will find this code helpful, but maybe you do so here is it.
Comment #2
tim.plunkettRerolled.
Comment #4
dawehnerWorking on that ... what a pain.
Comment #5
dawehnerThis is really just temporary code-tracking.
Comment #6
jibranRe-roll. Interdiff is for minor change in core.
Comment #8
jibranUpdated the old view and minor nip tuck.
Comment #10
jibranThis will fix the test.
Comment #11
jibranThis will fix the test.
Comment #13
dawehner#10: 1863898-10.patch queued for re-testing.
Comment #15
jibranWell according to verbose messages the test is correct. I have the same 8 fails locally.
Comment #16
pcambraHere's a re-roll of #10
I've fixed some minor things, like invoking camel cased views methods and fixing the "revisions" link as it should be in plural form, also the test view was disabled so no way to test the path.
Also removed testing the view link as the view op is always available (no particular permission for that).
Comment #17
jibranHere is the reroll and some minor fixes.
Comment #18
jibran:/
Comment #20
damiankloip CreditAttribution: damiankloip commentedHere are a couple more fixes. This should also fix the RevisionLinkTest test too.
Comment #21
dawehnerNo need for a public property.
There should be a visibility.
Let's use an injected database connection.
We can also replace user_access with \Drupal::currentUser()->hasPermission
Comment #22
jibranworking on it.
Comment #23
jibranFixed #21 and some clean up.
Comment #25
jibranFixed tests and typo :S
Comment #26
dawehnerCan we also typehint on the actual code?
Can't we just use getRevisionId() ?
Comment #27
jibranFixed #27.
Comment #28
damiankloip CreditAttribution: damiankloip commentedDo we really need to add this call here? We probably don't really even need the query method? or empty query method?
Otherwise, this is looking pretty good.
Comment #30
jibranI have removed query method. And reverting the typehinit of
renderLink
to match the parent definition.Comment #32
jibranLet's try that.
Comment #34
damiankloip CreditAttribution: damiankloip commentedI vote just reverting the changes to the query method and leaving it how it was. I think that will work for us?
Comment #35
jibranReverted.
Comment #36
jibranIt is a quite day for VDC team anyone want to RTBC.
Comment #37
jibranReverted all the changes of query method.
Comment #39
damiankloip CreditAttribution: damiankloip commentedSorry, let's go with the patch in #35! I guess we do need to ensure the table..
Comment #40
damiankloip CreditAttribution: damiankloip commentedrtbc for #35.
Comment #41
catchWhy's the access check being taken out of access() and put into the rendering?
Comment #42
damiankloip CreditAttribution: damiankloip commentedBecause access() is only called when handlers are initialised. If it failed here the handler would be removed from the view altogether. This needs to use the current node to determine the access, as well as user permissions.
Comment #43
catchWon't preRender() run before renderLink() though? If so we're counting all the revisions for all the nodes when generating the delete/revert links, even if the user never has a chance of seeing them.
Also it might be too early here, but I can't see the access check being added back for the view link?
Comment #44
damiankloip CreditAttribution: damiankloip commentedI spoke to catch about this, we need to add back access. We have two levels here, whether we can view the node or not and whether we have a revision to access etc.. We need to check user_access still, as probably node_access() to determine if we even show a view link. So I think the current logic to revision linking is ok, we just need to add back the access stuff in some form. However, probably not in the access() method like before?
Comment #45
jibran35: drupal8.node-module.1863898-35.patch queued for re-testing.
Comment #47
xjm(Merging "node system" and "node.module" components for 8.x; disregard.)
Comment #48
xjmThis seems like a major regression to me, and it's also blocking views conversions.
Comment #49
kim.pepper37: drupal8.node-module.1863898-37.patch queued for re-testing.
Comment #51
kim.pepperComment #52
lokapujyaRe-roll. I ran out of time to figure out what to do about access(). Had to change node_access to node.access(). But, I think tests are going to fail there.
Comment #54
lokapujyaFixed a double use statement.
Comment #56
lokapujyaGet the re-roll up to date with Core changes.
Comment #57
Jalandhar CreditAttribution: Jalandhar commentedPatch needs reroll.
Comment #58
mgiffordComment #61
lokapujyaRerolled.
Comment #62
criscomTesting
Comment #63
criscomRerolled and patched successfully with no conflicts/errors.
Comment #64
jhedstromFrom #52
Has the link access issue been resolved in the subsequent rerolls (it's hard to tell without interdiffs)? That comment is the only mention of catch's concern from #43.
Comment #65
lokapujyaaccess() is only called on handler initialization, so the access check was moved to renderLink(). My understanding is that we need a better place for the access check, so that preRender doesn't get called?
Comment #66
lokapujyaThere must be similar access checks somewhere that give a clue how this should be done.
Comment #67
pcambraJust making a couple of nitpick changes to get the ball rolling on this.
Regarding the access discussion, last comment from @damiankloip is #1863898-44: Add test coverage for Views revision link handlers which suggests that we need to get the access check back, it appears that there are two kind of checks here:
$this->currentUser->hasPermission('delete all revisions')
$revision_node->access('delete')
Could we add 1) back to access() method and keep 2) in the renderLink() method?
I understand this is an issue just for the Revert and Delete revision links.
Comment #69
lokapujyaI need to re-review this myself to make sure it's still relevant, but here is a re-roll.
Comment #72
lokapujyaStill haven't look at the relevancy, but here is a start: Let's see the test only patch.
Comment #73
jhedstromHmm...the test only patch is passing, so it either isn't testing this issue, or this issue has been fixed elsewhere.
Comment #74
lokapujyaThe test wasn't running. I moved them to the right place, but the test don't seem to work either.
Comment #75
jhedstromPatch above is empty.
Comment #76
lokapujyaOh yeah, these are new files so git diff didn't pick them up.
Comment #78
jhedstromNice. Can you upload the combined patch now that the test is being run?
Comment #79
lokapujyaIncluded interdiff to show what was changed in the view. Keep in mind that the test is failing (but not necessarily failing for the reasons that it should.) So, I still haven't tried to verify that the bug is still relevant. For example, the first fail is "Link containing href node/1 found." But, I think that we are expecting that part should work (even without the fix from this patch.)
Comment #81
jeqqComment #84
lokapujyaFirst, Needs a reroll.
Then, Needs someone to look at the newly added test. Verify that the test is actually testing the bug properly.
Comment #85
neetu morwani CreditAttribution: neetu morwani as a volunteer commentedComment #86
neetu morwani CreditAttribution: neetu morwani as a volunteer commentedComment #87
jeqqI think the the problem has been fixed in #2322949: Implement generic entity link view field handlers.
I've rerolled the patch, the test demonstrates that the links are displayed correctly.
Comment #88
jeqqComment #89
lokapujyaWhich patch was rerolled? Also, was this just a reroll or does it also have additional changes?
Comment #90
jeqqThe patch #87 contains only the test from #79. Other changes from #79 I think aren't relevant anymore.
Comment #91
lokapujyaIt's just that 4 months ago the test was failing, and now it's passing. For completeness, it would be nice to know what changed.
Comment #92
lokapujya@jeqq Basically, this needs an interdiff so that it the changes can be reviewed. It looks like 87 was more than a re-roll.
https://www.drupal.org/documentation/git/interdiff
Comment #93
lokapujyaI think this is now unused and can be deleted. Other than that, it looks like we should commit this test.
Comment #94
jeqqThanks @lokapujya. I've removed the unused field. Unfortunately, for some reason I can't create an interdiff.
Comment #95
lokapujyaIt would probably be easier to create 2 branches. First with the old patch, Second with the new patch. Then just diff the 2 branches.
Some whitespace snuck in there.
Comment #96
jeqqRemoved the whitespace.
Comment #97
jeqqComment #99
lokapujyaInterdiff for reviewing.Oops I can see this is wrong by the filesize. Will try again later.Comment #100
lokapujyaHad to adjust the diff between 76 and 96 to only include the test. Also, it seems the directory name of the test changed; In order to see what actually changed in the file, in the older branch, I first moved the the file from the old directory to the new one.
Comment #101
lokapujya@jeqq: Can you verify that the interdiff is right?
Comment #102
jeqq@lokapujya: The interdiff is right, thanks.
Comment #104
lokapujyaNow we can add this new test coverage.
Comment #105
alexpottStill needs an issue summary update... and a title update.
Comment #106
lokapujyaComment #107
lokapujyaComment #108
lokapujyaComment #109
no_angel CreditAttribution: no_angel as a volunteer commentedComment #110
lokapujyano_angel, are you still reviewing this?
Comment #112
jonathanshawRTBC per #104. If further IS update is needed, please specify.
Comment #113
alexpottNot needed
I don't think there is any need for this to be a public property on the class. How about just a variable $nodes in the scope of the testRevisionLinks() method?
This is makes someone reading the test have to think way too hard. Just do
foreach (['revert', 'delete'] as $operation) {
Let's change
$op
to something more explicit that helps someone reading the test - maybe $allowed_operation?Given the view is over 12 months old it could probably do with being re-exported.
Comment #114
alexpottComment #115
alexpottAddressed everything from #113. Amazingly there were no changes to the view on export!
Comment #116
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe changes in the interdiff look good to me, thanks Alex!
Comment #118
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedRandom DrupalCI fail...
Comment #120
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedComment #123
xjmCommitted 5d4300e and pushed to 8.2.x and ecedab9