Problem/Motivation

  • Right now fields are coupled to content entities, while there might be use-cases to have them on config entities as well. If possible we should avoid hard-coding fieldability on content entities and allow experiments in contrib.
  • Message module in d7 already provides a use-case + d7 implementation for fields on config entities.
  • Other capabilities of content entities are already implemented in separate interfaces, e.g. we've got RevisionableInterface and TranslatableInterface.

Proposed resolution

Introduce FieldableEntityInterface and make ContentEntityInterface extend it.

Remaining tasks

Do it.

User interface changes

-

API changes

- Mostly API additions (new Interface)
- It changes some methods to type-hint to FieldableEntityInterface instead of ContentEntityInterface, what is an API change for everyone overriding those methods. Those methods are:

  • EntityViewDisplayInterface::build()
  • EntityViewDisplayInterface::build()
  • FieldDefinitionInterface::getDefaultValue()
  • FieldStorageDefinitionInterface::getOptionsProvider
  • FieldItemListInterface::processDefaultValue()
  • CommentLinkBuilderInterface::buildCommentedEntityLinks()
  • CommentStatisticsInterface::create()
  • CommentStorageInterface::getNewCommentPageNumber()
Files: 
CommentFileSizeAuthor
#51 d8_fieldable.patch80.54 KBfago
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,622 pass(es).
[ View ]
#48 d8_fieldable.patch80.54 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_fieldable_13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#48 d8_fieldable.interdiff.txt3.46 KBfago
#43 d8_fieldable.patch77.31 KBfago
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,734 pass(es).
[ View ]
#43 d8_fieldable.interdiff.txt778 bytesfago
#42 d8_fieldable.patch77.31 KBfago
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,734 pass(es).
[ View ]
#39 d8_fieldable.interdiff.txt3.87 KBfago
#39 d8_fieldable.patch77.28 KBfago
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,730 pass(es).
[ View ]
#37 d8_fieldable.patch73.8 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,731 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#36 d8_fieldable-36.patch80.29 KBpgautam
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,730 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#33 d8_fieldable.patch73.81 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,720 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#33 d8_fieldable.interdiff.txt549 bytesfago
#31 d8_fieldable.interdiff.txt19.93 KBfago
#31 d8_fieldable.patch73.83 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#27 d8_fieldable.interdiff.txt3.96 KBfago
#27 d8_fieldable.patch67.28 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_fieldable_6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#25 d8_fieldable.patch65.84 KBfago
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,587 pass(es).
[ View ]
#21 d8_fieldable.interdiff.txt2.61 KBfago
#21 d8_fieldable.patch68.28 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_fieldable_4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#19 d8_fieldable.interdiff.txt7.83 KBfago
#19 d8_fieldable.patch69.53 KBfago
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,568 pass(es).
[ View ]
#16 d8_fieldable.patch58.56 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_fieldable_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 d8_fieldable.interdiff.txt9.99 KBfago
#14 d8_fieldable.patch58.56 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_fieldable_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 d8_fieldable.patch49.55 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_fieldable_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 d8_fieldable.interdiff.txt1.76 KBfago
#4 d8_fieldable.patch47.92 KBfago
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,871 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

Comments

Berdir’s picture

We would need to make it FieldableEntityInterface extends EntityInterface, because we can not just type hint on FieldableInterface in the relevant methods on the entity storage interface for example?

fago’s picture

Exactly, that is what I was thinking. There is no point in having FieldableInterface without an EntityInterface, so that's fine imho.

yched’s picture

+1, and also +1 on FieldableEntityInterface rather than FieldableInterface ;-)

fago’s picture

Title:Separate FieldableInterface out of ContentEntityInterface» Separate FieldableEntityInterface out of ContentEntityInterface
Issue summary:View changes
Status:Active» Needs review
StatusFileSize
new47.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,871 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

Let's see how that goes then.

Status:Needs review» Needs work

The last submitted patch, 4: d8_fieldable.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new1.76 KB
new49.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_fieldable_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
yched’s picture

+++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
@@ -0,0 +1,125 @@
+ * When implementing this interface which extends Traversable, make sure to list
+ * IteratorAggregate or Iterator before this interface in the implements clause.

Wasn't aware of that requirement - but is it still valid ? Coz it's not what ContentEntityInterface does now :-)

Otherwise, looks good.

fago’s picture

Wasn't aware of that requirement - but is it still valid ? Coz it's not what ContentEntityInterface does now :-)

I think it is, ContentEntityBase does - it's the implementation that has to do it.

yched’s picture

Status:Needs review» Reviewed & tested by the community

Makes sense. Thanks !

catch’s picture

Status:Reviewed & tested by the community» Needs review

Apart from initTranslation which has a @todo to remove, this is going to be an empty interface soon

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -70,7 +70,7 @@ public function view(EntityInterface $_entity, $view_mode = 'full', $langcode =
       $label_field = $_entity->getEntityType()->getKey('label');

I agree with splitting the interface out, but I'm really not sure about changing all these type hints. Are you 100% sure that all the places relying on ContentEntityInterface at the moment don't require Revisionable/Translatable compared to just Fieldable?

fago’s picture

Issue tags:+needs re-roll, +Drupalaton 2014

Yeah, I think we really should type-hint to what's really required, i.e. FieldableEntityInterface if that's enough. Else, implementing the interface would not allow you to use your entities with all that implementation what would render the interface useless.

I guess this needs a re-roll now.

fago queued 6: d8_fieldable.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 6: d8_fieldable.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new58.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_fieldable_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new9.99 KB

re-rolled and also updated the interface for the new default value method(s) of fields.

Status:Needs review» Needs work

The last submitted patch, 14: d8_fieldable.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
Issue tags:-needs re-roll
StatusFileSize
new58.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_fieldable_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

re-rolled.

fago queued 16: d8_fieldable.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 16: d8_fieldable.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new69.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,568 pass(es).
[ View ]
new7.83 KB

re-rolled patch and worked over latest additions.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -7,28 +7,14 @@
    /**
      * Defines a common interface for all content entity objects.
      *
    ...
      * @ingroup entity_api
      */
    ...
    +interface ContentEntityInterface extends FieldableEntityInterface, RevisionableInterface, TranslatableInterface {

    There seems to be basically nothing left here. Can't we add some description, that for example explains when you use this interface? Content, which should be stored in the database.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    --- a/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php

    +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -167,10 +167,10 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
    -  protected function invokeTranslationHooks(ContentEntityInterface $entity) {
    +  protected function invokeTranslationHooks(FieldableEntityInterface $entity) {

    Is there a possibility to rename the class or at least extract the fieldableEntity specific concepts out of it?

+++ b/core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteWidgetBase.php
@@ -219,7 +219,7 @@ protected function getSelectionHandlerSetting($setting_name) {
     $target_type_info = \Drupal::entityManager()->getDefinition($target_type);
-    return $target_type_info->isSubclassOf('\Drupal\Core\Entity\ContentEntityInterface');
+    return $target_type_info->isSubclassOf('\Drupal\Core\Entity\FieldableEntityInterface');
   }

Are you sure about this change? This method is used to identify whether an entity ID is numeric or not. Afaik this is unrelated to the concept of fieldability. Yes, this is a genera assumption of internal details, but the content entity interface seems to mach a bit better ...

fago’s picture

StatusFileSize
new68.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_fieldable_4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.61 KB

Thanks for the review!

There seems to be basically nothing left here. Can't we add some description, that for example explains when you use this interface? Content, which should be stored in the database.

Yep, I worked on the docs a bit and improved them - in particular I clarified when one would use content entity interface (always). The entity interfaces in general lack doc, but that should be dealt with in its own issue - so I just put the basic stuff there.

Is there a possibility to rename the class or at least extract the fieldableEntity specific concepts out of it?

Well, there is also translation in this class which is not in fieldableinterface. Given that, the conversion you quoted is actually wrong - I reverted this one. Generally, I think we should look into extracting fieldable parts into e.g. traits that match the interfaces, but given that's sometimes intertangled with translation or revision stuff it might not be always easy / doable. That said, that's something we should look at generally for entity classes, but this can be dealt with in separate issues and is not API changing as we can easily use the traits in the existing base classes.

Are you sure about this change? This method is used to identify whether an entity ID is numeric or not. Afaik this is unrelated to the concept of fieldability. Yes, this is a genera assumption of internal details, but the content entity interface seems to mach a bit better ...

True, reverted this one also.

fago queued 21: d8_fieldable.patch for re-testing.

fago’s picture

Issue tags:+beta target

adding beta target tag as I think it would make a lot of sense to have the interfaces settled then, even though this is not an API change.

Status:Needs review» Needs work

The last submitted patch, 21: d8_fieldable.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new65.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,587 pass(es).
[ View ]

Re-rolled patch, had quite some conflicts.

Berdir’s picture

Status:Needs review» Needs work
+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -189,10 +189,10 @@ protected function invokeTranslationHooks(ContentEntityInterface $entity) {
    * @param string $method
    *   The method name.
-   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
+   * @param \Drupal\Core\Entity\FieldableEntityInterface $entity
    *   The entity object.
    */
-  protected function invokeFieldMethod($method, ContentEntityInterface $entity) {
+  protected function invokeFieldMethod($method, FieldableEntityInterface $entity) {

This change is wrong, as it is about content entity storage and relies on TranslatableInterface.

Looking at the remaining uses of ContentEntityInterface:
- StringFormatterTest could use FieldableEntityInterface I guess but no biggie.
- EntityReferenceItem::schema() should go with FieldableEntityInterface I think?
- entity reference autocomplete widget has a pretty weird isContentEntity check, but I think changing that is out of scope.
- ContentEntityNormalizer could maybe just be about fieldable, not sure. Ok with doing that elsewhere too.
- CommentLinkBuilderInterface has a wrong docblock
- entity.api.php has a suboptimal reference, which assumes that there is nothing else than config or content entities, but not too problematic.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new67.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_fieldable_6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.96 KB

Thanks, fixed the invokeFieldMethod issue as well as the following two:
- StringFormatterTest could use FieldableEntityInterface I guess but no biggie.
- EntityReferenceItem::schema() should go with FieldableEntityInterface I think?

fago’s picture

Berdir queued 27: d8_fieldable.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 27: d8_fieldable.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new73.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new19.93 KB

Re-rolled and worked over recent additions as well. Attached is the merge-interdiff for those fixes also.

Status:Needs review» Needs work

The last submitted patch, 31: d8_fieldable.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new549 bytes
new73.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,720 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

ops - missed to adapt for quite important change :-) No complex data interface any more for fieldable entities.

Status:Needs review» Needs work

The last submitted patch, 33: d8_fieldable.patch, failed testing.

tim.plunkett’s picture

Issue tags:+Needs reroll
pgautam’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new80.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,730 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Rerolled Patch #33

1) Changed mock Test in EntityManagerTest.php
2) Changed entity.api.php file dependency from ContentEntityInterface to FieldableEntityInterface also make Drupal\Component\Utility\String dependency intact.

fago’s picture

StatusFileSize
new73.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,731 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

#36 does not account for the include computed change by #2346433: $entity iterators omit computed fields by default, so here is a re-roll which does. It does not fix the fail for me yet though :/

fago’s picture

@pgautam: Could you roll patch based on #37 with your other changes and provide an interdiff for that? That would help a lot to review them.

fago’s picture

StatusFileSize
new77.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,730 pass(es).
[ View ]
new3.87 KB

Somehow the reference in comment_entity_predelete() was changed back to ContentEntityInterface, fixed that. Attached patch also moves the new EntityAdapter to FieldableEntityInterface.

The last submitted patch, 36: d8_fieldable-36.patch, failed testing.

The last submitted patch, 37: d8_fieldable.patch, failed testing.

fago’s picture

StatusFileSize
new77.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,734 pass(es).
[ View ]

and re-rolled again for #2346635: Content entities are iteratable but do not implement Traversable - merge resolving commit interdiff attached. (duplicate)

fago’s picture

StatusFileSize
new778 bytes
new77.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,734 pass(es).
[ View ]

and re-rolled again for #2346635: Content entities are iteratable but do not implement Traversable - merge resolving commit interdiff attached.

fago’s picture

Issue summary:View changes
fago’s picture

Issue summary:View changes

Figured this represents an API changes for classes extending and overriding methods which get the new type-hint. Updating summary.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Still looks good to me. API Changes need to be signed off, but otherwise good to go.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
protected function getOptions(ContentEntityInterface $entity) {

In OptionsWidgetBase looks like it should be typehinted on FieldableEntityInterface

  /**
   * Gets the entity that field belongs to.
   *
   * @return \Drupal\Core\Entity\ContentEntityInterface
   *   The entity object.
   */
  public function getEntity();

In FieldItemInterface looks like it should be typehinted on FieldableEntityInterface

    if ($entity_type->isSubclassOf('\Drupal\Core\Entity\ContentEntityInterface')) {
      $this->setLastInstalledFieldStorageDefinitions($entity_type_id, $this->getFieldStorageDefinitions($entity_type_id));
    }

In EntityManager looks like it should be typehinted on FieldableEntityInterface

        if (!in_array('Drupal\Core\Entity\ContentEntityInterface', class_implements($entity_type_class))) {
          $this->propertyDefinitions = array();
        }

In EntityDataDefinition looks like it should be typehinted on FieldableEntityInterface

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new3.46 KB
new80.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_fieldable_13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks for the great review, fixed those.

plach’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 48: d8_fieldable.patch, failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new80.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,622 pass(es).
[ View ]

re-rolled.

plach’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed ba2b869 and pushed to 8.0.x. Thanks!

  • alexpott committed ba2b869 on 8.0.x
    Issue #2275659 by fago, pgautam: Separate FieldableEntityInterface out...
fago’s picture

Thanks, published the change record: https://www.drupal.org/node/2346875

Status:Fixed» Closed (fixed)

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