Problem/Motivation

fieldable/isFieldable() in the entity type annotation/on EntityTypeInteface is misnamed. It is not about having fields, the point is being able to add/update/remove fields, like field.module does.

Same for the FieldableEntityStorageInterface,

Proposed resolution

Remove the flag completely. All content entities are supposed to support it if the storage supports it. The only thing that has to be specified is the field_ui_base_route, that tells field_ui where the integration point is for the Manage fields and related local tasks. The interface is renamed to DynamicallyFieldableEntityStorageInterface and also rename the corresponding FieldableEntityStorageSchemaInterface to DynamicallyFieldableEntityStorageSchemaInterface.

There are now 3 different things to check for, depending on what the code is doing:

1. Can have fields/is a content entity:

<?php
// On the entity. there is an issue that adds a FieldableEntityInterface.
$entity instanceof ContentEntityInterface.
// On the entity type. This is not great DX, but there is an issue for this too.
$entity_type->isSubclassOf('\Drupal\Core\Entity\ContentEntityInterface')
?>

Use case: Check if an entity type can have fields. Every content entity type has at least one base field, and config entity types do not have any fields.

2. Check if the storage allows for field (storages) to be added, removed, or updated.

<?php
$entity_manager
->getStorage($entity_type_id) instanceof DynamicallyFieldableEntityStorageInterface
?>

Use case: As a module storing data for other/all entity types, check which entity types that can be done for.

3. Check if field UI is enabled for a given entity type.

<?php
\Drupal::moduleHandler()->moduleExists('field_ui') && $entity_type->get('field_ui_base_route')
?>

Use case: Extend/integrate with field UI.

Additionally, the onBundleCreate(), onBundleRename() and onBundleDelete() methods are moved from DynamicallyFieldableEntityStorageInterface to a new EntityBundleListenerInterface, similar to EntityTypeListenerInterface. The entity manager now also implements the same interface instead of slightly different methods. The order of arguments has been adapted and changed.

The UI for view modes, form modes and comment type creation is limited to entity types that specify a field UI base route. This is done because those all require the field UI to be used (manage displays, create comment fields).

Remaining tasks

User interface changes

API changes

As described above.

Original report by @fago

The entity definition / entity info key 'fieldable' does not fit any more - all content entities have fields now. So we should rename this + fix the name of FieldableEntityStorageControllerInterface // the base class to match that. We do not have that distinction yet anyway though, but I think this is where "field_ui" should move in the long-run.

Comments

fago’s picture

We discussed configurable entity field storage in Prague. So what we dicussed here is that we want to work on we right now call "fieldable entity storage" to work with entity fields in general, i.e. any module can use the API to create storage for its own fields - not being field API / field.module fields.

Reasoning

  • Improve DX: we do not need to rely on configurable/Field API fields to implement business logic, e.g. organic groups could use field storage without using fields as UI.
  • Broaden the number of data having access to a swappable storage, e.g. Metadata module storing additional data for an entity. This means contrib modules can easily leverage field API’s storage mechanism and benefit from a queryable, revisionable and translatable storage (while they can decide to not go with field UI/formatters/widgets)
  • Support translatable storage of fields by default (currently only node does that), i.e. get translation support for taxonomy terms, …

So what we came up with related to 'fieldable' and the entity definition in general is the following:

  • The ‘field ui’ flag determines on whether the field-ui is enabled. yched pointed out this is not necessarily about configuring fields, but works for entities just having base fields but do not allow adding custom fields also. So it should be renamed, e.g. to ‘admin ui’.
  • fieldable changed as it does not refer to whether field API is supported by the entity type. What it means now, is whether the storage controller supports storing additional fields defined by modules. We agreed that there needs to be a flag in addition to the storage controller implementing the respective interface. The best name we came up for it is ‘extensible_storage’ - as it indicates whether the entity storage supports adding fields.
  • ‘configurable_fields’ is a new boolean flag and used by Field.module to know whether it shoudl support the entity type and by Field UI to know whether it is allowed to add fields via the UI.

Please read up on the meeting notes for other details on the idea. [TODO: create and link issue for other stuff]

amateescu’s picture

fago’s picture

Issue tags:+beta target

This is critical for understanding the new concept of "fieldable".

yched’s picture

Priority:Normal» Major

"beta target" -> bumping priority accordingly

fago’s picture

Let's get started and implement the suggestion of #1 and
- add the flag "configurable_fields"
- rename "fieldable"

Can we agree on to go with "extensible_storage" instead of "fieldable", or are there any better suggestions meanwhile?

fago’s picture

Also we've got FieldableEntityStorageControllerInterface and related base classes, so those would have to renamed as well, i.e. FieldableEntityStorageControllerInterface -> ExtensibleEntityStorageControllerInterface

andypost’s picture

Issue tags:+Naming things is hard

Suppose this should be Extendable because extensible more like "stretchable"

Otoh do we really need the flag "fieldable" suppose better to check the storage controller interface, because the storage implementation actually affects a "fieldability"

Berdir’s picture

Otoh do we really need the flag "fieldable" suppose better to check the
storage controller interface, because the storage implementation actually
affects a "fieldability"

No, that is not the same thing.

Just because the storage controller is capable of doing so does not mean that the specific entity type does.

If we rename the storage controllers, I'd suggest Content as a prefix, because that's what it is...

Alternatively provide a different storage controller for non-fieldable/extendable ones. DatabaseStorageController can *not* be used for content entities.

fago’s picture

Just because the storage controller is capable of doing so does not mean that the specific entity type does.

In particular, as the base class is able to do do you might want to inherit from it without enabling it.

If we rename the storage controllers, I'd suggest Content as a prefix, because that's what it is...

For the base class yes, but the interface is explicitly about the field API storage mechanism.

Suppose this should be Extendable because extensible more like "stretchable"

True, seems that extendable is what we want.

Berdir’s picture

We might want to rename the storage classes in #2188613: Rename EntityStorageController to EntityStorage because that issue is renaming every single one of them anyway.

andypost’s picture

Berdir’s picture

Issue tags:+Novice

Ok, we discussed that we will do the storage class renames either the related issue or somewhere else.

Which should make it possible to do this as a Novice issue.

Start with this:

- Rename "fieldable" to "extendable" in the all the entity type annotations and the documentation for it
- Rename isFieldable() to isExtendable(), update all usages.

Yorirou’s picture

Assigned:Unassigned» Yorirou
Yorirou’s picture

Assigned:Yorirou» Unassigned
Status:Active» Needs review
StatusFileSize
new39.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,617 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work
sriharsha.uppuluri’s picture

Assigned:Unassigned» sriharsha.uppuluri

Fixing the failed test cases.

sriharsha.uppuluri’s picture

Status:Needs work» Needs review
StatusFileSize
new34.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,721 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new6.58 KB

Before creating the patch there are issues with simpletest/TestBase.php. Except those remaining are fixed.

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -91,7 +91,7 @@ class EntityType implements EntityTypeInterface {
        *
        * @var bool (optional)
        */
    -  protected $fieldable;
    +  protected $extendable;

    I think the description here also needs to be updated.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
    @@ -380,7 +380,7 @@ public function getPermissionGranularity();
        * @return bool
        *   Returns TRUE if the entity type can has fields, otherwise FALSE.
        */
    -  public function isFieldable();
    +  public function isExtendable();

    Same here.

Status:Needs review» Needs work
sriharsha.uppuluri’s picture

visabhishek’s picture

Status:Needs work» Needs review
StatusFileSize
new6.41 KB
new35.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,857 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

i am updating the patch as per #18. Please review.

Status:Needs review» Needs work
sriharsha.uppuluri’s picture

Assigned:sriharsha.uppuluri» Unassigned
martin107’s picture

StatusFileSize
new35.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,420 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

patch no longer applied, so reroll

martin107’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Berdir’s picture

When discussing Extensible vs. Extendable vs. Fieldable in the entity storage controller rename issue, @alexpott argued that fieldable made still more sense to him, as he interprets it as "Being able to add additional fields", which has the advantage that is more specific than just "extendable", which doesn't explain what is extendable exactly.

That's why we rolled back the Fieldable* interface and base class and it should be a tiny change to rename those, which we now should do here if we decide to go with the rename.

alexpott’s picture

@Berdir is spot on - what do extendable, extendible, or extensible say about how the storage is being augmented... what about augmentable :P

Fieldable has the advantage of using terminology people know - yes the entity already has fields but to me fieldable says I can add more fields.

yched’s picture

Not a big fan of the alternatives either, but : then you could have a "non-fieldable" entity type with fields ?

Berdir’s picture

We discussed this a bit in IRC, here's the log:

(10:42:06) berdir: alexpott: remember our discussion re Fieldable vs. Extendable vs Extensible? Can you read the discussion in https://drupal.org/node/2100343 and possibly better explain your opinion that I tried to do there?
(10:42:08) Druplicon: https://drupal.org/node/2100343 => Rename 'fieldable' key in entity definitions [#2100343] => 28 comments, 5 IRC mentions
(10:42:17) alexpott: oh dear
(10:43:26) alexpott: so your last comment is spot on
(10:43:29) alexpott: berdir: ^^
(10:44:14) alexpott: berdir: extendable extendible extensible all say nothing about how the thing is being augmented
(10:59:55) alexpott: berdir: what name do you think is best? Or are you way past the point of caring?
(11:01:39) berdir: alexpott: I'm not sure. I see your argument and I'm fine with keeping it, but I wondering if isFieldable() really explains what it means.
(11:02:01) berdir: alexpott: I actually used it incorrectly myself in https://drupal.org/node/2116363 ;)
(11:02:04) Druplicon: https://drupal.org/node/2116363 => Unified repository of field definitions (cache + API) [#2116363] => 66 comments, 16 IRC mentions
(11:02:21) alexpott: berdir: canAddAndRemoveFields() :)
(11:05:37) amateescu: supportsFieldStorage() ?
(11:06:17) amateescu: too bad the "extra fields" concept is already taken..
(11:06:32) amateescu: `cause this is exactly what we could use it for
(11:07:08) amateescu: or, maybe, hasFieldStorage()
(11:08:32) berdir: well, the base fields are field storage too :)
(11:09:33) alexpott: Flexible :)
(11:09:50) amateescu: didn't you all get so excited about that FieldStorage rename? :)
(11:10:03) amateescu: so I could've asked you the same thing then :)
(11:10:56) amateescu: flexible sounds good
(11:11:35) alexpott: Flexifieldable
(11:11:37) alexpott: :)
(11:11:39) amateescu: lol
(11:11:52) amateescu: MindBendable
(11:12:09) alexpott: in homage to https://drupal.org/project/flexifield
(11:15:08) alexpott: berdir: seriously. I think that having field in the interface name is a good thing since the whole things is about being able to attach and remove fields to the entity definition - right?
(11:15:29) berdir: yes
(11:16:25) alexpott: berdir: therefore since we truly do not have a better name fieldable is good for me - since nothing else is telling me in any way that this is to do with fields.
(11:17:30) alexpott: berdir: and we just need to document the distinction between something that is fieldable and the base fields that an entity definition has
(11:22:42) alexpott: msonnabaum: naming things is easy ^^ ? :)
(11:25:55) berdir: alexpott: something like extensible_storage, hasExtensibleStorage() might work?
(11:26:02) berdir: because that's what we're talking about
(11:26:09) berdir: not adding fields, but adding the storage for them
(11:26:56) alexpott: berdir: but you only need the storage because you're adding fields?
(11:26:59) berdir: We have methods like onFieldCreate, the storage doesn't create fields, only reacts to a field creation and does the necessary storage stuff
(11:27:11) berdir: yes, but the storage doesn't add/create fields
(11:27:22) berdir: the storage reacts to someone else creating fields
(11:28:31) alexpott: berdir: just semantically... but extensible storage does not suggest that... to mean it means the ability to add more storage
(11:28:40) berdir: it also doesn't have to involve the field_ui/field anymore with https://drupal.org/node/2144263
(11:28:40) Druplicon: https://drupal.org/node/2144263 => Decouple entity field storage from configurable fields [#2144263] => 16 comments, 3 IRC mentions
(11:29:12) alexpott: berdir: like raid 5 is extensible but not other raid configurations :)
(11:33:29) alexpott: berdir: EntityType::storageSupportsAdditionalFields()
(11:34:34) berdir: alexpott: looks like http://help.autodesk.com/view/RVT/2014/ENU/?guid=GUID-113B09CA-DBBB-41A7-8021-005663B267AE is using extensible storage in a somewhat similar way ;)
(11:35:29) berdir: and there is http://en.wikipedia.org/wiki/Extensible_Storage_Engine, although I'm not quite sure what's the  extensible part there :)
(11:39:10) ***alexpott shrugs - if everyone wants extensible or a variant - ok - I just think it is a bit meaningless. I'm sure the Ikea billy range of shelves is sold as extensible :)
fago’s picture

Fieldable has the advantage of using terminology people know - yes the entity already has fields but to me fieldable says I can add more fields.

To me Fieldable says I can put fields on it. As developer providing an entity type and putting fields on it saying it's not fieldable is a contradiction?

I've not idea whether extensible/extendable fits better, but I dislike keeping fieldable.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new40.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_extensible.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

ok, so here is a start to rename to "extensible". Based on above discussion it seems to be better suited than extendable.

However, when writing docs I figured that the "extensible" flag actually means "ui extensible" now as the interface already means that entity storage is capable of storing additional fields. So should $entity_type->isExtensible() just check the interface and "fieldable" become something like "ui_extensible"? -> This boils into #2226567: Rename 'field ui' to 'configurable_fields' and introduce a new 'admin ui' entity type property then.

sun’s picture

In light of that assessment, wouldn't $entity_type->isConfigurable() be more accurate?

andypost’s picture

+1 to Configurable content entities!

fago’s picture

Issue summary:View changes

$entity_type->isConfigurable() together with ExtensibleEntityStorageInterface does seem to fit rather well. However, formatters and widgets of *existing* fields could be still configurable, while you may not add new fields to a non-configurable entity type. Not sure that's reflected by the term?

yched’s picture

Hmm, dunno...

- ConfigEntities
- Content Entities
  - Configurable Content Entities
  - non-Configurable Content Entities

Unfortunate clashing IMO.
I don't think "adding fields to an entity bundle through the UI" can be seen as "configuring the Entity type".

fago’s picture

Good point. Any other suggestions?

What about "ui_extensible" along side with

public function isExtensible($ui = FALSE) {
  // $ui == FALSE -> check storage interface
  // $ui == TRUE -> check "ui_extensible" flag in addition
}
yched’s picture

TBH, I've somewhat lost track here, so let's see if I got the concepts right:

0) ContentEntities have fields. All of them. That's what they do.

1) A StorageController can support (or not) "dynamically adding new fields", such as (but not restricted to) ConfigFields.
--> notion of "extendable storage"

2) An entity type can support (or not) adding custom ConfigFields through the UI.
It if it does, it needs to use an "extensible storage" class.
If can also chose *not* to allow ConfigFields, even if the storage class it uses could support it.
--> this has to be a dedicated flag on the entity type, independently of the storage class the entity type uses.

3) Regardless of the above, any entity type can enable "out-of-the-box" UI screens provided by field_ui (which could be renamed entity_ui, but different story)
If it doesn't support ConfigFields, it gets "Manage Display" screens, which is still useful for non-ConfigFields
If it does support ConfigFields, it additionally gets a "Manage Fields" screen to add / configure / remove ConfigFields


If the above is a correct assessment, then my suggestion atm would be :
1) "extendable" seems fine to qualify storages
2) regarding an entity type supporting ConfigFields or not, we failed to find a concise / non-ambiguous name for the flag so far, so maybe we need to be litteral : 'supports_config_fields' / supportsConfigFields() ?
3) Allowing out-of-the-box UI screens requires the entity type to provide admin path/route info to attach those pages to, and this is in place already, not sure we need to change whatever there ?

Then again, it's entirely possible I'm missing some of the stakes here :-)

fago’s picture

The assessment is correct, yes.

However, regarding 2:

If can also chose *not* to allow ConfigFields, even if the storage class it uses could support it.

I do not think we really need a flag for that, do we? We need a flag for the having configurable fields in the UI, yes - but if the storage is extendable/extensible, why not have field module support it from API/config?

yched’s picture

[An entity type can also chose *not* to allow ConfigFields] I do not think we really need a flag for that, do we?

Well, that would be the equivalent of D7's 'fieldable' ? "if an entity type is not fieldable, D7 doesn't try to load or save configurable field data".

If for some reason an entity type sees fit to state "no admin-defined fields here, please", I don't think this should be enforced just by not displaying a UI. It should really be "do not look in config for field definitions at all".

We need a flag for the having configurable fields in the UI, yes - but if the storage is extendable/extensible, why not have field module support it from API/config?

The storage we ship in core and use for all content entities by default (ContentEntityDatabaseStorage) *is* extendable (i.e is *able* to support dynamic fields). A Mongo storage will be extendable too. So in practice all content entity storage are going to be "extendable" (which, BTW, makes me question whether we really need a term to qualify that).

So we can't say "an entity type supports configurable fields if it uses an 'extendable' storage", because all storages are going to be extandable, and so in practice no entity type could say "no ConfigFields please", which would be a regression compared to D7.

Or we'd need to make a separate, non-extendable version of ContentEntityDatabaseStorage (and same with MongoStorage, etc) which feels absurd. Better to have feature-rich storages, and let the entity type specify whether it actually wants to leverage the feature ?

andypost’s picture

@yched great arguments! otoh that means it would be hard to make performant entities to make them skip field hooks

fago’s picture

Well, that would be the equivalent of D7's 'fieldable' ? "if an entity type is not fieldable, D7 doesn't try to load or save configurable field data".

Yep, but different to d7 there is would be no extra weight any more as config fields go into the same field definition repository and are handled as any other extended field.

If for some reason an entity type sees fit to state "no admin-defined fields here, please", I don't think this should be enforced just by not displaying a UI. It should really be "do not look in config for field definitions at all".

Yeah, I'm not sure there is a use-case for having config but no UI for it, so I'd be fine with disabling it as a whole.

(which, BTW, makes me question whether we really need a term to qualify that)

Yeah, because being extendable is still optional and not every storage will be able to support it; e.g. when exposing read-only remote data you really don't want it.

otoh that means it would be hard to make performant entities to make them skip field hooks

which field hooks? ;-) There are no entity-level config field hooks that go beyond what we have for base fields - it's all unified!

yched’s picture

being extendable is still optional and not every storage will be able to support it; e.g. when exposing read-only remote data you really don't want it.

Ah right, forgot about read-only storages. But then that is a split bewteen read-only and read-write. Would it be safe to assume a read-write storage has to be extendable ?

fago’s picture

Would it be safe to assume a read-write storage has to be extendable ?

I don't think so, assume a web service with read/write - there is no way to extend this then.

andypost’s picture

As I remember right this was introduced for menu_links core case, so for now not sure a purpose.
As we suppose to clarify this why not make just a method with default implementation a-la public function isExtandable() { return self instance_of ContentEntityInterface;}

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

Caught up on the recent discussion here.

Originally I would have proposed to separate $entity_type->isExtendable() from $storage instanceof ExtendableEntityStorageInterface but #39 makes a compelling case to let entity types disable extendability even if the storage supports it. Thanks for that post @yched, that was very insightful.

If I judge the latest comments correctly, it seems we have an agreement between at least @yched and @fago here and the latest patch seems to follow along the same lines. I.e. simply rename isFieldable() to isExtendable() and replace usages. No introduction of instanceof checks.

I reviewed the patch and couldn't find anything to complain about. So tentatively marking RTBC. Sorry if I missed anything or if there are in fact still reservations against what I perceived as agreement.

yched’s picture

Well, current patch does $entity_type->isExtendable(), which didn't seem to raise concensus as being clear about what it means (see #28 to #37).

#39 proposed to
- keep "is extendable" to qualify EntityStorage classes
- use a litteral & explicit "supportsConfigFields()" for EntityType classes

Not that I'm married to it, just clarifying the basis of the recent discussion as far as I get it :-)

tstoeckler’s picture

Status:Reviewed & tested by the community» Needs review

OK, thanks for the clarification.

fago’s picture

ad #48 - so does that mean isExtendable() checks for the interface and supportsConfigFields() a respective config_fields = TRUE|FALSE?

on extensible/extendable - I've no idea which one fits better, based on berdir's discussions with alexpott I though "extensible", but I'd be glad to go with which of the two is right for people.

yched’s picture

Sorry for the delay, let that one slip through :-/

so does that mean isExtendable() checks for the interface and supportsConfigFields() a respective config_fields = TRUE|FALSE?

Sounds right to me ?

extensible/extendable

No strong opinion either, but I kind of agree with the remarks made earlier that "extensible" feels like "stretchable", so I'm temptatively +1 on "extendable".
Then again, those all came from non-native english speakers, so maybe we should let @alexpott decide (is "extendable" even a word ? ;-)

fago’s picture

Talked to alex_pott about it again, and he feels like "extendable" does not get it right as people might think "extending storage" be like adding additional data, i.e. entities. We could not come up with a better term though.

I'll think a little bit more about a good term, but if we are not able to come up with anything better I think we'll have to go with "extendable", as "fieldable" does not work anymore.

andypost’s picture

#39.2 said supportsConfigFields() but this is not only about flag in annotation, the entity class should implement ContentEntityInterface, is not it?

About #39.3 - entity type annotation should provide the annotation key with route name #2309187: Fix double-link-entry between Entity and Entity Type classes

So let's limit the scope by renaming:
1) annotations key to "field_ui_allowed" or 'field_ui_fields_allowed"
2) the isFieldable() to isFieldUIAllowed()

benjifisher queued 33: d8_extensible.patch for re-testing.

Status:Needs review» Needs work

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

Berdir’s picture

The question here is if there is a performance problem if we make the setting just about the UI integration. For loading, we have the entity cache now and for writing, it should probably not hurt too much.

If so, then I agree with @andypost, the easiest option would IMHO be "field_ui = TRUE/FALSE", would work quite well in combination with with "field_ui_base_route" that we have now. And I would actually suggest to *remove* any possibly misleading methods, and just go with get('field_ui') in the few places that would then still have to call it.

andypost’s picture

There could be another way - try to re-use "thirdPartyWTF" trait somehow

Berdir’s picture

Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new41.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

I don't think that makes sense, that's about config entities not plugins/annotations.

Here's a first patch that renames it to field_ui, just trying to see what this would mean exactly..

yched’s picture

If we make it just about UI integration, then why do we need a field_ui = true/false flag, the presence/absence of a field_ui_base_route should be enough ? Having both would be redundant ?

Berdir’s picture

True :)

andypost’s picture

Title:Rename 'fieldable' key in entity definitions» Remove 'fieldable' key in entity definitions in favour of 'field_ui_base_route'
Issue tags:+Needs issue summary update

and summry needs clean-up

Status:Needs review» Needs work

The last submitted patch, 58: fieldable-field_ui-2100343-58.patch, failed testing.

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -544,9 +544,7 @@ protected function mapFromStorageRecords(array $records) {
     // Attach field values.
-    if ($this->entityType->isFieldable()) {
-      $this->loadFieldItems($entities);
-    }
+    $this->loadFieldItems($entities);

@@ -1119,7 +1117,7 @@ public function getQueryServiceName() {
   protected function loadFieldItems(array $entities) {
-    if (empty($entities) || !$this->entityType->isFieldable()) {
+    if (empty($entities)) {
       return;

Looks this not about UI but about ability to load/save configurable fields.
This means that some of isFieldable() should be changed to check the interface of the storage controller...

Berdir’s picture

StatusFileSize
new37.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,457 pass(es), 11 fail(s), and 1 exception(s).
[ View ]

Fixed the installer, removed field_ui. interdiff would be useless.

Berdir’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 64: fieldable-field_ui-2100343-64.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new41.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,658 pass(es).
[ View ]
new13.59 KB
  1. +++ b/core/modules/comment/comment.views.inc
    @@ -22,7 +22,7 @@ function comment_views_data_alter(&$data) {
       // Provide a integration for each entity type except comment.
       foreach (\Drupal::entityManager()->getDefinitions() as $entity_type_id => $entity_type) {
    -    if ($entity_type_id == 'comment' || !$entity_type->isFieldable() || !$entity_type->getBaseTable()) {
    +    if ($entity_type_id == 'comment' || !$entity_type->getBaseTable()) {
           continue;
         }

    add content entity check.

  2. +++ b/core/modules/comment/src/CommentTypeForm.php
    @@ -91,7 +92,7 @@ public function form(array $form, FormStateInterface $form_state) {
           $options = array();
           foreach ($this->entityManager->getDefinitions() as $entity_type) {
    -        if ($entity_type->isFieldable()) {
    +        if ($entity_type instanceof ContentEntityTypeInterface) {
               $options[$entity_type->id()] = $entity_type->getLabel();
             }

    use field_ui_base_route, because code.

  3. +++ b/core/modules/comment/src/CommentViewsData.php
    @@ -386,7 +386,7 @@ public function getViewsData() {
         foreach ($entities_types as $type => $entity_type) {
    -      if ($type == 'comment' || !$entity_type->isFieldable() || !$entity_type->getBaseTable()) {
    +      if ($type == 'comment' || !$entity_type->getBaseTable()) {

    @@ -465,7 +465,7 @@ public function getViewsData() {
         // Provide a relationship for each entity type except comment.
         foreach ($entities_types as $type => $entity_type) {
    -      if ($type == 'comment' || !$entity_type->isFieldable() || !$entity_type->getBaseTable()) {
    +      if ($type == 'comment' || !$entity_type->getBaseTable()) {

    same as above.

  4. +++ b/core/modules/field/field.module
    @@ -152,43 +152,38 @@ function field_system_info_alter(&$info, Extension $file, $type) {
    function field_entity_field_storage_info(\Drupal\Core\Entity\EntityTypeInterface $entity_type) {
    -  // Expose storage definitions for all exposed bundle fields.
    -  if ($entity_type->isFieldable()) {
    -    // Query by filtering on the ID as this is more efficient than filtering

    check storage?

  5. +++ b/core/modules/field_ui/src/Controller/EntityDisplayModeController.php
    @@ -24,7 +24,7 @@ class EntityDisplayModeController extends ControllerBase {
         foreach ($this->entityManager()->getDefinitions() as $entity_type_id => $entity_type) {
    -      if ($entity_type->isFieldable() && $entity_type->hasViewBuilderClass()) {
    +      if ($entity_type->hasViewBuilderClass()) {
             $entity_types[$entity_type_id] = array(

    @@ -47,7 +47,7 @@ public function viewModeTypeSelection() {
         foreach ($this->entityManager()->getDefinitions() as $entity_type_id => $entity_type) {
    -      if ($entity_type->isFieldable() && $entity_type->hasFormClasses()) {
    +      if ($entity_type->hasFormClasses()) {
             $entity_types[$entity_type_id] = array(

    check the route thing too.

  6. +++ b/core/modules/field_ui/src/EntityDisplayModeListBuilder.php
    @@ -92,8 +92,8 @@ public function render() {

    -      // Filter entities
    -      if ($this->entityTypes[$entity_type]->isFieldable() && !$this->isValidEntity($entity_type)) {
    +      // Filter entities.
    +      if (!$this->isValidEntity($entity_type)) {

    same.

  7. +++ b/core/modules/field_ui/src/Form/EntityDisplayModeAddForm.php
    @@ -49,7 +49,7 @@ public function validate(array $form, FormStateInterface $form_state) {
       protected function prepareEntity() {
         $definition = $this->entityManager->getDefinition($this->targetEntityTypeId);
    -    if (!$definition->isFieldable() || !$definition->hasViewBuilderClass()) {
    +    if (!$definition->hasViewBuilderClass()) {

    +++ b/core/modules/field_ui/src/Form/EntityFormModeAddForm.php
    @@ -19,7 +19,7 @@ class EntityFormModeAddForm extends EntityDisplayModeAddForm {
         $definition = $this->entityManager->getDefinition($this->targetEntityTypeId);
    -    if (!$definition->isFieldable() || !$definition->hasFormClasses()) {
    +    if (!$definition->hasFormClasses()) {
           throw new NotFoundHttpException();
         }

    same

Above review is fixed, the tests should also be passing.

Berdir’s picture

StatusFileSize
new56.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch fieldable-field_ui-2100343-68.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new18.4 KB

Renamed FieldableEntityStorageInterface to DynamicallyFieldableEntityStorageInterface and moved the onBundle* methods to a separate interface. They're not related to that.

Status:Needs review» Needs work

The last submitted patch, 68: fieldable-field_ui-2100343-68.patch, failed testing.

Berdir’s picture

Issue summary:View changes
Status:Needs work» Needs review
Issue tags:-Needs issue summary update
StatusFileSize
new56.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Re-rolled, updated the issue summary.

Status:Needs review» Needs work

The last submitted patch, 70: fieldable-field_ui-2100343-71.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new56.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,655 pass(es).
[ View ]
new1.55 KB

Checking the wrong interface.

effulgentsia’s picture

Issue summary:View changes
effulgentsia’s picture

Issue summary:View changes
Berdir’s picture

Issue summary:View changes
StatusFileSize
new60.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,657 pass(es).
[ View ]
new5.37 KB

Forgot about FieldableEntityStorageSchemaInterface.

Berdir’s picture

Issue summary:View changes
Berdir’s picture

Created change record: https://www.drupal.org/node/2346455

Berdir’s picture

StatusFileSize
new59.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,710 pass(es).
[ View ]
new4.2 KB

@swentel found a few issues:

- Some left-over debug()s
- Reference to FieldInstance, but HEAD was already wrong there in a different way
- "synced" the example in hook_entity_field_storage_info() with the real implementation, so the diff is smaller.

swentel’s picture

Status:Needs review» Reviewed & tested by the community

Looks good, thanks, should be green as they are only doc changes.

webchick’s picture

That middle hunk unfortunately doesn't look like docs changes so have to wait for testbot. :(

Berdir’s picture

That part is only a hook example, copied from field module. Changed it only to make the patch smaller.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed 6d27e11 on 8.0.x
    Issue #2100343 by Berdir, fago, martin107, visabhishek, sriharsha....
Berdir’s picture

Great! :)

Published [#2346455], will go through existing change records tomorrow, there's one about onBundle*() for example.

Status:Fixed» Closed (fixed)

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