As a follow-up to #2462589: Provide test coverage for access checking of all views fields.... That issue created generic tests to validate that base entity fields obeyed entity access rules when used in views.
However, when that patch was created, not all of the base entity fields were included, because they would have at that time failed the tests.
So, once all of the critical "convert this field to use an Entity-aware formatter in Views" issues on #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality are fixed, we need to come back here and make sure all the entity base fields for all our entities are covered.
Novice task
Start with the patch in comment #8. See comment #12 for instructions.
Comment | File | Size | Author |
---|---|---|---|
#53 | interdiff_49-53.txt | 5.7 KB | namitasaxena |
#53 | 2464635-53.patch | 12.52 KB | namitasaxena |
Comments
Comment #1
dawehnerI don't think this issue has to be critical, given that we have a good amount of test coverage with that other patch AND
all things do rely on the same underlying code, so I would vote just for major.
Comment #2
jhodgdonI was thinking that this would be the place to say "Make sure we really got everything". But either way, the parent is critical after all.
Comment #3
xjmShouldn't we just be adding to the test in each issue? That's the testing gate.
I don't think this is critical on its own but also I don't think it's a separate issue.
Comment #4
xjmComment #5
jhodgdonOnce we get #2462589: Provide test coverage for access checking of all views fields. in, yes we can add tests in each issue. Just depends on the order of things, and whether that actually happens. If everything gets added as we go then we can eventually just close this.
Comment #6
xjmAh, I get it now. Thanks!
Comment #7
Mile23Mentioned in @todos in a number of test files.
Comment #8
jhodgdonHere's a first pass patch. I grepped for this issue number in Core, and checked to see which base fields on each entity were and were not covered by access tests. I un-commented a few lines that were already there, added a few lines if they were obvious, and made a list of additional base fields that still need tests added.
So... it's not done but at least it shows more clearly what needs to be done, and we can also see if the uncommented and new tests pass.
Comment #10
jhodgdonWell, it looks like the Comment one was wrong, as there's no langcode() method on Comment:
Fatal error: Call to undefined method Drupal\comment\Entity\Comment::langcode() in /var/www/html/core/modules/comment/src/Tests/Views/CommentViewsFieldAccessTest.php on line 71
Also the tests failed on the added comment_type test in Comment, and also one of the timestamp fields in File (either file changed or file created -- not sure which of the two newly uncommented lines failed).
So this still needs some work, as well as some more tests added, but it should probably be done?
Comment #11
tstoeckler@jhodgdon: Looking at some of the other tests it seems you want
$comment->language()->getName()
?Comment #12
jhodgdonYes probably. The other test fails need to still be looked into, though, plus more tests need to be added. I don't really plan to work on this... maybe we should mark it Novice, as it's not really all that difficult to make sure those fields are set on the entities in the test, and add tests for them. Hopefully one of our outstanding Novice code contributors will pick it up.
Instructions for what needs to be done still:
a) Start with the patch in comment #8.
b) Background information: these tests go something like this. First, they create an entity of the type being tested, using code something like this ("user" entity in this example):
Then, they test access for each of the base fields on the entity, using code something like this:
So basically, you need to both make sure that the created entity has a value set, and then test its access by calling assertFieldAccess, passing in the value that you set on the entity.
c) You'll need to fix the 4 test failures, which are detailed in comment #10. Comment #11 has a suggestion for how to fix the PHP error. For the others, see (b).
d) In the patch in #8, there are still some @todo lines remaining that say we need to test additional fields. For each of these, add them to the test [see (b)] and then remove the @todo comments.
Comment #13
vg3095 CreditAttribution: vg3095 commentedI added some more tests in AggregatorFeedViewsFieldAccessTest .
I am new at this, so point me , if I am going in the wrong direction
Comment #14
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedUpdate to run the test.
Comment #16
jhodgdonPlease, when you make a patch on an issue that had a previous patch, always make an interdiff file. Without one, it is very difficult to see what you have changed. At least you noted in your comment what you had done, so that is helpful -- but next time, please do make an interdiff file.
Anyway, it looks like the Aggregator additions are heading in the right direction, thanks!
I did notice one typo: in the tests, you tested 'image' twice -- the second time I think it should have been 'modified'.
Also I thought some of the values that were submitted for the test field were a bit weird. For instance,
I think that 'modified' is supposed to be a time stamp? So please look over all of the values in that section, and make sure each one is something that is actually reasonable for that value to be. And I don't think I would use 0 for timestamps, even though that is a legal value.
Thanks!
Comment #17
vg3095 CreditAttribution: vg3095 commentedThank you for the detailed feedback,
Now I have made an interdiff file and added some more tests.
Comment #18
vg3095 CreditAttribution: vg3095 commentedComment #20
jhodgdonThis is looking better, but there are still some problems, and also there's quite a bit of work to be done in this patch. And... I'm not sure if everything in the interdiff file actually got into the patch? I reviewed the patch file, not the interdiff...
Some notes:
This change is incorrect. The indentation of this line was correct in the original. Why did you change this line?
See previous review. This is not a good value for 'image'.
I'm not sure about these two either -- are those really values you'd expect for a feed hash and etag? It would be better to use realistic values. See previous review.
This indentation change is also incorrect.
If you are going to comment out the line for comment_type (a few lines down), you need to add it to this "Still needs coverage" line.
This indentation change is incorrect.
The indentation of this } is incorrect. The original was correct.
Comment #21
lokapujyaThe reason that comment_type fails is that the view created by assertFieldAccess() does not show the comment type id.
If I were to create a view in the UI - By default, the view would show the comment type label. The test might not have even created a comment type (and have no label to show). So would assertFieldAccess() need to be modified to show the comment type id?
Comment #22
lokapujyaAlso, do we really need these tests? access tests on every base field? Why would one be different from any other?
Comment #23
jhodgdonThe code that made all base fields both translatable and under access control was added piecemeal (by type of field), and testing of each piece (and whether or not all needed pieces were actually done) was deferred to this issue. Right now they are not tested.
Comment #25
lokapujyaI broke some tests, so fixing those first.
Comment #26
lokapujyaRegarding comment_type, the attached interdiff doesn't fix it. :( I attempted to force the view to show the id; It might be trying to display the comment type label, which doesn't exist.
Comment #30
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedTagging as "Needs reroll" against 8.1.x
Comment #31
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commentedRe-rolling against 8.1.x
Comment #33
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedComment #36
Torenware CreditAttribution: Torenware as a volunteer commentedThis one needs a patch reroll, not too bad as a novice task.
Comment #37
benjifisherComment #38
Mile23Patch in #31 still applies to 8.3.x.
Either add these tests or make a follow-up to add them if there's some reason we can't add them now.
Remove all the @todos for this issue. Search the whole codebase and make sure there aren't others to address.
Unused use statement.
Either fix or remove commented code.
Also a bunch of coding standards errors. Check the testbot results for a CS report: https://www.drupal.org/pift-ci-job/634797
Comment #39
pritish.kumar CreditAttribution: pritish.kumar at OpenSense Labs commentedAs per the #38
Made the following changes.
1. Removed all the @todos for this issue
2. Removed Unused use statement.
Comment #44
vacho CreditAttribution: vacho at Skilld commentedComment #45
vacho CreditAttribution: vacho at Skilld commentedPatch rerolled.
Comment #46
kostyashupenkoreroll of #39
Comment #49
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedRe-roll in 8.8.x and fixed some tests, Please review.
Comment #50
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedComment #52
mradcliffeI performed Novice Triage on this issue. I am leaving the Novice tag on this issue because I think that good progress has been made in the past and it is still clear what needs to be done.
I bumped this to 9.2.x-dev and changed it back to Needs work based on my review of #49 below.
It looks like these tests still need to be written. I think this is the issue to do this, so I'm going to bump this back to Needs work to add the missing tests.
1. Control statement should have a space between the statement and opening parantheses.
2. Operator should have spaces on either side.
3. Control statements should always be within code blocks (curly braces).
Comment #53
namitasaxena CreditAttribution: namitasaxena at Srijan | A Material+ Company for Drupal India Association commentedResolved 2nd point mentioned in #52 and coding standards. Working on remaining test cases.
Comment #54
lokapujyaI see link in the test data.
Comment says we need to add link.
Here is the assert for url. I wonder if link needs a similar assert.
Comment #55
lokapujyaThe FeedInterface doesn't have a getLink(), but it extends ContentEntityInterface, which extends FieldableEntityInterface, which extends EntityInterface, which has a toLink() function. Maybe, that's the function to call in the test.
Comment #56
lokapujyaComment #62
andypostShould be added
too
The test group also needs fix to "file" module