Problem/Motivation

In #2831274: Bring Media entity module to core as Media module we want to use hook_entity_access to prevent that users can delete the source field of their media type.

While implementing that, we found this issue in the field ui. FieldConfigListBuilder::getDefaultOperations() is adding "edit" and "delete" keys in operations array, also when these operations are forbidden for the entity.

Proposed resolution

Add title attributes in the if clause

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chr.fritsch created an issue. See original summary.

chr.fritsch’s picture

Status: Active » Needs review
FileSize
1.5 KB

Here is the fix

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I assume this will pass the tests. Preemptively RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Looks like we need to add an automated test for this bug/.

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.47 KB
2.97 KB
2.65 KB

Here's a test. It will fail due to the bug causing malformed variables to be passed into template_preprocess_links(), but it squarely proves the fix, because the drop buttons on any Manage Fields page should never contain any empty links.

phenaproxima’s picture

Status: Needs review » Needs work

The fail patch didn't fail?

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
3.08 KB

Trying again. The fail patch definitely fails on my localhost, yet somehow it doesn't fail on Drupal CI...possibly due to a difference in error reporting.

Status: Needs review » Needs work

The last submitted patch, 7: 2836384-7-FAIL.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review

GOOD! That is exactly the failure I was getting, and it proves the fix. This is ready for review.

chr.fritsch’s picture

New patch that contains the fix and the test which fails in #7

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Munavijayalakshmi’s picture

Rerolled the patch.

phenaproxima’s picture

Status: Needs review » Needs work

Very close to done. The test needs a newline at the end of the file to appease the Git Gods, and it could do with a few comments to explain exactly what it's asserting. Otherwise, I'd mark this RTBC if I could but I contributed to the patch, so I can't :)

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
4.84 KB
1014 bytes

I fixed the newline issue and added some comments to the test.

chr.fritsch’s picture

I adjusted the patch and removed the assertions. They are not needed, because test would fail right after opening the page because of notices.

Status: Needs review » Needs work

The last submitted patch, 15: 2836384-15-FAIL.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

This looks good and makes sense. The fail patch pretty well proves the fix. And it's necessary :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed b13cdc6 to 8.4.x and 8854fe6 to 8.3.x. Thanks!

diff --git a/core/modules/field_ui/tests/src/Functional/ManageFieldsTest.php b/core/modules/field_ui/tests/src/Functional/ManageFieldsTest.php
index e27db93..1c21b06 100644
--- a/core/modules/field_ui/tests/src/Functional/ManageFieldsTest.php
+++ b/core/modules/field_ui/tests/src/Functional/ManageFieldsTest.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\Tests\field_ui\Functional;
 
-use Behat\Mink\Element\NodeElement;
 use Drupal\Tests\BrowserTestBase;
 
 /**

Fixed on commit

  • alexpott committed a12c564 on 8.4.x
    Issue #2836384 by chr.fritsch, phenaproxima, Munavijayalakshmi: Field...

  • alexpott committed 8854fe6 on 8.3.x
    Issue #2836384 by chr.fritsch, phenaproxima, Munavijayalakshmi: Field...

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

This forgot to remove media_entity_operation_alter() which has this comment:

/**
 * Implements hook_entity_operation_alter().
 *
 * Fix broken operations array in field UI for entities with restricted access.
 *
 * @todo This hook can be removed when issue #2836384 is done.
 * @see https://www.drupal.org/node/2836384
 */

Opened #2895857: Remove media_entity_operation_alter() as planned earlier for that.

Wim Leers’s picture

Issue tags: +Media Initiative