Remove 'fieldable' key in entity definitions in favour of 'field_ui_base_route'

"beta target" -> bumping priority accordingly

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"

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.

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.

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.

Fixing the failed test cases.

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

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.

17: drupal8-rename-fieldable-key-in-entity-definitions-2100343-17.patch queued for re-testing.

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

patch no longer applied, so reroll

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.

@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.

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

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: Remove 'fieldable' key in entity definitions in favour of 'field_ui_base_route' => 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: Unified repository of field definitions (cache + API) => 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: Decouple entity field storage from configurable fields => 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 :)

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

+1 to Configurable content entities!

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".

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 :-)

[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 ?

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

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 ?

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;}

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.

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 :-)

OK, thanks for the clarification.

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 ? ;-)

#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()

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.

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

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..

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 ?

True :)

and summry needs clean-up

+++ 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...

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

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.

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

Re-rolled, updated the issue summary.

Checking the wrong interface.

Forgot about FieldableEntityStorageSchemaInterface.

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

@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.

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

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

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

Committed and pushed to 8.x. Thanks!

Great! :)

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