Problem/Motivation

After #2165725: Introduce hook_entity_operation() was committed, the field instance config entity operations turned out to be out of order.

Proposed resolution

Fix this by making field instance config entities re-use the EntityListBuilder functionality, instead of overriding it by using its own custom operations.

Remaining tasks

None.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#6 interdiff.txt7.05 KBtim.plunkett
#6 field_ui-2235347-6.patch9.63 KBtim.plunkett
#4 field_ui-2235347-4.patch4.37 KBtim.plunkett
#1 field_ui-2235347-1-PASS.patch4.45 KBtim.plunkett
#1 field_ui-2235347-1-FAIL.patch1.17 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
field_ui-2235347-1-FAIL.patch1.17 KB
field_ui-2235347-1-PASS.patch4.45 KB

Here's the fix. Not sure if we should fix the access controller here or not.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

seems legit

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: field_ui-2235347-1-PASS.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
field_ui-2235347-4.patch4.37 KB

Heh, base_path strikes again!

The last submitted patch, 1: field_ui-2235347-1-FAIL.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
field_ui-2235347-6.patch9.63 KB
interdiff.txt7.05 KB

Here it is with resolving the @todo about access.

Assuming the base_path fix was correct, I think this is done.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented 
  1. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php
@@ -23,7 +23,7 @@
- *     "list_builder" = "\Drupal\field_ui\FieldInstanceConfigListBuilder",

    Shouldn't the corresponding class be reomved as well, if this is no longer needed? 

  2. +++ b/core/modules/field/lib/Drupal/field/FieldInstanceConfigAccessController.php
@@ -0,0 +1,29 @@
+  protected function checkAccess(EntityInterface $entity, $operation, $langcode, AccountInterface $account) {
+    if ($operation == 'delete' && $entity->getField()->isLocked()) {
+      return FALSE;
+    }
+    return $account->hasPermission('administer ' . $entity->entity_type . ' fields');
+  }

    This looks good. I don't know if a 'view' operation even makes sense in this case, but maybe it makes sense to exclude that here? Not sure.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
+++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstanceConfig.php
@@ -23,7 +23,7 @@
- *     "list_builder" = "\Drupal\field_ui\FieldInstanceConfigListBuilder",

+++ b/core/modules/field_ui/field_ui.module
@@ -108,6 +108,7 @@ function field_ui_element_info() {
+  $entity_types['field_instance_config']->setListBuilderClass('Drupal\field_ui\FieldInstanceConfigListBuilder');

It's not removed, just moved to the right place (since it's field_ui, not field)

I'm not sure what it means to "view" a field... But its true that Views, DateFormats, FilterFormats, etc. all special case 'view' and return TRUE.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community

I think patch is ok with nice clean up so #7 is ans in #8 so setting it to RTBC.

  1. +++ b/core/modules/field_ui/field_ui.services.yml
@@ -4,10 +4,6 @@ services:
-  access_check.field_ui.field_delete:
-    class: Drupal\field_ui\Access\FieldDeleteAccessCheck
-    tags:
-     - { name: access_check, applies_to: _field_ui_field_delete_access }

    And this.

  2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldInstanceConfigListBuilder.php
@@ -62,29 +62,13 @@ public function getDefaultOperations(EntityInterface $entity) {
+    ) + $entity->urlInfo('field-settings-form')->toArray();

    Oh I like it.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Issue summary: View changes

I was thinking the operations are now out of order because the new situation also sorts them by label, but that is not the case.

Any way, I updated the issue summary, as this issue does a whole lot more than just fix the order.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed e51789c and pushed to 8.x. Thanks!

  • Commit e51789c on 8.x by alexpott: 
    Issue #2235347 by tim.plunkett: Field instance operations are out of...
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned

Status: Fixed » Closed (fixed)

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