Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
entity_page_access() works great as an access callback for hook_menu(), but we need an OO one for routes.
I discussed this with fubhy and he suggested an array value for the requirements key, but Symfony routes only support strings.
Dependent issues
#1946466: Convert all confirm_form() in user.module and user.pages.inc to the new form interface and convert route
Comment | File | Size | Author |
---|---|---|---|
#55 | 1947432-entity-page-access-54.patch | 22.27 KB | andypost |
#54 | interdiff.txt | 557 bytes | andypost |
#54 | 1947432-entity-page-access-54.txt | 22.27 KB | andypost |
#52 | entity-page-access-1947432-52.patch | 22.64 KB | tim.plunkett |
#52 | interdiff.txt | 1.98 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettSee attached.
Comment #2
Crell CreditAttribution: Crell commentedThe code uses a ., but the example in the docblock users a :. Pick one. :-) (Probably the .)
This looks like we could (gasp!) make a unit test for it rather than a functional test.
Should this be a return FALSE, or a return null? (False overrides all other access checkers unconditionally; null allows others to grant access. I'm inclined toward null.)
Comment #3
tim.plunkettYeah, I don't know how to write a unit test for routes, \Drupal\system\Tests\Routing\RouterTest was a functional test and I just mimicked that.
Fixed the other two things.
Comment #5
tim.plunkett#3: entity-access-check-1947432-3.patch queued for re-testing.
Comment #6
tim.plunkett@msonnabaum gave me this, can someone turn it into a test?
https://gist.github.com/msonnabaum/5207807
Comment #7
msonnabaum CreditAttribution: msonnabaum commentedBro, the test is there at the bottom. You just need to do the other method.
Comment #8
tim.plunketts/turn it into a test/turn the gist into a patch
There. :)
Comment #9
mtiftI'll try.
Comment #10
mtiftI've got a start on this, but I have a couple of questions.
1. Like the other classes that extend UnitTestCase, is it OK to put this one in core/tests/Drupal/Tests/Core (away from all of the other Entity tests)?
2. Is the getInfo() method in this test desirable, so that this test shows up on admin/config/development/testing?
3. Is my testAccess() method anywhere close?
I'm getting a fatal error on:
$operation = $access_check->access($route, $request);
Comment #11
jibranFor test
Comment #13
andypost#10: entity-access-check-1947432-10.patch queued for re-testing.
Comment #14
tim.plunkettLet's rename this _entity_access
Turns out there is a getRequirement() method, so we should use that instead.
Contains \Drupal\...
Start with "Tests" and it needs to be under 80 chars.
This doesn't follow our coding standard, each of the chained calls should be indented two spaces, regardless of how long the top one is.
Comment #15
Crell CreditAttribution: Crell commentedThis class can be removed, as we don't need the functional test if we have the unit test.
Ibid.
That seems wrong... access() should be returning a boolean, not a string.
Comment #16
BerdirI don't have a lot of experience with this yet, so mostly a few questions/remarks.
Is there a reason it was combined instead of _access_entity + _access_entity_operation or something like that?
Something that we've been fighting with in the access callback function is create access, there are still changes for that pending in #1862758: [Followup] Implement entity access API for terms and vocabularies. Basically, we need to create an entity including the correct bundle to be able to do that. Would be unfortunate if we'd need a custom access check for all entity types for that.
->access() isn't entity specific, it exists for all typed data, you could e.g. also use it on a field. Sounds complicated, all I'm suggesting to do is use the specific interface (Accessible) instead of EntityInterface and a different variable name.
Would it be possible to write a generic implementation that could check access for multiple arguments? ("node:view, user:view")
Comment #17
mtiftHere is an updated patch that covers most of #14 and #15. I have not been able to get the testAccess() method to work correctly.
Comment #19
tim.plunkettThese would also need to be switched to _entity_access.
Comment #20
mtiftThe attached patch changes all instances of _access_entity to _entity_access. Next, I need to dig into the PHPUnit Manual and figure out the correct way to use setConstructorArgs().
Comment #21
mtiftI still haven't figured out testAccess()
Comment #22
mtiftComment #24
Crell CreditAttribution: Crell commentedI don't think we need this test route at all since we're just doing unit tests.
Comment #25
BerdirAdd all the tags.
Comment #26
dawehnerWhat you could use is /entity_test/{entity_test}/delete as path and directly set the entity_test object into the route defaults. We test the EntityAccessChecker not the param converter.
In general you would probably have to write a DrupalUnitTest for access checking at the moment, because the entity->access() method relies on drupal_container() which is not available in PHPUnit
Comment #27
Crell CreditAttribution: Crell commentedThis is wrong. $request->attributes is a ParameterBag. Assigning a string to it is likely to break all sorts of things in exciting ways.
It should actually be set to a new mock object of type EntityInterface, and we can then mock what the access() method does.
Comment #28
XanoI'm converting this to an check access for any typed data object that implements AccessibleInterface, as per Berdir's original suggestion in #16. Need to run now, but I'll post a patch later tonight.
Comment #29
Crell CreditAttribution: Crell commentedUgh. I really have to stop this somewhere. Replacing every object in Drupal with typed data, a system that several people still don't understand the purpose of in the first place, is not going to fly. For Dries' sake, all we need here is to finish the tests. This issue is holding up many others at this point. Do NOT complicate it further.
Comment #30
tim.plunkettI second Crell's sentiment and request.
Also, since this blocks a half dozen conversions, it's major.
Comment #31
tim.plunkettDidn't touch the tags.
Comment #32
BerdirAll I'm suggesting is to use the proper interface. Accessible, which defines access(), not EntityInterface, which extends from Accessible :)
Comment #33
msonnabaum CreditAttribution: msonnabaum commentedJust because EntityInterface extends AccessibleInterface doesn't mean we should always default to the lowest common denominator.
Comment #34
andypostIs it ok to return NULL? Suppose better $result = FALSE; return $result
Comment #35
XanoMaybe throw an exception when the object does not implement EntityInterface to help debugging?
Comment #36
Xano@larowlan expressed an interest in finishing the patch this weekend.
Comment #37
larowlan1) Credit to @rootatwc who pointed me to an awesome reference on php mock units (this is my first time, be gentle)
2) Uses AccessibleInterface instead of EntityInterface, yes this is the lowest denominator but in this case it is the relevant chunk, the access method comes from that interface, not EntityInterface.
3) Fixes the unit tests by mocking a node.
4) Removes the test route (we don't need it with unit tests)
5) Adds a comment regarding #34 to clarify that NULL *is* a valid return, and that it indicates that this access check has no opinion.
Go bot go!
Comment #38
larowlanComment #39
larowlan@Crell in terms of timing - would this entity access check fire before the param converter had validated the entity?
ie would we ever hit the chunk of code that checks the $slug parameter implements AccessibleInterface without a valid option (other than when the routing configuration was broken).
IE has the param converter already verified that the slug is actually an entity by this stage of the processing?
Comment #40
klausiWhy do we ever return NULL here? If we cannot retrieve the entity then the route configuration must be broken?
And why is this entity specific? The only thing we rely on here is AccessibleInterface, not entities? Not sure about a good name instead of _entity_access, maybe _object_access and ObjectAccessCheck?
Also: silently eating errors if the AccessibleInterface is not implemented by the object does not help developers. Either throw exceptions or just remove the check, a fatal error indication is fine that the route is somehow b0rken :-)
(I'm assuming here that the routing system already handled non existing entities at the point when the access checkers are invoked, correct me if I'm wrong. So we can assume that the object is just there.)
Comment #41
tim.plunkettNo, that's ridiculous. We have a callback to check access for entities, and we need an OO version.
We have no more need for ObectAccessCheck than we do StringAccessCheck.
I also agree with msonnabaum that we should use the interface we care about, which is EntityInterface.
Comment #42
larowlanLet's just switch back to entity interface and get it in.
Simple name change from here.
Comment #43
tim.plunkettComment #44
Crell CreditAttribution: Crell commentedWe're done now. Let's unblock things.
Comment #45
larowlan+1 Thanks @tim.plunkett
Comment #46
webchickI have absolutely no idea what a "slug" is (assume that's some Symfony thing) but in this case we know that $slug actually means $entity, so it would make the code clearer if we just called it that.
Additionally, we need to document somewhere in here what the valid operations are, and that the first part of that key corresponds to the machine name of the entity.
And finally, I'd love to see at least one conversion in this patch too, so we can verify that this is working properly in the "real world."
Comment #47
tim.plunkettSlug is the term for {foo} placeholders in Symfony routes. It comes from http://en.wikipedia.org/wiki/Slug_(publishing)
I can revisit those docs and add in a conversion. I'll just pick whichever issue this blocks that is closest.
Comment #48
tim.plunkettOkay, here it is combined with #1945564: Convert confirm_form() in shortcut to the new form interface and convert route which is a pretty representative conversion.
Comment #49
tim.plunkettrootawc reminded me that now we can inject services.
Comment #50
ParisLiakos CreditAttribution: ParisLiakos commentedlooks good now, thanks!
Comment #52
tim.plunkettI typehinted the factory not the class, and the unit test needed some constants. Back to RTBC per #50.
Comment #53
andypostpatch needs re-roll
after #1807042: Reorganizie entity storage/list/form/render controller annotation keys
could change after #1947892: Improve DX with EntityAccessControllerInterface
Comment #54
andypostAlso no reason to mark access callback as deprecated because it's used to grant access to edit not delete
Comment #55
andypostwith patch extension
Comment #56
tim.plunkettThis was RTBC before the reroll, and I guess I was a little overzealous with that @deprecated, removing it is fine. Thanks @andypost!
Comment #57
webchickCommitted and pushed to 8.x. Thanks!
Will need a change notice to tell people what to do with former calls to entity_page_access() probably.
Comment #58
tim.plunkettAdded http://drupal.org/node/1982078
Comment #59
BerdirLooks good, but not sure about mentioning entity_page_access(), it's a function that we only added a few weeks ago, doesn't have a change notice (No reason to, we already knew when we added it that it's only temporary and will be replaced with this).
I'd just say something like "Route access depending on an entity access operation" (possibly linked to the entity access api change notice).
Comment #60
BerdirDiscussed in IRC with @timplunkett, changed a bit: http://drupal.org/node/1982078/revisions/view/2667842/2671300.
Let's close this now :)
Comment #61.0
(not verified) CreditAttribution: commentedUpdated issue summary.