EntityResource: Provide comprehensive test coverage for FieldStorageConfig entity

Hey - I'm keen to help out with this. Just wondering, should I change it so the version is 8.4? I was reading through the allowed changes documentation and it doesn't seem like this fits any of the allowed changes:

criticals
non-disruptive bug fixes
non-disruptive contributed project blockers
API documentation improvements
patch-level library updates
issues for backport to D7 that meet the above criteria
Internal code cleanup that improves maintainability and is not disruptive may sometimes be backported to a patch release, but this is always at committer discretion. API and feature additions are not allowed, for compliance with semantic versioning.

...though i guess it might count as internal code cleanup?

I'm trying my best to understand this but I'm drawing a bit of a blank. Based on reading around the comments in this group of issues, what I understand is the following:

There are already a group of test classes, in the modules/rest/tests/src/Functional/EntityResource directory, that extend the EntityResourceTestBase class. The ones I can see are for Block, Comment, ConfigTest, EntityTest, MenuLinkContext, Node, Role, Term, User and Vocabulary.

What's missing are tests for - amongst other things - FieldStorageConfig.

So the task here is to provide tests - presumably following the pattern in the existing directories (that is to say, NodeJsonAnonTest, NodeJSonBasicAuthTest, etc), for FieldStorageConfig?

My second question is - I'm struggling a little with running the tests. I'd assumed you'd run the test files as follows:

From docroot/core run ../vendor/bin/phpunit modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php (for example)...

But when I do that I get the following error:

Class 'modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase' could not be found in '/var/www/site/docroot/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php'.

Am I missing something? Should I run the tests differently?

I'd be really grateful if someone could help me out!

#3: this should be solely adding tests. Adding more tests to prove correctness and prevent regressions is acceptable in 8.3.

#5:

1. Yes, this should be adding \Drupal\Tests\rest\Functional\EntityResource\FieldStorageConfig\FieldStorageConfigTestResourceTestBase, \Drupal\Tests\rest\Functional\EntityResource\FieldStorageConfig\FieldStorageConfigTestJsonAnonTest, \Drupal\Tests\rest\Functional\EntityResource\FieldStorageConfig\FieldStorageConfigTestJsonCookieTest and \Drupal\Tests\rest\Functional\EntityResource\FieldStorageConfig\FieldStorageConfigTestJsonBasicAuthTest
2. You're trying to run an base class (an abstract one even) that the concrete tests extend. Try running NodeJsonAnonTest instead. Or, if you want to run all tests in that directory, do
   

   php vendor/bin/phpunit -c core core/modules/rest/tests/src/Functional/EntityResource/Node
   

P.S.: to run all rest tests, do php vendor/bin/phpunit -c core --group rest. To run all hal tests, run php vendor/bin/phpunit -c core --group hal.

Great - thanks very much, Wim - running the tests like that works fine. And thanks for confirming my assumption about what needs to be done. I'll get started!

Thanks a lot, @jamesdesq :)

Hey - I'm trying to get a response out of the endpoint itself before I start testing, just so I can see how it's meant to work in real life. I'm not 100% what parameter should be passed to the endpoint - the UI just gives:

/entity/field_storage_config/{field_storage_config}

Looking at the data, it looks as though the field storage config is stored as follows:

collection: config.entity.key_store.field_config
name: uuid:edbad61b-637e-4515-80f7-24f82e26d9c4
value: a:1:{i:0;s:29:"field.field.node.article.body";}

i've tried passing both the UID and the name (i.e. field.node.article_body) as the parameter, and I either get an empty message, or the following message:

{
  "message": "The \"field_storage_config\" parameter was not converted for the path \"/entity/field_storage_config/{field_storage_config}\" (route name: \"rest.entity.field_storage_config.GET.hal_json\")"
}

Am I passing the correct parameter?

Thanks

James

Go to /admin/config/development/configuration/single/export. Select Field storage in the first dropdown. The values in the second dropdown list all field_storage_config IDs.

Alternatively:

SELECT * FROM config WHERE name LIKE 'field_storage.%'

Cheers Wim.

I've found the field_storage_config ID as you describe, but I'm still having some trouble with this.

I think I'm probably passing the credentials in the wrong way. I'm using Postman to make a request to /entity/field_storage_config/node.body?_format=hal_json. I've set the endpoint to accept basic authentication, and I'm passing the credentials in using the Postman basic authentication UI. I'm also using Content-Type: application/hal+json in the header.

I've put some breaks in the code, and I can see that where I'm having trouble is in EntityResource.php, where it looks as though $entity_access is being set to AccessResultNeutral, and so is returning an access error.

I can see that previous versions of RESTful defined CRUD permissions on the item in question, but that doesn't appear to be happening in 8.3. I can access, for instance, node resources via the API, and I can create content using the credentials I've provided.

I can also see in the access function in Drupal\Core\Entity the account parameter is null. Is this because I'm passing the credentials incorrectly in the get request?

Sorry for all the preliminary questions. I'll get there in the end.

OK, so it looks like I can get a lot further with the view entity, so I figure for the time being I'm going to put this one on hold and write tests for the view entity (https://www.drupal.org/node/2824572). Hopefully on the way I'll learn some more stuff about how the REST module works and I'll be able to come back to this one.

\Drupal\field\Entity\FieldStorageConfig has this entity type annotation:

 * @ConfigEntityType(
 *   id = "field_storage_config",
 *   label = @Translation("Field storage"),
 *   handlers = {
 *     "storage" = "Drupal\field\FieldStorageConfigStorage"
 *   },
 *   config_prefix = "storage",
 *   entity_keys = {
 *     "id" = "id",
 *     "label" = "id"
 *   },
 *   config_export = {
 *     "id",
 *     "field_name",
 *     "entity_type",
 *     "type",
 *     "settings",
 *     "module",
 *     "locked",
 *     "cardinality",
 *     "translatable",
 *     "indexes",
 *     "persist_with_no_fields",
 *     "custom_storage",
 *   }
 * )

As you can see, it specifies only one handler: a storage handler. This means it inherits the default access handler: \Drupal\Core\Entity\EntityAccessControlHandler.

The problem here is that basically nothing so far has been trying to view FieldStorageConfig entities.

In order for this to work, you'll need to create a new FieldStorageConfigAccessControlHandler. Which means this is not a Novice issue after all. Sorry about that!

1. +++ b/core/modules/field/src/FieldStorageConfigAccessControlHandler.php
   @@ -0,0 +1,33 @@
   + * Defines the access control handler for the field entity type.
   

   s/field/field storage config/

2. +++ b/core/modules/field/src/FieldStorageConfigAccessControlHandler.php
   @@ -0,0 +1,33 @@
   +class FieldStorageConfigAccessControlHandler extends EntityAccessControlHandler {
   

   Just change this to:
   class FieldStorageConfigAccessControlHandler extends class FieldConfigAccessControlHandler {}

   :)

Oops!

Changed class FieldStorageConfigAccessControlHandler extends EntityAccessControlHandler to class FieldStorageConfigAccessControlHandler extends FieldConfigAccessControlHandler (#15)

@brentschuddinck I'm afraid that's not what your patch is doing :) Can you post the right patch? Thanks!

+++ b/core/modules/rest/tests/src/Functional/EntityResource/FieldStorageConfig/FieldStorageConfigResourceTestBase.php
@@ -0,0 +1,104 @@
+      'id' => $this->entity->id(),

Can't we hardcode this?

Once this is fixed (or answered), this is RTBC.

np :)

Committed and pushed df2fd66 to 8.4.x and bfaa4f5 to 8.3.x. Thanks!

Committed to to 8.3.x because it is mostly adding test coverage. Yes there is a change to the entity access handling but calling the same access handler as for FieldConfig entities for FieldStorageConfig makes perfect sense and likely will help avoid access mistakes in the future. The lack of an access handler is a bug.

@jamesdesq - nice efforts on this issue and I hope you get the chance to learn more about the REST API. And thank you for explaining everything you were thinking on the issue. This really helps others follow along. Whilst I did not give you patch credit in the git log because you didn't contrib to the patch I am going to give you issue credit for the effort you took to document what you were trying and how you were learning along the way.

#27: RE access handler: exactly :)

#30: <3

Thanks guys - that's really nice to hear! I'm sorry I couldn't help more - I've had a really busy couple of weeks. Next time!

Unfortunately this should be rolled back. The choice to re-use the field config access handler is not correct for the delete case.

Therefore calling

$field_storage->access('delete');

Now yields:

Call to undefined method Drupal\field\Entity\FieldStorageConfig::getFieldStorageDefinition() in core/modules/field/src/FieldConfigAccessControlHandler.php on line 22

whereas it worked before.

Luckily neither our UIs nor Rest are calling this, but there is still the chance of contrib using this.

@tstoeckler nice spot thanks... obviously we're missing testing coverage of actually calling the delete via REST with the correct permission.

  /**
   * Tests a DELETE request for an entity, plus edge cases to ensure good DX.
   */
  public function testDelete() {
    // @todo Remove this in https://www.drupal.org/node/2300677.
    if ($this->entity instanceof ConfigEntityInterface) {
      $this->assertTrue(TRUE, 'DELETEing config entities is not yet supported.');
      return;
    }

So that's why we didn't get a test fail with this.

This still needs to be reverted in 8.3.x, as far as I can tell.

@tstoeckler it appears you put way too much faith in the git to d.o integration :) http://cgit.drupalcode.org/drupal/commit/?h=8.3.x&id=68571abcac07e7c6b20...

#34: Wow, great find!

#39: yeah, unfortunately commits scarily often are not mentioned on d.o issues :(

So… how do we add test coverage for something that we literally can't test? Functional test coverage for this is impossible, because deleting config entities is simply not yet supported. I'd say let's add unit test coverage, but \Drupal\field\FieldConfigAccessControlHandler also doesn't have unit test coverage. So… that's kind of sad, and out of scope to fix here :(

I started looking into this. And I noticed that FieldConfigAccessControlHandler was actually retrieving a FieldStorageConfig object to check whether it can/should be deleted. So… we can actually flip things around: we can move 99% of the logic in FieldConfigAccessControlHandler to FieldStorageConfigAccessControlHandler, and then let FieldConfigAccessControlHandler extend FieldStorageConfigAccessControlHandler, and let it call the parent implementation!

I think that addresses all concerns? Thoughts?

+++ b/core/modules/field/src/FieldConfigAccessControlHandler.php
@@ -2,32 +2,23 @@
+class FieldConfigAccessControlHandler extends FieldStorageConfigAccessControlHandler {
...
+    $field_storage_entity = $entity->getFieldStorageDefinition();
+    return parent::checkFieldAccess($field_storage_entity, $operation, $account);

Why are we not calling $field_storage_entity->access()? I think that would be more explicit and would allow us to not extend the field storage access handler. So if someone swaps that out things would still work.

#42: Field access != entity access.

EDIT: except that this is using entity access so you're totally right :) Rerolling.

Addresses #42.

Thanks, looks good to me!

Minor nitpick:

+++ b/core/modules/field/src/FieldStorageConfigAccessControlHandler.php
@@ -0,0 +1,32 @@
+  }
+}

Missing newline here.

#47: if you'd make this a unit test, then calling setLocked(TRUE/FALSE)->save() would work just fine.

See \Drupal\Tests\user\Unit\UserAccessControlHandlerTest for an example.

At first I was not planning to let this issue add test coverage. But with #47 already doing that in part, we might as well get it done.

No, you're not wrong. I didn't even know EntityAccessControlHandler had its own static cache. That's … a bit silly.

Static caches don't respect cache tag invalidation. Hence the need for that "rude hack" as you put it. It's really the only solution. Nothing "rude" about it :)

+++ b/core/modules/field/tests/src/Unit/FieldStorageConfigAccessControlHandlerTest.php
@@ -0,0 +1,168 @@
+    $this->anon
+      ->expects($this->any())
+      ->method('id')
+      ->will($this->returnValue(1));

The anonymous user is always user zero. So it's a bit strange to see 1 here. Fixed.

Also added a comment regarding resetting the static cache.

Still left to be done: unit test for FieldConfigAccessControlHandler. You'll be able to mostly duplicate \Drupal\Tests\field\Unit\FieldStorageConfigAccessControlHandlerTest :)

NW for Still left to be done: at the end of #50.

This looks great, thank you so much, @vaplas! This is very helpful :)

P.S.: this is definitely crossing the Novice boundary. Hope you're learning useful things here!

Don't know if anyone can help me out with this, but I'm having a heck of a time getting Xdebug working with PHPStorm and PHPUnit. What I want to be able to do is drop a breakpoint in the tests and debug them from there. I've got Xdebug working for the site itself and I can run the tests from within PHPStorm, but I can't seem to get debugging working. I've got Storm configured as described here (https://www.drupal.org/docs/8/phpunit/running-phpunit-tests-within-phpstorm).

Am I missing something obvious? Is that how you guys are doing it, or should I be doing it differently?

If anyone can point me towards anything that'll help me get set up I'd be really grateful.

Note these are not typical PHPUnit tests. They use PHPUnit as a test runner, but they're not unit tests, they're functional tests. They make requests etc. #2866056: ResourceTestBase should not have a timeout will probably help.

Thanks both. @wim-leers - that's an important distinction, and one which I hadn't really understood until you pointed it out.

If anyone else is following this thread and, like me, has failed to understand the difference between ordinary unit tests and functional tests, here is a useful tutorial:

https://www.drupal.org/docs/8/phpunit/phpunit-browser-test-tutorial

+++ b/core/modules/field/src/FieldConfigAccessControlHandler.php
@@ -18,16 +17,9 @@ class FieldConfigAccessControlHandler extends EntityAccessControlHandler {
+    /** \Drupal\field\FieldConfigInterface $entity */
+    $field_storage_entity = $entity->getFieldStorageDefinition();
+    return $field_storage_entity->access($operation, $account, TRUE);

I think this needs a comment about why delegating access control to the field storage is the correct thing to do. The new unit tests look great.

I think this needs a comment about why delegating access control to the field storage is the correct thing to do. The new unit tests look great.

Done.

Committed a6b84bf and pushed to 8.4.x. Thanks!

I think this should be backported to 8.3.x because adding the access handler is a bug fix. But cause of BC etc I want to get another committer opinion first.

diff --git a/core/modules/field/src/FieldStorageConfigAccessControlHandler.php b/core/modules/field/src/FieldStorageConfigAccessControlHandler.php
index ce783a6..da48556 100644
--- a/core/modules/field/src/FieldStorageConfigAccessControlHandler.php
+++ b/core/modules/field/src/FieldStorageConfigAccessControlHandler.php
@@ -12,7 +12,7 @@
  *
  * @see \Drupal\field\Entity\FieldStorageConfig
  */
-class FieldStorageConfigAccessControlHandler extends EntityAccessControlHandler  {
+class FieldStorageConfigAccessControlHandler extends EntityAccessControlHandler {
 
   /**
    * {@inheritdoc}
@@ -29,4 +29,5 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
     }
     return AccessResult::allowedIfHasPermission($account, 'administer ' . $entity->getTargetEntityTypeId() . ' fields');
   }
+
 }

Coding standards fixed on commit.

It'd be great if the issue handler can be updated so to make it clear why this became more than just adding tests.

Done.

What is the BC concern here for Drupal 8.3.x? That services accessing field storage config had full access before and now governed by permissions?

Looking at the code I don't think that there full access before - in fact I think everything would have been denied because in core the access check for field storages would return neutral.

>>> \Drupal\field\Entity\FieldStorageConfig::load('user.user_picture')->access('create');
=> false
>>> \Drupal\field\Entity\FieldStorageConfig::load('user.user_picture')->access('view');
=> false
>>> \Drupal\field\Entity\FieldStorageConfig::load('user.user_picture')->access('view label');
=> false
>>> \Drupal\field\Entity\FieldStorageConfig::load('user.user_picture')->access('delete');
=> false
>>> \Drupal\field\Entity\FieldStorageConfig::load('user.user_picture')->access('edit');
=> false

So actually I'm not sure there are any BC concerns here actually. And adding or changing access control handlers is explicitly exempt in the BC policy. Still given the complexities of BC judgements it'd be great if another reviewer can confirm my thoughts are correct.

That's correct: \Drupal\field\Entity\FieldStorageConfig did not specify an access handler, nor did it specify an admin_permission. Which means this code ran in \Drupal\Core\Entity\EntityAccessControlHandler:

  protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
    if ($operation == 'delete' && $entity->isNew()) {
      return AccessResult::forbidden()->addCacheableDependency($entity);
    }
    if ($admin_permission = $this->entityType->getAdminPermission()) {
      return AccessResult::allowedIfHasPermission($account, $this->entityType->getAdminPermission());
    }
    else {
      // No opinion.
      return AccessResult::neutral();
    }
  }

The middle return never ran. So always forbidden or neutral.

And:

  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
    if ($admin_permission = $this->entityType->getAdminPermission()) {
      return AccessResult::allowedIfHasPermission($account, $admin_permission);
    }
    else {
      // No opinion.
      return AccessResult::neutral();
    }
  }

The first return never ran, so always neutral.

Therefore indeed this should be safe: where access was never granted before, now it is, given sufficient permissions.

Thanks for confirming. Merged to 8.3.x.