Updated: Comment #N

Problem/Motivation

The code is still based on the old assumption that fields can be re-used for other entity types and relies only on the entity keys and not the base fields.

Proposed resolution

1. Only check the entity type the field is attached to.
2. use \Drupal::entityManager()->getFieldDefinitions($entity_type) of the entity manager to check for a conflict with all base fields.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#10 interdiff.txt897 bytesswentel
#10 2199811-10.patch5.76 KBswentel
#5 interdiff.txt1000 bytesswentel
#5 2199811-5.patch5.76 KBswentel
#3 2199811-3.patch5.87 KBswentel
#1 2199811-1.patch5.84 KBswentel

Comments

swentel’s picture

swentel commented
Status: Active » Needs review
StatusFileSize
new 2199811-1.patch5.84 KB

Here we go. I've added a test to the Field UI tests because this also fixes an error in FieldOverview which would blow up very easily when trying to show the message with the error. Also cleaned up a leftover todo in FieldTypePluginManager along the way.

swentel’s picture

swentel commented

Ok, this needs a reroll now the field class rename patch is in of course.

swentel’s picture

swentel commented
StatusFileSize
new 2199811-3.patch5.87 KB

Reroll

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
@@ -14,8 +14,6 @@
 
 /**
  * Plugin manager for 'field type' plugins.
- *
- * @todo Add FieldTypePluginManagerInterface in https://drupal.org/node/2175415.
  */
 class FieldTypePluginManager extends DefaultPluginManager implements FieldTypePluginManagerInterface {

    Best forgotten @todo ever :) 

  2. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
@@ -306,13 +306,11 @@ protected function preSaveNew(EntityStorageControllerInterface $storage_controll
+    // Disallow reserved field names.
+    $disallowed_field_names = array_keys($entity_manager->getDefinition($this->entity_type)->getKeys());
+    $disallowed_field_names += array_keys($entity_manager->getFieldDefinitions($this->entity_type));
+    if (in_array($this->name, $disallowed_field_names)) {
+      throw new FieldException(format_string('Attempt to create field %name which is reserved by entity type %type.', array('%name' => $this->name, '%type' => $this->entity_type)));
     }

    Everything on a content entities is a field, so there can't be a key that is not also a field, so the first line shouldn't be necessary.

swentel’s picture

swentel commented
Status: Needs work » Needs review
StatusFileSize
new 2199811-5.patch5.76 KB
new interdiff.txt1000 bytes

Right, duh, where's my mind.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Now it looks great :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

5: 2199811-5.patch queued for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
@@ -306,13 +306,10 @@ protected function preSaveNew(EntityStorageControllerInterface $storage_controll
+    $disallowed_field_names = array_keys($entity_manager->getFieldDefinitions($this->entity_type));

We can use getBaseFieldDefinitions() here.

The last submitted patch, 5: 2199811-5.patch, failed testing.

swentel’s picture

swentel commented
Status: Needs work » Needs review
StatusFileSize
new 2199811-10.patch5.76 KB
new interdiff.txt897 bytes
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Thanks, back to RTBC.

alexpott’s picture

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

Committed 7099e22 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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