[Followup] Implement entity access API for terms and vocabularies

Started 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).

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -21,6 +21,7 @@
+ *   access_controller_class => "Drupal\taxonomy\TaxonomyAccessController",

Ups!

#3: drupal-1862758-3.patch queued for re-testing.

Just a rerole against current version of core.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -394,14 +394,8 @@ function taxonomy_admin_paths() {
+function taxonomy_term_access($op, Term $term) {
+  return $term->access($op);

The user issue added a entity_page_access() access callback that should be able to replace the whole function :)

Good point!

Changed that.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -306,7 +306,7 @@ function taxonomy_menu() {
     'access callback' => 'taxonomy_term_access',
-    'access arguments' => array('delete', 2),
+    'access arguments' => array(2, 'delete'),

I guess you need to change access callback here as well

Besides that this seems ready

Also, seems class files are missing from the patch

Thanks rootawc, such small things could cause hours.

In general it probably helps to include the actual access controllers as well :)

This 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 :)

i think this issue will greatly help/simplify #1848686: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission)

i am gonna take care of #13

hrm..really i cant find any user_access call for taxonomy permissions:/
and i think my grep queries had nothing wrong

Oh great!

RTBC if bot agrees:)

Hm, well, there are user_access() "calls" in taxonomy_menu()

  $items['taxonomy/term/%taxonomy_term'] = array(
    'title' => 'Taxonomy term',
    'title callback' => 'taxonomy_term_title',
    'title arguments' => array(2),
    'page callback' => 'taxonomy_term_page',
    'page arguments' => array(2),
    'access arguments' => array('access content'),
    'file' => 'taxonomy.pages.inc',
  );

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.

Let's see whether this works already.

Opened 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().

Ah,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

+++ b/core/includes/entity.incundefined
@@ -749,3 +749,21 @@ function entity_query($entity_type, $conjunction = 'AND') {
+ * This access callback might not be enough depending on the entity type.

This needs an explanation berdir thinks and i agree. Its too vague now, can we add why?

What about that?

In 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?

This 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.

Can 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.

some more taxonomy_term_access callbacks sneaked in term translation UI test

#30: drupal-1862758-30.patch queued for re-testing.

It has been RTBC before. The last fail seemed to be yet another random test failure.

Just noticed that term access controller is named TaxonomyAccessController
I guess it should be TermAccessController

Oh right that's way better

We can simplify the term translation controller based on that.

Bot agrees, lets get this in

Looks great. Committed/pushed to 8.x.

+++ b/core/includes/entity.incundefined
@@ -742,3 +742,22 @@ function entity_query($entity_type, $conjunction = 'AND') {
+  $entity = drupal_container()->get('plugin.manager.entity')
+    ->getStorageController($entity_type)
+    ->create(array());

So 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:

      foreach (field_info_instances($this->entityType, $this->bundle()) as $field_name => $instance) {
        $field = field_info_field($field_name);
        if (field_is_translatable($this->entityType, $field) && isset($this->$field_name)) {

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:

  return $field['translatable'] && field_has_translation_handler($entity_type);

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.

Thanks 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.

+++ b/core/includes/entity.incundefined
@@ -742,3 +742,22 @@ function entity_query($entity_type, $conjunction = 'AND') {
+function entity_page_create_access($entity_type) {
+  $entity = drupal_container()->get('plugin.manager.entity')
+    ->getStorageController($entity_type)
+    ->create(array());
+  return $entity->access('create');

Right, 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()

Yeah this was wrong.

Ok, 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.

+++ b/core/includes/entity.incundefined
@@ -752,13 +752,24 @@ function entity_page_access(EntityInterface $entity, $operation = 'view') {
+  if (isset($definition['entity_keys']['bundle']))

oh the { seems to be missing.

+++ b/core/includes/entity.incundefined
@@ -752,13 +752,24 @@ function entity_page_access(EntityInterface $entity, $operation = 'view') {
   return $entity->access('create');

Should we return FALSE, explicit else?

Forgot about this, that if actually shouldn't exist at all...

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -369,6 +369,19 @@ function taxonomy_menu() {
+function taxonomy_term_create_access(Vocabulary $vocabulary) {
+  return entity_page_create_access('taxonomy_term', $vocabulary->id());
+}

i 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

function entity_page_create_access($entity, $entity_type = NULL) {
  $bundle = $entity_type ? $entity_type->id() : FALSE;
  // Do stuff
}

@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.

There 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.

I agree

I 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

#47: taxonomy-term-create-access-1862758-47.patch queued for re-testing.

Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].

#47: taxonomy-term-create-access-1862758-47.patch queued for re-testing.

back to rtbc

We might want to pro-actively check for that and through an exception if it's missing but that's another issue.

Can 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.

I'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.

Perhaps I'm misunderstanding this but here the permission is called "update terms in":

  /**
   * Implements \Drupal\Core\Entity\EntityAccessControllerInterface::updateAccess().
   */
  public function updateAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
    return user_access("update terms in {$entity->bundle()}", $account) || user_access('administer taxonomy', $account);
  }

and here it's called "edit terms in":

    $permissions += array(
      'edit terms in ' . $vocabulary->id() => array(
        'title' => t('Edit terms in %vocabulary', array('%vocabulary' => $vocabulary->name)),
      ),
    );

shouldn't this be both update?

Or should I create a new issue for this?

@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.

I'm adding better test coverage here:
http://drupal.org/node/1848686#comment-7261878

I'll open a new issue. (http://drupal.org/node/1966614)

Jess said she wanted to take a crack at this before commit.

Don'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.) :)

#47: taxonomy-term-create-access-1862758-47.patch queued for re-testing.

I'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.

Small nit...

+++ b/core/includes/entity.incundefined
@@ -774,13 +774,23 @@ function entity_page_access(EntityInterface $entity, $operation = 'view') {
+function entity_page_create_access($entity_type, $bundle = NULL) {
+  $definition = drupal_container()->get('plugin.manager.entity')->getDefinition($entity_type);

Old way of accessing the container

Re-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.

Committed 17f4d14 and pushed to 8.x. Thanks!