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.
#1696660: Add an entity access API for single entity access got commited, but now we need to actually implement the API for all entity types.
- Implement an entity access controller for terms and vocabularies
- Convert access checks to use
$entity->access()
.
Comment | File | Size | Author |
---|---|---|---|
#67 | taxonomy-term-create-access-1862758-67.patch | 2.49 KB | Berdir |
#67 | taxonomy-term-create-access-1862758-67-interdiff.txt | 1.01 KB | Berdir |
#47 | taxonomy-term-create-access-1862758-47.patch | 2.3 KB | Berdir |
#45 | taxonomy-term-create-access-1862758-45.patch | 2.36 KB | Berdir |
#43 | drupal-1862758-43.patch | 1.13 KB | dawehner |
Comments
Comment #1
dawehnerStarted with using access controller for the taxonomy term.
Not sure about the best way to handle vocabularies as access is currently only done via user_access, so the access controller could override all methods, so there should be discussion, whether we really want to add a new function for vocabulary access.
In general it feels odd to have a createAccess() method which requires an instance of an entity (so basically the same question as the entity operation API).
Comment #3
dawehnerUps!
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commented#3: drupal-1862758-3.patch queued for re-testing.
Comment #6
dawehnerJust a rerole against current version of core.
Comment #7
BerdirThe user issue added a entity_page_access() access callback that should be able to replace the whole function :)
Comment #8
dawehnerGood point!
Changed that.
Comment #9
ParisLiakos CreditAttribution: ParisLiakos commentedI guess you need to change access callback here as well
Besides that this seems ready
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedAlso, seems class files are missing from the patch
Comment #11
dawehnerThanks rootawc, such small things could cause hours.
In general it probably helps to include the actual access controllers as well :)
Comment #13
BerdirThis looks good.
I guess most other access checks related to terms and vocabularies use direct user_access() checks as the actual conversion here is extremely short.
I'm not sure if we want to check them too and replace with correct access() method calls. We could defer that to a follow-up but we could end up with an inconsistent situation where not everything goes through the access controller in case that follow-up doesn't get done.
So I'd vote for a quick check for user_access() calls in the module and replace with access() where easily possible in this issue :)
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedi think this issue will greatly help/simplify #1848686: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission)
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedi am gonna take care of #13
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedhrm..really i cant find any user_access call for taxonomy permissions:/
and i think my grep queries had nothing wrong
Comment #17
dawehnerOh great!
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedRTBC if bot agrees:)
Comment #19
BerdirHm, well, there are user_access() "calls" in taxonomy_menu()
This for example should now use entity_page_access('view'). Same for the feed path below. And there is an edit vocabulary path as well that could use this.
Create is tricky, I'm not sure what to do about that yet. Because we have no entity object that we could invoke access('create') on, so I guess we can leave that for now. But the ones above should be easy to implement.
Comment #20
dawehnerLet's see whether this works already.
Comment #21
dawehnerOpened a follow up to improve the EntityTestAccessController: #1900198: Use the access controller in the entity_test
There is no need for taxonomy_add_term_access().
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedAh,hmm right, taxonomy_menu..i missed that indeed
not sure about the create here..i would prefer not to add a callback and just keep using user_access in the hook_menu implementation..
but i can live with it ofc
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedThis needs an explanation berdir thinks and i agree. Its too vague now, can we add why?
Comment #24
dawehnerWhat about that?
Comment #25
Johnny vd Laar CreditAttribution: Johnny vd Laar commentedIn the issue that I was working on mentioned in comment #14 I also added a number of tests and usability improvements perhaps we can merge those into this patch?
Comment #26
BerdirThis issue does not introduce any new features. It's just an internal refactoring of how the permissions are checked.
So your issue will stay open but will have to be re-rolled after this is in to build on top of this.
Comments looks fine to me. I personally try to avoid using "you" and write it indirectly but that's more related to translatable strings I think, there are already a lot of you's in core, so I don't think that's a problem.
Comment #27
fagoI think we should find a way to create that object via the menu system, such that we can use the same object for access checks and for the page callback.
However, that's another clean-up which should be handled in its own issue - for now I think this is a good way to proceed.
Patch looks RTBC to me as well.
Comment #28
ParisLiakos CreditAttribution: ParisLiakos commentedCan we get this committed please?
#1848686: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission) is kinda blocked on this one and we are running out of time:(
#24: drupal-1862758-24.patch queued for re-testing.
Comment #30
ParisLiakos CreditAttribution: ParisLiakos commentedsome more taxonomy_term_access callbacks sneaked in term translation UI test
Comment #32
dawehner#30: drupal-1862758-30.patch queued for re-testing.
Comment #33
dawehnerIt has been RTBC before. The last fail seemed to be yet another random test failure.
Comment #34
ParisLiakos CreditAttribution: ParisLiakos commentedJust noticed that term access controller is named
TaxonomyAccessController
I guess it should be
TermAccessController
Comment #35
dawehnerOh right that's way better
Comment #36
dawehnerWe can simplify the term translation controller based on that.
Comment #37
ParisLiakos CreditAttribution: ParisLiakos commentedBot agrees, lets get this in
Comment #38
catchLooks great. Committed/pushed to 8.x.
Comment #39
swentel CreditAttribution: swentel commentedSo this line bothers me, especially the call to create(). If you go to admin/structure/taxonomy/tags, this will trigger hook_entity_create(), which triggers field_entity_create(), which triggers $entity->getTranslationLanguages() - however, there's no bundle on the $entity, so field_info_instances will return all fields. Now, all is ok when you have no fields on the tags vocabulary. But it you add one, instances will be found, however, no bundle, so this code:
is not getting the $field_name, but the actual bundle as the first parameter. field_info_field() will fail and field_is_translatable() will receive an empty field.
Now, field_is_translatable() is simply this:
I don't even understand why this doesn't trigger notices when field is NULL, must be something I'm learning again in PHP. However, while converting field api to cmi, $field['translatable'] becomes $field->translatable which /does/ trigger notices.
Anyway, something is funky here anyway because of that whole chain. A simply fix could be to add a check on $field, but that seems really dumb to me.
Comment #40
dawehnerThanks for pointing out the problem with that approach. It feels like we need some kind of way to disable hooks in places like that, but yeah this is no actual good solution. Some other idea could be to introduce another step before create, called rawCreate or anything else.
Comment #41
fagoentity_create() should always receive the bundle (see its docs), thus if that's not happening that's the bug. We might want to pro-actively check for that and through an exception if it's missing but that's another issue.
Comment #42
BerdirRight, which means this helper function is wrong and we need to add an argument for the bundle, check the bundle key and pass it correctly to create()
Comment #43
dawehnerYeah this was wrong.
Comment #45
BerdirOk, made the bundle optional (entities are not required to have bundles, which means that there is single bundle named after the entity type) and added a helper function to translate $vocabulary to the bundle name. Maybe we can standardize that later on but this is only temporary as we will need to solve id differently for the new routing system anyway.
This passed all taxonomy tests locall.
Comment #46
dawehneroh the { seems to be missing.
Should we return FALSE, explicit else?
Comment #47
BerdirForgot about this, that if actually shouldn't exist at all...
Comment #48
ParisLiakos CreditAttribution: ParisLiakos commentedi may be missing something, but after looking this after a while..why do we want this funtion, if it only passes the ->id() method? Since we pretty much know that all entities have id() method..why dont we pass the $entity_type (or subtype now) and then call ->id() inside
entity_page_create_access()
?eg sth like
Comment #49
dawehner@rootatwc
This feels really hacky, but if we do this here, we should make it really clear that we have an bundle_entity in that case.
Comment #50
BerdirThere is currently no requirement for the bundle types to be an entity themself. vocabularies are and node types will be soon but I'd say we should keep it as a string for now. It's a temporary thing anyway, access checks move to the container anyway for symfony routes.
Comment #51
dawehnerI agree
Comment #52
Johnny vd Laar CreditAttribution: Johnny vd Laar commentedI have used the patch from #47 to create a new "Create terms in vocabulary" permission in this issue:
http://drupal.org/node/1848686#comment-7208060
Comment #53
xjm#47: taxonomy-term-create-access-1862758-47.patch queued for re-testing.
Comment #55
ParisLiakos CreditAttribution: ParisLiakos commentedRepository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
#47: taxonomy-term-create-access-1862758-47.patch queued for re-testing.
Comment #56
ParisLiakos CreditAttribution: ParisLiakos commentedback to rtbc
Comment #57
xjmCan we get a followup for that?
Also, it'd be good to update the summary with the current issue status, including why we're making this change. And next time, please file a followup instead of reopening the original issue. It's really confusing for reviewers. :) It stopped this followup patch from maybe getting committed last week, in fact.
Comment #58
BerdirI'm adding such an exception in #1818560: Convert taxonomy entities to the new Entity Field API because I needed it to help with debugging cases where it was missing.
Comment #59
Johnny vd Laar CreditAttribution: Johnny vd Laar commentedPerhaps I'm misunderstanding this but here the permission is called "update terms in":
and here it's called "edit terms in":
shouldn't this be both update?
Or should I create a new issue for this?
Comment #60
Berdir@Johnny vd Laar: Hm, strange. Let's open a new issue for that, the patch in here already got derailed for weeks because of being a follow-up patch in an already commited issue that looks scary :) Sounds like we will also have to improve our test coverage if it's not failing at the moment.
Comment #61
Johnny vd Laar CreditAttribution: Johnny vd Laar commentedI'm adding better test coverage here:
http://drupal.org/node/1848686#comment-7261878
I'll open a new issue. (http://drupal.org/node/1966614)
Comment #62
webchickJess said she wanted to take a crack at this before commit.
Comment #63
xjmDon't hold this on me; we misidentified what this issue was (again, apparently, based on my comment in #57) because of the misleading title and reuse of the original issue for a followup. (A good time to get my input would have been back before the first commit in #38, but we have plenty of time before code freeze if something is incomplete.) :)
Comment #64
webchick#47: taxonomy-term-create-access-1862758-47.patch queued for re-testing.
Comment #65
BerdirI'm not sure why nobody wants to commit this, this was RTBC since over a month and it's blocking #1818560: Convert taxonomy entities to the new Entity Field API, so setting to major.
Comment #66
alexpottSmall nit...
Old way of accessing the container
Comment #67
BerdirRe-roll, setting back to RTBC if that's ok with you :) If not, bump it back.
Also removed a now bogus line in the docblock of that function as we now do support bundles, this was pointed out by @WimLeers in the taxonomy term NG review.
Comment #68
alexpottCommitted 17f4d14 and pushed to 8.x. Thanks!