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.
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
- Move the tests to
core/modules/edit/tests/Drupal/edit/Tests/Access
- 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"
Comment | File | Size | Author |
---|---|---|---|
#8 | missed_unit_tests-2108691-08.patch | 4.13 KB | Wim Leers |
#8 | interdiff.txt | 3.39 KB | Wim Leers |
#7 | 2108691-07-missed-unit-tests.patch | 2.59 KB | Gábor Hojtsy |
#6 | 2108691-06-missed-unit-tests.patch | 1.85 KB | Gábor Hojtsy |
#1 | 2108691-01-missed-unit-tests.patch | 1.85 KB | linclark |
Comments
Comment #1
linclark CreditAttribution: linclark commentedHere is a patch (from a paste by timplunkett) which tackles part 1, moving these tests to the correct directory.
Comment #2
Wim LeersUGH! 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!
Comment #3
Gábor HojtsyComment #4
Gábor HojtsyYeah 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!
Comment #6
Gábor HojtsyShould 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.
Comment #7
Gábor HojtsyOk, 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:
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.
Comment #8
Wim LeersThe
$empty_field
case was nonsensical, and due to the move fromfield_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 :)
Comment #9
Gábor HojtsyIndeed mocking get() to return the field looks to me one of possible good solutions. :)
Comment #10
Gábor HojtsyThis 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.
Comment #11
webchickWow, 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!
Comment #12
jessebeach CreditAttribution: jessebeach commentedremoved the sprint tag