Updated: Comment #N

Problem/Motivation

The two PHPUnit tests in Edit module do not run with the rest of the tests. This is because they are in lib/, when they should be in tests/. Additionally, they are currently failing when run.

Proposed resolution

  1. Move the tests to core/modules/edit/tests/Drupal/edit/Tests/Access
  2. Fix the failing tests. Berdir says "looks like they do tricks for field_access(), which no longer exists, so at least that will need to be updated now"
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

linclark’s picture

Status: Active » Needs work
FileSize
1.85 KB

Here is a patch (from a paste by timplunkett) which tackles part 1, moving these tests to the correct directory.

Wim Leers’s picture

Issue tags: +sprint, +Spark

UGH! Great find!

Strange that none of us in #2047659: Add test coverage for edit module access checkers — where these were introduced — noticed this :(

Thanks for pinging me!

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Yeah I just noticed this a couple days ago when reviewing another phpunit test elsewhere. I grepped and edit module seems the only one that has its unit test in the wrong place. I thought then it would be found here, but apparently not :D Thanks for the issue @linclark!

Status: Needs review » Needs work

The last submitted patch, 2108691-01-missed-unit-tests.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.85 KB

Should be moved to a slightly different place (/tests dir under the module) for the simpletest runner to find it as well. Like all other phpunit tests are in core.

Gábor Hojtsy’s picture

Ok, so there are no fails in EditEntityAccessCheckTest locally. The only fail in EditEntityFieldAccessCheckTest is related to the test with a *non-existent field*: testAccessWithNonExistingField. This will pass through the validateAndUpcastRequestAttributes() AFAIS because the field info does have the example field for the entity. *However* the mock entity does not actually have that field, so the current implementation of accessEditEntityField() that is being testing will barf out a method called on non-object:

  public function accessEditEntityField(EntityInterface $entity, $field_name) {
    return $entity->access('update') && $entity->get($field_name)->access('edit');
  }

So the question is whether this method should be more resilient and somehow work even if the field is not actually there, although the request upcasting should already fend those situations off? If this method should stay as-is then the mocks need to be more elaborate and actually add the field on the entity. Not sure how to do that due to my very limited phpunit experience...

Here is a minor update that adds more fault tolerance to this method. It will merely let the tests run without a fatal error. Not the suggested final solution necessarily.

Wim Leers’s picture

The $empty_field case was nonsensical, and due to the move from field_access() to $entity->get($field_name)->access('edit'), it was no longer $this->fieldInfo that was called when checking edit access for a field, but $entity.

So the solution is actually pretty simple :)

Gábor Hojtsy’s picture

Indeed mocking get() to return the field looks to me one of possible good solutions. :)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

This looks great. It did not keep *any* of my code additions (which merely helped to pinpoint the issue itself), so I should be fine to RTBC :) The mocking of field seems right. The code also removes the field_access() function which is not used anymore. The field_valid_language() is still used in the code, so needs to stay in tests.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow, great find. I wonder if there's some way to write a test for missing PHPUnit tests (wow, meta ;)), but I guess not really.

Committed and pushed to 8.x. Thanks!

jessebeach’s picture

Issue tags: -sprint

removed the sprint tag

Automatically closed -- issue fixed for 2 weeks with no activity.