Switch from Field-based storage to Entity-based storage

Didn't watch any video / slides from the Core Conv session yet, just browsed through the git branch.

Is this removing the ability to have different fields on the same entity / entity type use different storage backends ?

OK, so I guess this should be discussed :-).

This is not a minor change in the entity / field architecture.

- Allowing fields on a given entity bundle to come from different storage backends (possibly "remote") was one of the key design concepts from the very first "data api / fields in core" sprints 4 years ago. Moving back to "entity types are monolithic storage-wise" is a 180° turn.

Granted, I couldn't say for sure that he concept has lived up to its promises - at least in the visible contrib land; we haven't seen that many alternate field storage backends being published, and AFAIK the ones I'm aware of (mongo, riak ?) are document (entity-as-a-whole) oriented. But at least mixed storage is currently possible in D7, and it's hard to tell whether existing sites out there rely on that feature.

I understand that this design change is probably at the core of the "Entity as document / PHPCR" proposal (although is it that clear-cut ?), and that the promises there possibly outweight the flexibility loss, but this needs to be a conscious move.

This being said, I do need to watch that core conv video now...
[EDIT: aw, http://denver2012.drupal.org/content/open-standards-and-document-oriente... has the wrong video :-(]

- In the current architecture, a Field exists independently of any entity type. It knows where it is stored (in fact a Field is almost only that : "a data bucket somewhere"), and therefore you can create Instances of it in any entity type & bundle.
Stating that "a field is stored where the entity type is stored" means that a Field is created for a given entity storage backend, and can only have instances on entity types that use the same storage. Why not I guess, but right now I'm not exactly sure how this bubbles up.

crosspost

I understand that this design change is probably at the core of the "Entity as document / PHPCR" proposal (although is it that clear-cut ?), and that the promises there possibly outweight the flexibility loss, but this needs to be a conscious move.

Exactly. I mentioned that issue during the core-conversations. As at least all known field-storage implementations are working on the complete entity anyway no one had a problem with that. As it's going to make everyone's life easier when implementing remote-storage engines I think that's a good trade-off.
Personally, I like the "one entity type - one storage backend" approach as it conceptually does not make much sense to me to have an entity split between multiple storage backends. Also, it ensures that entities stay query-able as far as possible. Then, if you still want more storage backends, split your entity up - e.g. by using field-collection.

We also discussed the needs for cross-entity-type queries (joins?) which won't work with different storage backends. However, we can still support them by having multiple entity-types using the same storage. Whether we want or need to do that in core is another issue we'll have to deal with on the way.

Yeah things get extremely hairy and ugly when you have an entity with fields in multiple storages. You can't query, caching becomes tricky to say the least. I would say you want two entities in that case and relate.

The canonical use case we used back when we were originally designing what became Field API is a museum. You have Artwork entities in a legacy 3rd party data store of some kind, and wnat to extend it with comments and fivestar ratings in Drupal. While I have not done that recently, I DID build that site in Drupal 5 (sans the formal entity part, of course; we used a rather ugly bridge node design that I would rather not have to repeat).

So if we switch to per-entity storage, how would one implement that use case in a non-ugly way? If we can figure out a way to do that then I'm OK with this simplification.

So if we switch to per-entity storage, how would one implement that use case in a non-ugly way? If we can figure out a way to do that then I'm OK with this simplification.

I'd just go with a single "local" entity that also contains a reference to the "remote artwork entity". Then, the local entity could be used to display the data of both the local + the remote entity.
That way, modules would automatically treat the storage as what they are: split. So, loading, queries and caching would be separated - what makes much sense imo.

The problem is then creating the "local" entity on the fly and associating it. That's non-trivial right now, and the last time I did it (the aforementioned Drupal 5 site) I had to do a ton of should-be-unnecessary redirects.

For background, this is where I'm coming from: http://www.palantir.net/blog/remote-data-drupal-museums-and-web-2009 I in particular want to ensure that we still have a better approach for method 2 than what I describe in that article. Right now, per-field storage is that. If we switch to per-entity storage, I want to make sure that I don't have to fall back to what I describe in that article since a lot of work went into not having to do that anymore.

Hm. Damien, if I'm understanding you correctly then you're suggesting that per-field storage doesn't technically go away, it just moves to an "even more custom" entity storage driver, correct? And such driver could be written in such a way to allow runtime addition of fields that use local storage, and just does its own separate queries as needed?

Since I expect any such hybrid case to be fairly custom anyway I don't see that as a significant problem. If my understanding above is correct, then I have no objection to the refactoring suggested here.

Not strictly related but I guess most people here might be interested in giving their feedback in #1498634-7: [meta] Define revision/translation SQL schema for core entities (no patch) too.

The problem is then creating the "local" entity on the fly and associating it. That's non-trivial right now, and the last time I did it (the aforementioned Drupal 5 site) I had to do a ton of should-be-unnecessary redirects.

If that's desired, you can of course go with the remote entity by default and add in the data from the local entity if available only. No redirects or on the fly associations involved.

This suggested refactoring doesn't reduce flexibility, it just moves it from fields to entity types.

True - of course one can still implement multiple-storage backends in the entity controller, so the flexibility is not gone.

Implement an Entity Controller for your remote entity... and alter Entity Field Query of that entity to query the remote API directly. Doing so, you are basically going to implement something similar to the Entity Storage Interface that is suggested here.
Next, you are going to need to implement a Field Storage for your remote fields. It would probably need to be a NULL field storage, because the data has already been retrieved, right?

Yep, and in addition you'd have to alter the efq field-query support just as for the entity properties part. Yuck!

The point is we'll have single interface/controller for dealing with an entity's storage. So it will be easier to change (one thing to implement) and easier to use (one backend to talk to). Thus, this will make remote entity use-cases much simpler to do - not worse! :-)

1. No issue summary?
2. I wonder about the bird-level storage engine case with this. If entity (type) A and all of its fields, including an image field X, is stored in mongo, and entity B and all of its fields, including a/the same image field XY, is stored in SQL, then we're saying that XY can no longer be an instance of the same field as X, even though it has been before?

   How do we explain that differentiation to users? With regard to the Re-use existing field functionality? ("I want to re-use the image field I already have there, but... why doesn't it appear?) To be honest, the UX for users sounds most troublesome to me here.

   And how do we explain that differentiation to site/db admins? ("I want to find all entities that use image Z, but... oink?")

2. is indeed a revelant question, if the two image fields are not stored on the same backend, they are not the same field anymore because you cannot cross query entities from both storage at the same time (which we can with the actual field storage mecanism).

As I wrote in #7, this approach indeed means that a field is created for a given entity storage backend, and can only have instances on entity types that use that storage. Field UI would take that in consideration when presenting the list of "existing fields" that can be added, and field_create_instance() would raise an exception.

As sun points, this could mean some level of WTF ("why can I add this field from entity type B but not this one from entity type C ?"). Although, note we already have something along those lines currently : field definitions can (programmatically) state that they are only eligible for a given list of entity types - was added in D7 so that users don't have the confusing option to add the 'comment_body' field in nodes.

More radically, I think we could live with the limitation that fields can only be shared within a given entity type.

The inability to share fields between entity types with different storage sounds like quite small fallout compared to the other benefits this has. It's also something I can't see affecting very many existing sites.

Limiting fields within an entity type sounds potentially quite painful to me though. I'd think entity reference, relation and similar modules encourage cross-entity-type shared fields and I wouldn't fancy trying to write the upgrade path for it.

If we can figure out a good UI that makes it clear why certain fields are available only on certain entity types, I'd rather do that, although within-type fields is a fallback.

That said, let's also keep in mind that only a very small minority of sites are likely to have mixed-storage in practice. So we may be able to have extra UI information that only appears if more than one storage backend is in use, and just hides itself for the majority case where it wouldn't be relevant anyway.

Far from ready for human-review yet (the code has been moved around, but the API has not been cleaned-up).

Is this still the case, and if so, what if any help/feedback are you wanting at this time? Also, do you think this is close enough that we should try to finish it before continuing with #1498634: [meta] Define revision/translation SQL schema for core entities (no patch), or would you suggest work on that issue proceed independently?

This needs to be in the core issue queue.
It is frequently discussed and referenced, but hard to find.

Also needs an issue summary.

A big chunk of this work is the EFQ rewrite that that is almost ready, at #1801726: EntityFieldQuery v2.

Added a very brief (and potentially inaccurate) summary.

Resetting status since I'm guessing that the code will need to be redone from scratch (taking into account EFQ v2, the new entity controllers, etc)

About fields-are-only-shareable-within-entity-type:

> I'd think entity reference, relation and similar modules encourage cross-entity-type shared fields and I wouldn't fancy trying to write the upgrade path for it.

I can certainly speak for relation module: it defines an endpoint field type which is attached to the relation entity type and absolutely nothing else. Regardless, I think this issue should not introduce this change, it's radical enough that we say fields-are-only-shareable-within-storage.

Of interest : http://wunderkraut.com/blog/remote-entities-in-drupal-7/2012-10-25

I didn't have a closer look at the code yet, but couldn't we just

• define an EntityStorageExtendableInterface and move existing field-sql storage code to it
• move field-sql storage module functions into a ExtendableDatabaseStorageExtender class
• wire together extendable storage implementations for our fieldable entities

?

@fago: the hard part is that our current storage controller is mixing up entity-type specific lifecycle code (for example: Node wants to manage ->created and ->updated) and the actual storage specific lifecycle operation. This in essence makes the storage totally non pluggeable right now.

True. Still, once we've everything running through the storage controller, it's possible to do new entity types with any storage backend (including fields). Pluggability is more difficult, yep. I think it's something we should look at in a second step (if time permits...).

I can see entity-type specifc customizations like node created and changed to be moved to property/field classes. However, I see more the problem in stuff like applying indexes and storage updates in an storage-acgnostic fashion. For that to be solved it might require introducing another storage engine layer as you suggested already in some core conversation.

I'm finding it hard to follow what the current proposal/line of thought is, can someone update the issue summary?

Currently we create fields from field-types, and a field can be attached to different entity-types / bundles.

One possible approach: impose the restriction that a bundle can be made up of only those fields that rely on the same storage back-end.

That way all components of an entity are stored in the same back-end, which should simplify querying across all entities that are instances of a particular bundle.

I'm not sure how that would fit in with the philosophy of document-oriented storage back-ends such as Mongo.

Add basic summary.

Updated issue summary.

Updated issue summary.

I'm finding it hard to follow what the current proposal/line of thought is, can someone update the issue summary?

I've updated the issue summary based on my pov.

The question remaining is whether we should get started with this now or postpone it on entityNG supporting all fields. Once that has happend every field API field would be exposed as entity field allowing us to rely on entity field definitions for storage - instead of depending on field API $field arrays what would create a weird interdependency of field and entity systems.

Thus, I think we should postpone this on having every field API field exposed, what means we need to have all fieldable entites and all field types converted to EntityNG.

Related: #1893772: Move entity-type specific storage logic into entity classes

I am very wary about postponing this issue, given how late we are in the development cycle. Since Field API currently (AFAIK) just does its own thing via hooks, why can't we just go ahead with this implementation and say "yo, if you're not using the new API yet you don't get this, sucks to be you, time to convert!"? We know we want everything to convert to EntityNG sooner or later, don't we?

>yo, if you're not using the new API yet you don't get this, sucks to be you, time to convert!

hm, true. Well, we could leave the "old", existing field storage API in place while moving it over to converted-entities for. We can keep both in place until the "old" one is unneeded and can be removed. I like.

add lists

As posted on http://drupal.org/node/1949932#comment-7206860 we discussed this a bit on a hangout yesterday. I try to provide short summary of the discussion here.

This is a diagram of the architecture we discussed:

So the idea is basically to move the field-storage "plugin" interface to an ExensibleStorageInterface which is optional for an entity storage controller. The implementation could be moved to a separate helper class, which the entity storage controllers can re-use. There was an agreement that this idea is sound, however we miss someone who is in to take care of it.

Problem is, that this does not provide us with a crucial new feature as both field and entity storage is already pluggable - it's more a refactoring that makes things more sane and most notable storage engines easier to implement.

After the call I figured that real advantage would be that modules could re-use field storage without providing a configurable field (aka field API field) though, as already noted in the issue summary.

How does #37 relate to #1893772: Move entity-type specific storage logic into entity classes?

I would need a bit more instructions to start coding.

First the issue was stalled for two months now it is for ten days because while I would like to help, I have no idea how.

What do you think the API should look like? Some specs? Skeleton code? Something so that I can start coding.

Work is in http://drupal.org/sandbox/chx/1857558 branch entitystorage. The fundamental approach is that hooks are out, events are in and field storage is merely an event subscriber. When I am writing this, revisions and purge needs work, otherwise it seems to work.

There's not a lot of new code mostly it's ripping out the code from field.attach.inc supporting cross storage fields which are unqueriable anyways and then moving the code from the field sql module into a class so that the class can be a proper little subscriber.

Every entity storage will need to fire the entity.* events but not the entity.sql.* events. That's storage backend specific. If it's leaky abstraction, sorry about that. We can add different approaches, let's discuss.

Standardizing the events across storage engines makes sense. I don't have a fundamental objection to an individual implementation also firing additional events if it makes sense to, as long as it still fires all of the standard events in the right places. (I'm unclear on what an entity.sql.* event would do for me, but I don't at the moment have a conceptual objection to it existing.)

Event-driven approach to notify field-storage has fundamental difference because event subscriber able to stopPropagation() of the event that hooks don't

In context of "attach-api" this probably makes sense but could lead to state when entity get partial updates.

We can create an unstoppable eventdispatcher if that's desired. I am using the eventdispatcher because it allows me to create dependency injected classes. It's not yet 100% complete but the subscriber classes are getting close to pure unit testable -- I think Drupal\aggregator\StorageSubscriber already is (despite it's not yet finished). The node subscriber will be after #1921426: [Followup] Move node access storage to DIC. The menu link subscriber likely won't be in D8. User uses session and mail so that's also D9... but, you can write mock versions of these functions, for there are very, very few.

The controller classes will be unit testable the moment plugins can become dependency injected. There are no functions call there.

The controller classes will be unit testable the moment plugins can become dependency injected.

Controllers are not plugins :-), and it's still not clear whether they will be in D8. Making them dependency injected would be a different task story.

Oh right sorry. Anyways, they are not unit testable because we use "controller class" and not "controller service" but that's prolly a different issue, this issue is big enough...

While I'm not opposed to events in general, I'm not sure it's a good idea to do this now. So here are some questions/thoughts:

a) Why do we add in field storage in a modular way? I mean, that makes us support having multiple field storages? Why that? Couldn't we just off-load calls to $this->fieldStorage ?

b) Generally, this issues events for all CRUD operations what makes storage work modular. E.g. node storage additions are added in a separate class - what's per se nice - but we expose an API now what makes this possible for everyone.
But handling storage in a modular way is what makes things difficult and what the issue was supposed to do away with, such we've a single point that's in charge of storage. By exposing events and allowing anyone to take part, we lose control.

c) Similar to b), but another point. Imho we should move towards #1729812: Separate storage operations from reactions, but this unifies storage-insert with the possibility to react upon inserts. I'd rather do not expose "insert" for storage, so we can use it for reactions only. You can still add in a Field that has storage insert logic, if you really need to.

d) This aims for more that what the issue is about. While I'm totally not opposed to improving our storage logic, I think it would be better to try to separate tasks to get this issue done faster. I fear it becomes to much distracted with lots of general-storage related discussion else.

I don't meant to change priority.

The current branch has four things:

1. Storage controllers. As small as we can and needs to have interfaces. Like user needs to store roles and that's it (of course, in addition to normal entity storage duties).
2. Storage subscribers -- like calculating threads do not need to be in a storage controller, the storage controller needs to be able to read the maximum thread necessary but that's all.
3. Entity subscribers -- these were entity hooks before, hook_node_load, hook_entity_presave etc. You can inject stuff here.
4. Field storage as a subscriber.

We are changing 4. into a direct call from 1. per fago's request and we are agreed on the architecture of the rest.

ad 1) - yeah, so storage controllers are that part which are really in charge of storing things only. As discussed I'm not sure about separing role-storage methods, but I agree we want to keep the current logic/flow in this patch and do further cleanups elsewhere.

ad 2) Yes, separating this is a plus as discussed in #1893772: Move entity-type specific storage logic into entity classes. I think should try to move as much as possible from this logic to field classes though - like comment threading stuff. That's stuff for other issue also though and probably depends on #1969728: Implement Field API "field types" as TypedData Plugins to give is more flexibility here.

ad 3) Sounds and looks great. I'd love to see this become notifications only as by #1729812: Separate storage operations from reactions, but that's unrelated on events vs hooks.

ad 4) Yeah, let's let the entity storage decide which field storage gets used (and only one ..).

So this patch does a lot, and it cleans up a lot! Impressive work!

Now I remember why I wanted field storage to be a subscriber: ordering. If we move field storage out of the event system, where should the event fire before or after field storage is called? I recommend keeping it as an event. Currently there's no weight defined for it so it's 0 , you can weight your subscriber positive to fire before it, negative to fire after.

If we move field storage out of the event system, where should the event fire before or after field storage is called?

The event shouldn't be about storage, but about notifying. So neither before or after, field storage should be detached from events. E.g. we do not have preSave for storage. We've pre/post field storage hooks - but that's something different. Imho one storage event for maintaining any denormalized storage like the forum table should be enough, whether that's pre or post the other storage won't matter. $skip_fields I think we should drop - if it's a field, it get's stored.

As said, I'm not in favour of any storage events - but the possibility to maintain de-normalized stuff like forum does we need to keep. But we should document that this is for additional, thus de-normalized storage and not for supporting modular storage.

I'd see it that way:

save($entity):
 - notify: pre-save
 - (transaction start)
 - storage operation (ordering below is arbitrary)
      - entity base fields insert/update
      - just as other entity base fields (not necessarily in the base table), like user roles, term parents, ..
      - extended fields insert/update
      - notify: storage event
 - (transaction end)
 - notify: insert|update

IIRC $skip_fields was introduced to support sharding / denormalizing field storage.
I.e. let 3rd party modules store the data themselves and "steal" it from the regular storage engine.

The main use case was something à la http://drupal.org/project/pbs / "CCK D6 storage model", back when many people freaked out about the "each field in its own table" approach in field_sql_storage.

Pbs remained at the "proof of concept" stage, and to my knowledge no other notable contrib code leverages that, I'd think this can be removed / simplified.

#1982880: Refactor the entity system to be evented

I don't think that's necessarily duplicated. #1982880: Refactor the entity system to be evented could incorporate this one - (not sure if that's still the goal?), but doing this issue does not require us to do #1982880: Refactor the entity system to be evented. So let's keep this issue for tracking joint-storage success.

So here a short start on #37, so we have something more concrete to discuss.

- For invoking field storage we need to know the entity type now, so either we need to make fields per entity type or move calls over to instance CRUD.
- @todo: Figure out what to do with reamining hook implemenations of field-sql-storage.
- @todo: Fix moved field API attachers to use methods for invoking storage.

So what about this first step, move everything but keep it working as is. Then continue with refactorings later?

While remote entities which can not store configurable fields provide the use case for the interface, there is no use case for the separate class. Indeed, by providing a separate class we achieve little or nothing: we wanted to make sure that the whole of an entity is stored in one place and there's a single point of API entry point for every interaction (load, save etc) with it. I very strongly suggest to merge the field sql storage into the DatabaseStorageController and be done with it. If someone wants to write an entity controller which does something fundamentally different with the nonconfigurable properties then it's still possible to extend the DatabaseStorageController and override the load/save/etc methods while keeping the fieldSqlWrite etc methods in place. So there's nothing lost by merging in to one class while there is no win at all if that's not done.

we wanted to make sure that the whole of an entity is stored in one place and there's a single point of API entry point for every interaction (load, save etc) with it.

That's the goal of this issue, yes! I think we have already agreed on the most important part here.

While remote entities which can not store configurable fields provide the use case for the interface, there is no use case for the separate class.

ok, it's great to have agreement for the interface also, as discussed I still think separating the ExtensibleStorage code *internally* is useful. It helps to keep the code better separated and makes it easier to re-use the ExtensibleStorage from within your storage controller. Yes, we do not support having split storage, e.g. remote entity + local fields - but if you want to so you can still go for it and wire it together in your storage controller yourself. And yes, you are in charge of providing a suitable query class also. For that, it's also useful to have ExtensibleStorage logic implemented in a separated class.

Howsoever, I think that's an implementation detail and we've tougher issues to solve (see #58), so to move on I'd suggest to postpone this discussion for now, go with either approach and re-visit it once we've solved everything else. It's just a question of moving around code anyway.

> and makes it easier to re-use the ExtensibleStorage from within your storage controller.

And your storage controller does what? Well, I will frreely presume it stores the content somewhere which is not SQL. And then

> the whole of an entity is stored in one place

is violated. Decide.

> the whole of an entity is stored in one place

is violated. Decide.

the whole of an entity is stored via a single controller -> the entity controller. What this controller decides to implement itself is up to it - but sure, usually it's single storage.

> the whole of an entity is stored via a single controller -> the entity controller

Hey, that's what I suggested! You are suggesting using an entity controller and a separate ExtensibleDatabaseStorage class.

You are suggesting using an entity controller and a separate ExtensibleDatabaseStorage class.

That's how it is for the system, yes. How it implements stuff *internally* does not matter to the entity system though, so we decide to structure the code internally as it's useful for us while having the single-controller architecture in place.

We are running in circles. If you provide an ExtensibleDatabaseStorage it's no longer an internal matter. That is the problem itself: you insist it's an internal matter and I maintain that in order to make it an internal matter you need to move it in-class and make them protected methods. Otherwise another, non-SQL class could just use that to store things. We win nothing.

If I understand the problem space directly, it can be considered internal to the storage engine and still be a separate class as long as the storage engine is fronting for it as far as any users of the storage engine are concerned. That is, as long as users of the storage engine don't know the difference between it being internal or being a separate utility class, it's still "internal".

Whether or not it SHOULD be a separate class, I don't know enough about the details here to say. But a utility object does not necessarily break encapsulation here.

Updated issue summary.

In Portland we agreed on internal methods, one class. Progress is in https://drupal.org/sandbox/chx/1857558 branch 1497374. Tests need to be done, everything else should be. I hope a patch tomorrw.

This feels very important to me for a number of reasons so I suggest promoting this to critical.

This won't pass yet but fundamentally only the tests are left and so it's ready to review. The following files are material; the rest is not:

• core/lib/Drupal/Core/Entity/EntityStorageControllerInterface.php
• core/lib/Drupal/Core/Entity/EntityStorageControllerBase.php
• core/lib/Drupal/Core/Entity/DatabaseStorageController.php
• core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.php
• core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.php

The actual removal of field_sql_storage code can happen in a followup; it'd be just patch bloat.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -234,4 +237,311 @@ public function invokeFieldItemPrepareCache(EntityInterface $entity) {
+    // Invoke the hook.
+    module_invoke_all($this->entityType . '_' . $hook, $entity);
+    // Invoke the respective entity-level hook.
+    module_invoke_all('entity_' . $hook, $entity, $this->entityType);

deprecated $this->moduleHandler->invokeAll()

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -234,4 +237,311 @@ public function invokeFieldItemPrepareCache(EntityInterface $entity) {
+      $cache = cache('field')->getMultiple($cids);
...
+      foreach (module_implements('field_storage_pre_load') as $module) {
...
+        \Drupal::entityManager()
...
+      module_invoke_all('field_attach_load', $entity_type, $queried_entities, $age);
...
+          $instances = field_info_instances($entity_type, $entity->bundle());
...
+          cache('field')->set($cid, $data);
...
+    foreach (module_implements('field_storage_pre_insert') as $module) {
...
+    foreach (module_implements('field_storage_pre_update') as $module) {
...
+      cache('field')->delete('field:' . $entity->entityType() . ':' . $entity->id());

deprecated and needs injection

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -234,4 +237,311 @@ public function invokeFieldItemPrepareCache(EntityInterface $entity) {
+      cache('field')->delete('field:' . $entity->entityType() . ':' . $entity->id());

cache backed should be injected

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -234,4 +237,311 @@ public function invokeFieldItemPrepareCache(EntityInterface $entity) {
+    $values = $this->fieldValues($entity_id, $field, $instance);
+    foreach ($values as $value) {
+      $definition = _field_generate_entity_field_definition($field, $instance);
+      $items = \Drupal::typedData()->create($definition, $value, $field->id());
+      $items->delete();

move $definition out of loop

thanks for the quick review. I have converted file info to injection; doing more is way too much work and not necessary at all for a passing patch. Followup material. I will get $definition out of the loop.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.phpundefined
@@ -75,4 +75,14 @@ public function setStatus($status);
+  public function getExportProperties();

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -529,4 +529,11 @@ public function importDelete($name, Config $new_config, Config $old_config) {
+  public function storageType() {
+    return 'config';

Looks like awesome stub!!!

+++ b/core/lib/Drupal/Core/Database/Connection.php
index b647009..73469e1 100644
--- a/core/lib/Drupal/Core/Entity/DatabaseStorageController.php

--- a/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -558,4 +546,637 @@ public function baseFieldDefinitions() {
+          $entities[$row->entity_id]->{$field_name}[$row->langcode][] = $item;

Since most entity types have been moved to EntityNG, wouldn't it be wiser to move field storage code to NG in here? I know it's not easier, so just asking.. :)

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -558,4 +546,637 @@ public function baseFieldDefinitions() {
+    $load_current = $age == FIELD_LOAD_CURRENT;
...
+    $this->doFieldWrite($entity, FIELD_STORAGE_INSERT);
...
+    $this->doFieldWrite($entity, FIELD_STORAGE_UPDATE);

I guess these constants should be moved to EntityStorageControllerInterface.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -558,4 +546,637 @@ public function baseFieldDefinitions() {
+  public function handleUpdateField(Field $field, Field $original) {

Missing docblock.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -558,4 +546,637 @@ public function baseFieldDefinitions() {
+        throw new FieldUpdateForbiddenException("field_sql_storage cannot change the schema for an existing field with data.");

Stale comment.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -558,4 +546,637 @@ public function baseFieldDefinitions() {
+   * @param $field
+   *   The field structure.
+   *
+   * @return
+   *   A string containing the generated name for the database table.
+   *
+   */
+  static public function fieldTableName($field) {

Missing data types and type hint.

Same for a couple other methods below.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.php
@@ -194,6 +175,14 @@ class Field extends ConfigEntityBase implements FieldInterface {
+   * The storage type this field is in. Typically 'sql'.

Not sure "Typically 'sql'" adds any value here :P

+++ b/core/modules/field/tests/modules/field_test/field_test.module
index e5172b5..0000000
--- a/core/modules/field_sql_storage/field_sql_storage.info.yml

--- a/core/modules/field_sql_storage/field_sql_storage.info.yml
+++ /dev/null

I guess the gist of my comment is... Awesome!!

re @andypost: getExportProperties existed before but it was not clear at all that it was used as the actual list of properties being saved so I documented it.

re @yamateescu thanks for the review. I will see to the doxygen fixes. I am not sure on moving the methods to NG yet. We can do that later. Constants moving, sure, they need to. Not sure "Typically 'sql'" adds any value here :P -- I think it does because it's not quite explained wtf is a "storage type" so it's a good example.

Actually, disregard my comment about doing it the 'NG way' in this patch, we can't do that until *all* entity types are converted.

Not sure "Typically 'sql'" adds any value here :P -- I think it does because it's not quite explained wtf is a "storage type" so it's a good example.

If the purpose of it is to explain what's the deal with "storage type", I guess we can say something like: "Drupal core provides a basic 'sql' storage type by default, but contrib or custom modules can provide alternative storage types." .. or something along those lines.

To further the issue I am uploading a hopefully passing version -- note that it's made to pass by killing the FilledStandardUpgradePathTest -- but I wanted to solicit more reviews. I sincerely doubt any material changes will be necessary to get that test to pass. Documentation fixes are not yet in.

This should be passing all including FilledStandardUpgradePathTest -- keeping up with HEAD, it's hard.

Keeping up with HEAD.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -566,4 +551,637 @@ public function baseFieldDefinitions() {
+        ->condition($load_current ? 'entity_id' : 'revision_id', $ids, 'IN')
+        ->condition('langcode', field_available_languages($entity_type, $field), 'IN')

I like that 'IN' is specified even it is not needed as far as I remember, so it is easier to read.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -566,4 +551,637 @@ public function baseFieldDefinitions() {
+  /**
+   * {@inheritdoc}
+   */
+  public function storageType() {
+    return 'sql';
+  }

I am wondering whether it would make sense to have this as static method.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -566,4 +551,637 @@ public function baseFieldDefinitions() {
+        // If the database supports transactional DDL, we can go ahead and rely

it would be cool to not as an abbreviation here as it would really help people to understand what DDL could mean. (needed to ask wikipedia).

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -566,4 +551,637 @@ public function baseFieldDefinitions() {
+   * @return array
+   */
+  public static function fieldSqlSchema(Field $field) {

Would be nice to have a short description about the result of the method. Maybe you want to avoid that to not help people to use it.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -566,4 +551,637 @@ public function baseFieldDefinitions() {
+   * @param $field
...
+   * @return
...
+   * @param $name
...
+   * @return
...
+   * @param $name
...
+   * @param $column
...
+   * @return
...
+   * @param $name
...
+   * @param $column
...
+   * @return

Just a couple of places where a typehinting would be cool

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -104,21 +113,38 @@ class EntityManager extends PluginManagerBase {
+  /**
+   * Add more namespaces to the entity manager.
+   *
+   * @param \Traversable $namespaces

We should document why we need the addNamespaces method. For example: the uninstall methods for comment and taxonomy.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -104,21 +113,38 @@ class EntityManager extends PluginManagerBase {
+    $iterator = new \AppendIterator;
+    $iterator->append(new \IteratorIterator($this->namespaces));
+    $iterator->append($namespaces);

Nice!

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -153,6 +179,9 @@ public function hasController($entity_type, $controller_type) {
+    if (!$definition) {
+      throw new \InvalidArgumentException(sprintf('The %s entity type does not exist.', $entity_type));
+    }

+1 for the way better error message!

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -234,6 +237,26 @@ public function invokeFieldItemPrepareCache(EntityInterface $entity) {
+    }
+    // Invoke the hook.
+    module_invoke_all($this->entityType . '_' . $hook, $entity);
+    // Invoke the respective entity-level hook.
+    module_invoke_all('entity_' . $hook, $entity, $this->entityType);

@@ -255,4 +278,291 @@ protected function invokeTranslationHooks(EntityInterface $entity) {
+    $info = entity_get_info($entity_type);
...
+        \Drupal::entityManager()
...
+      module_invoke_all('field_attach_load', $entity_type, $queried_entities, $age);

As follow up we should inject the module handler in here to be able to run it properly, load entity info etc.

Hopefully this passes, I have added lots of doc fixes based on the review / discussion of amateescu and dawehner.

I have reverted the api.php removals, we can fix that later -- we will do that with field_sql_storage too, it just bloats the patch.

The patch is close enough that I think an Avoid commit conflicts tag is in order. It breaks all the time and very hard to fix. Not just the 2 fails above but since then I have *another* commit conflict (in less than half a day).

Urgh, $entity->getType doesn't return the entity type but $entity->entityType does? That's much fun!

tagging

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -234,6 +237,26 @@ public function invokeFieldItemPrepareCache(EntityInterface $entity) {
+    module_invoke_all($this->entityType . '_' . $hook, $entity);
...
+    module_invoke_all('entity_' . $hook, $entity, $this->entityType);

@@ -255,4 +278,291 @@ protected function invokeTranslationHooks(EntityInterface $entity) {
+      foreach (module_implements('field_storage_pre_load') as $module) {
...
+        \Drupal::entityManager()

Novice follow up (to be filled): Inject the dependencies into the base storage controller.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -255,4 +278,291 @@ protected function invokeTranslationHooks(EntityInterface $entity) {
+  protected function fieldValues(EntityInterface $entity, Field $field, FieldInstance $instance) {
...
+  public function fieldPurge(Field $field) {

Another place which should use the FieldInterfaces (see basically everywhere in the patch)

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerInterface.phpundefined
@@ -173,4 +175,86 @@ public function invokeFieldMethod($method, EntityInterface $entity);
+  public function handleFirstInstance(FieldInstance $instance);
...
+  public function handleUpdateField(Field $field, Field $original);
...
+  public function handleDeleteField(Field $field);
...
+  public function handleInstanceDelete(FieldInstance $instance);

This should use the FieldInterface and FieldInstanceInterface instead.

Novice followup (to be filled): Need docs for the parameters

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerInterface.phpundefined
@@ -173,4 +175,86 @@ public function invokeFieldMethod($method, EntityInterface $entity);
+   * @param $field
...
+   * @param $instance

Novice follow up (to be filled): Typehint entities.

+++ b/core/modules/field/field.attach.incundefined
@@ -324,8 +285,7 @@ function field_invoke_method_multiple($method, $target_function, array $entities
-        if ($grouped_items[$instance_id][$langcode][$id] !== array() || isset($entity->{$field_name}[$langcode])) {
-          $entity->{$field_name}[$langcode] = $grouped_items[$instance_id][$langcode][$id];
+        if ($grouped_items[$instance_id][$langcode][$id] !== array() || isset($entity->{$field_name}[$langcode])) {     $entity->{$field_name}[$langcode] = $grouped_items[$instance_id][$langcode][$id];
         }

You seemed to have killed the linebreak.

+++ b/core/modules/field/field.installundefined
@@ -90,6 +80,66 @@ function _update_8003_field_create_instance(array $field_config, array &$instanc
+      db_create_table($name, $table);

Novice follow up (to be filled):Why do we still use the wrappers even there are tons of issues everywhere to remove such stuff.

+++ b/core/modules/field/field.installundefined
@@ -90,6 +80,66 @@ function _update_8003_field_create_instance(array $field_config, array &$instanc
+function _update_8000_field_sql_storage_write($entity_type, $bundle, $entity_id, $revision_id, $field_name, $data) {

It is odd to have revision_id required here. For example block.install is calling it but it probably actually has no clue about a revision ID of a block. What about make revision_id optional and just remove all previous revisions if it is not set before?

+++ b/core/modules/field/field.installundefined
@@ -293,16 +338,14 @@ function field_update_8003() {
+      $field = new Field($config);
...
+        "field_deleted_data_{$record['id']}" => DatabaseStorageController::fieldTableName($field),
+        "field_deleted_revision_{$record['id']}" => DatabaseStorageController::fieldRevisionTableName($field),

I got confused by the fact that $field is never saved, but all we want is the schema for the update.

Readd the tags

adding followup

The actual removal of field_sql_storage code can happen in a followup; it'd be just patch bloat.

I'm unclear on this. Do you mean it just adds a bunch of "-" lines to the .patch file? If so, that's ok, let's do it. Or do you mean there's still code making calls to field_sql_storage.module? If that, then please update the issue summary or open a follow up issue explaining what that is and what needs to be done to fully remove it.

Fixed FieldInterface and FieldInstanceInterface in 33 cases. Commented field_update_8003, fixed the linebreak in field.attach.inc. _update_8000_field_sql_storage_write is copied verbatim and I do not think it should be refactored in this issue?

Re #92 please observe that field_sql_storage.info.yml is nuked and so there is absolutely no way anything making a call to field_sql_storage module would work because it is no longer recognized as a module. So yes, it would just add a lot of - lines.

This should pass. The interdiff is against the last passing / reviewed patch in #88.

Sorry for starting with trivial reviews so far rather than anything fundamental, but I'm still only slowly getting up to speed with the patch. I'll have very limited time for the next week, so please don't hold back an RTBC on my account, but I'll post small reviews as I'm able to.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
+   * @private Calling this function circumvents the entity system and is
+   * strongly discouraged. This function is not considered part of the public
+   * API and modules relying on it might break even in minor releases. Only
+   * call this function to write a query that \Drupal::entityQuery() does not
+   * support. Always call entity_load() before using the data found in the
+   * table.
+  static public function fieldTableName(FieldInterface $field) {

We don't yet have an @private annotation for Drupal core. I like the idea of introducing it and using it more broadly, not just the few places in this patch. However,
1. Should we rename it to @internal instead? That would match Symfony's convention.
2. Should we prefix the method name with a leading underscore, so it's easier to see when a caller is doing something questionable?
3. file.views.inc, image.views.inc, and taxonomy.views.inc call this private/internal method. Do we need a follow up issue to replace those use cases with something that's not strongly discouraged?
4. file.install and taxonomy.install have update functions that change the schema of their field type, and call this private/internal method to update existing data accordingly. Do we need a follow up issue to provide a proper API to do that?

Concerning @private (since that was my suggestion), @internal might work, i didnt know about that. The docs seem to be a bit different than what I've seen @private mean though:

http://manual.phpdoc.org/HTMLSmartyConverter/PHP/phpDocumentor/tutorial_...

vs

http://rubydoc.info/docs/yard/file/docs/Tags.md#private
http://usejsdoc.org/tags-private.html

Reading the links by msonnabaum in #100 it seems @private is more appropriate.

As for leading underscore, I have no preference. I guess yes, though.

Views integration is problematic! It is SQL bound and needs to reach this information but we can't put Views functionality on DatabaseStorageController, can we?

As for updates, the pox on updates. They do whatever needs to be done, there is no clean code requirement for them.

Some more doc and typehints nip and tucks. I hope this is not out of line for a critical issue.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -255,4 +278,291 @@ protected function invokeTranslationHooks(EntityInterface $entity) {
+  protected function doFieldInsert(EntityInterface $entity) {
...
+  protected function doFieldUpdate(EntityInterface $entity) {
...
+  protected function doFieldDelete(EntityInterface $entity) {
...
+  protected function doFieldRevisionDelete(EntityInterface $entity) {
...
+  protected function doFieldLoad($entity_type, $queried_entities, $age) {

docs missing.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.phpundefined
@@ -255,4 +278,291 @@ protected function invokeTranslationHooks(EntityInterface $entity) {
+

Extra space.

+++ b/core/modules/field/field.installundefined
@@ -65,10 +53,12 @@ function _update_8003_field_create_field(array &$field_config) {
+ *   TRUE ff this is the first instance of the field. Defaults to TRUE.

Typo it should be if.

+++ b/core/modules/field/field.installundefined
@@ -90,6 +80,66 @@ function _update_8003_field_create_instance(array $field_config, array &$instanc
+function _update_8000_field_sql_storage_write($entity_type, $bundle, $entity_id, $revision_id, $field_name, $data) {

Some typehints and docs missing.

Renamed the @private methods to _methods, addressed #102. Thanks for the reviews!

That is a quite important patch and opens the door for more improvements, like generate the tables automatically.

There might be some more docs issues, like using EntityInterface instead of \Drupal\Core\Entity\EntityInterface (see last interdiff.txt) but there will be more and more issues
which will deal with these points.

I'll have a look to this tonight, but if it is ready go on without me :)

We need a critical follow-up to convert the save logic to no longer use the BC decorator, but I agree that this should get in asap and then we'll *change* the code when we can get a useful interdiff out of it that can actually be reviewed.

FWIW, I'm back home and started reviewing this.

As a first reaction, as (I think) @eff above, I'd also vote to actually remove the code that's intended for deletion:
- storage hook docs in field.api.php - especially as some storage hooks are apparently preserved : hook_field_storage_pre_[load|insert|update]()
[edit: and then I guess strictly speaking they should move out of field.api.php, into - hmm, not sure where...]
- field_sql_storage code
- the former implementation of the field_test_storage in field_test module's field_test.storage.inc

Overall this looks very good to me, I found some minor stuff that it would be better to fix before commit. Patch attached. Below some questions.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -7,13 +7,15 @@
+use Drupal\field\FieldInfo;
+use Drupal\field\FieldUpdateForbiddenException;
+use Drupal\field\FieldInterface;
+use Drupal\field\FieldInstanceInterface;
 use Drupal\Core\Entity\Query\QueryInterface;
 use Drupal\Component\Uuid\Uuid;
 use Drupal\Component\Utility\NestedArray;
 use Drupal\Core\Database\Connection;
+use Drupal\field\Plugin\Core\Entity\Field;

I am really concerned by the amount of dependencies we are introducing between the entity system and the Field API module. I realize this might be needed atm, but I am wondering whether there is a way to exploit field definitions and the new field definition interfaces instead of directly referencing Field API classes.

Since our ultimate goal is not special-casing fields provided by the Field API module, I am wondering whether it makes sense to keep the distinction between configurable and non-configurable fields at storage level. This might be follow-up material, but I'd feel better if we at least had an answer to this concern before commit. And since we are past API freeze the less API changes are required to get this refactoring done, the better, hence maybe we should try to address this here if at all feasible in the current state.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -566,4 +552,648 @@ public function baseFieldDefinitions() {
+    $ids = $load_current ? array_keys($entities) : array();
+    foreach ($entities as $entity) {
+      $bundles[$entity->bundle()] = TRUE;
+      $ids[] = $entity->getRevisionId();
+    }

I've read these lines multiple times and I still don't get why they are working. An inline comment would be welcome.

--- a/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.php
+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.php

I am wondering wether it would be better to introduce a FieldableEntityStorageController class to avoid "bloating" the base storage controller with methods that are used only by content entities. We have a similar issue for the entity objects (see #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface), this solution would be consistent with the one proposed there.

Related to this, what about moving the Field CRUD code to a dedicated helper class? Right now we are loading in memory at every request code that will be used very few times. I know this is already happening in D7 but I see the potential for some optimization here.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerBase.php
@@ -255,4 +278,330 @@ protected function invokeTranslationHooks(EntityInterface $entity) {
+      module_invoke_all('field_attach_load', $entity_type, $queried_entities, $age);

Do we still need this, given that the other storage-related field attach hooks are gone?

Tagging as per explicit approval from @Dries in #68 above.

In New York, yched and me have agreed on me to remove the ability to share a field between entity types (Damien has also indicated this would be good quite some time ago) and we are working through the failures in the sandbox.

remove the ability to share a field between entity types

That will have a rather large impact on site builders.

For example, Views won't be able to filter or sort by a single field anymore.

For example, Views won't be able to filter or sort by a single field anymore.

#111 says "between entity types", not "between bundles". Does Views ever allow cross-entity-type queries?

Tag removal in #113 was unintentional; restoring them.

I know I've done it. I can't remember the use case, or if it was just academic...
Yes, across bundles is obviously essential.

What's the upgrade path for sites already using a field across two entity types?

What about duplicating fields and prefixing them by entity type?

Am I wrong or in Denver the conclusion was we would allow for cross-entity-type fields if they shared the same storage?

> What's the upgrade path for sites already using a field across two entity types?

Instead of having a single field attached to user and node they will have two fields of the same field name. Once you loaded your user or node, there is no change. If they use SQL storage: the table name changes -- it now includes the entity type but the upgrade path is very easy, it can be done in a single SQL INSERT-SELECT command which already is written.

@chx:

So we have fields with the same machine name and different ids?

> So we have fields with the same machine name and different ids?

$field->id() is now "$entity_type.$field_name" if that's what you mean.

Yes, basically a $field is now identified by [entity type, field name] (field_info_field() signature is changed accordingly)
Two fields with the same name on nodes and users are just like two different fields - storage location is different, we don't support cross entity type queries, and never have.
This is just a direct consequence of the "storage controller is in charge of storing the whole entity". Different entity types means different controller classes, means different "data buckets".

As @chx pointed in #122, this means
- the ConfigEntiy id() of a field is now "$entity_type.$field_name" (instead of "id() = field name" previously)
- we get back to a $field->name property that is just the field name.

That's an unfortunate back and forth dance after #1735118: Convert Field API to CMI that stated "$field['name'] in D7 is $field->id() in D8", but I actually think this is for the best. Base (non-config) fields have a 'name', and all the code that deals with config or base fields indifferently, adresses them by "name". So, consistency ++.

$field->id(), just like $instance->id(), is now just a CMI id that should only matter to very little code.

#124 makes perfectly sense to me too, nice solution :)

I totally second #123 + #124 as well!

Amazing this one is green. Here a review:

- storage hook docs in field.api.php - especially as some storage hooks are apparently preserved : hook_field_storage_pre_[load|insert|update]()

Looks like the hook is still around, but that does not make sense to me. If there is no field storage, there should be no pre-field-storage [load|insert|update] hooks. Can we remove them as we've done with other pre-field-API hooks? If we need a pre-storage hook it should be on the entity level.

* The type of storage.
*
* Drupal core provides a basic 'sql' storage type by default which stores
* every field in a separate table. Contrib or custom modules can provide
* alternative storage types.
*
* @return string
*/
public function storageType();

Oh, that's a useful addition/port!

+ * The storage type this field is in. Typically 'sql'.

I'm not sure why this still needs to be stored with the field - it has no choice anyway. I guess we could replace it with a method?

   * Allows reaction to the deletion of a configurable field.
   */
  public function handleDeleteField(FieldInterface $field);

  /**
   * Allows reaction to the deletion of a configurable field instance.
   */
  public function handleInstanceDelete(FieldInstanceInterface $instance);

I think it would be a great goal if we could manage to get field-API interfaces out here, but stick to entity-api interfaces only - so both APIs would be better decoupled/separated. That should be doable if we would move entity field definitions to a class implementing the field definition interface as well. What remains then is some usages of field instances, but those do not seem to be used a lot besides getting the field object from it anyway. Howsoever, I'll open a separate issue for the class based field definition point so we can re-consider that once we know more there. (updated: see #2047229: Make use of classes for entity field and data definitions)

Note: latest patch here is stale and doesn't reflect the new approach yet.
Notably, 'storage_type' is no more. Instead, $field gains an 'entity_type' property.

Current state of the patch with the new approach.
Not green yet, but should be reviewable.

Too many changes & rerolls for an interdiff ...

Will try to answer @plach #109 and @fago #107 shortly.

Side note: opened #2047777: Helper issue for "Switch from Field-based storage to Entity-based storage" to go wild with testbot runs.

Let's put the new approach in issue summary.

/me runs

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -528,42 +536,633 @@ protected function saveRevision(EntityInterface $entity) {
+      $query = $this->database->insert($table_name)->fields($columns);
+      $revision_query = $this->database->insert($revision_name)->fields($columns);

I may be missing something, but are we assuming that fields are always revision-ed even when the entity may not be? What about a SQL storage method that's only difference is to not use revisions, would they have to completely replace this method?

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.phpundefined
@@ -157,7 +158,7 @@ function addField($field, $type, $langcode) {
+        $sql_column = DatabaseStorageController::_fieldColumnName($field, $column);

@@ -242,12 +243,12 @@ protected function ensureEntityTable($index_prefix, $property, $type, $langcode,
+      $table = $this->sqlQuery->getMetaData('age') == FIELD_LOAD_CURRENT ? DatabaseStorageController::_fieldTableName($field) : DatabaseStorageController::_fieldRevisionTableName($field);

In the storage controllers above we use late static binding to make sure we get the correct implementation of _fieldColumnName(). Is called DatabaseStorageController directly here going to lead to an inconsistency?

+++ b/core/modules/field/field.views.incundefined
@@ -89,18 +113,22 @@ function field_views_field_label($field_name) {
+  $current_table = DatabaseStorageController::_fieldTableName($field);
+  $revision_table = DatabaseStorageController::_fieldRevisionTableName($field);

As above.

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.phpundefined
@@ -286,7 +287,8 @@ public function clickSort($order) {
+    $field = field_info_field($this->definition['entity_type'], $this->definition['field_name']);

As above.

Didn't man to remove tag, sorry!

I also think this issue has the potential to solve #2038707: Entity query sql backend limits storage controllers changes in contrib - we need to make sure efq's play nicely.

@rlmumford:

I think the issue summary could still use a refresh :)

Crosspost

#131

I may be missing something, but are we assuming that fields are always revision-ed even when the entity may not be?

I believe this is related to the D7 issue, where Field API always creates revision db field tables even though the nodes do not have revisioning turned on. This almost doubles SQL writes for nodes and decreases performance. More info at: #1279440: Deeply decouple revisions from Entity API and Node module
Another issue with a start on decoupling nodes and revisions: #120967: Separate revisions from node.module
Here is a case study that shows node write performance doubles using https://drupal.org/project/field_sql_norevisions Been a while since I've seen a patch that increases Drupal node write performance by 100%, but maybe this can be it :P

skipping revision writes: followup. We are changing enough here. We are changing what we must. That's still a 400kb+ patch...

> Is called DatabaseStorageController directly here going to lead to an inconsistency?

We could change static:: to self:: I do not care at all. Note that the Views integration will treat everything extending DatabaseStorageStorage as having the same structure, anyways.

Note that the Views integration will treat everything extending DatabaseStorageStorage as having the same structure, anyways.

This is where I think the previous storage-type, or a similar solution using an additional interface was better. Views should not depend on a certain implementation class to think this is sql-based storage, there should be an implementation in-dependent way to specify that. If it relies on additional methods also, then this should be an additional interface imho.

> Is called DatabaseStorageController directly here going to lead to an inconsistency?

We could change static:: to self:: I do not care at all. Note that the Views integration will treat everything extending DatabaseStorageStorage as having the same structure, anyways.

From a contrib point of view, to be able to "almost-do" something is often more unhelpful than not being able to do it at all. To write a new storage engine to find that some parts of drupal don't respect it is going to be a big DX issue surely?

It can't be that difficult to get hold of the storage class for the entity type your dealing with in these instances can it?

LOL #118: I have been arguing that fields should be local to an entity type. In most websites build that I participated recently we had a lot of stupid prefixing of field names by their entity types.

Yeah I've opened an issue about this #1945278: Allows fields to be stored by entity type instead of name..

And while speaking at stupid things we should also remove EntityInterface; successful frameworks i've seen do not dictact how to define an entity by extending base class or implementing interfaces.

Plain Old PHP Object are simple and effective!

Can we avoid pure and unrelated trolling in a complex issue already at 140 comments ? thanks.

I am completely sorry I thought we were speaking about API changes / improvements of the Field API... Let me bring some Ctools and Views fluff talk in my post so I will be not considered as a troll.

I noted some people saying

remove the ability to share a field between entity types

or Damien in #118

I have been arguing that fields should be local to an entity type. In most websites build that I participated recently we had a lot of stupid prefixing of field names by their entity types.

I am more than happy if this would be fixed and I proposed a solution in #1945278: Allows fields to be stored by entity type instead of name..

What do you think of this issue ? It is separate and does not require overhead in this discussion.

Sylvain, you are one of those people who have the excellent capability to derail / talk to an issue to death. Enough. Thanks.

add new follow ups

Updated issue summary.

This patch here implements the non-shareable fields. It includes two databases issues I linked from the issue summary which needs to get in first to make field_update_8006 doable -- it copies the data table structure and then copies the data with an INSERT ... SELECT *. Much thanks to amateescu and yched for getting this to green -- I only did a small part. We will clean uninstall up in the next few days -- either by the module disable removal issue getting in or storing the storage class and then doing something with entityManager to get that in there.

Now that we're green :-), I'll try to update the issue summary with the new approach and the API changes. Probably won't be before monday, though, so anyone welcome to beat me to it.
I'll also try to answer the comments received since #109 above.

One issue that we still need to resolve: table name length.
In the new code, a field is identified by [$entity_type, $field_name] rather than just a field name. So table names, that previously only included $field name as their "variable part", now need to include both information (current patch does "field_{$entity_type}__{$field_name}" / "field_rev_{$entity_type}__{$field_name}").
Problem:
- we hit "table name too long" much sooner
- that's two varying parts, so we can't solve this by just putting a harder limit on field names.

The current patch reaches green by patching a couple tests to just use shorter field names (simpletest makes things worse by eating 10+ chars just with the db prefix). We need to solve that more reliably though.

Optionally, we could consider switching to just "{$entity_type}__{$field_name}" / "{$entity_type}_rev__{$field_name}" pattern for table names, this saves up a couple more chars, and has a nice side effect of kicking phpMyAdmin's autofolding in the "list of tables" left column, + laying field tables next to each entity type base table. Whether we do this or not, it's obviously not good enough though.

Possible approaches I've been thinking about:

[edit: snip approaches 1) & 2), they involved more restrictive name lengths, which, as @chx points below, forbids an upgrade path]

Other than that, we need a way to generate a unique, (relatively) human readable string out of an arbitrary [$entity_type, $field_name] pair:

3) truncate and add numeric suffixes
Drawback: it makes the table names depend on CRUD history on a specific site instance (e.g "create field A, create field B, delete field A" & "create field B" result in different table names for field B.
So table names might end up being different on dev & prod, which feels bad - and would actually break since views CMI definitions currently include field table names (even though this specific point feels wrong from Views).

4) Truncate and add a short hash of the Field UUID
We already do something similar for "data tables for deleted fields" in current HEAD.
Could be something like:

$table_name = "{$entity_type}__${table_name}";
if (strlen($table_name) > 50) { // Keeps a 14 chars margin for db prefixes
  $table_name = substr($table_name, 0, 40) . '_' . substr(hash('sha256', $field->uuid), 0, 10);
}

Side note: opened #2057269: Exported CMI views definitions contain actual table names ? for the Views remark above.

Limited entity or field name lengths makes me fear that angry mobs in Austin will beat me over the head with "640k is ought to be enough for anybody" signs (although Gates denied ever saying that). Aside from small personal issues like that, the update will fail if we try to enforce new limits. And it's much more plausible that someone has a long entity type (just commerce_line_item is 18 characters). So obviously 4) is the only way forward with the only detail waiting for some delicious bikeshedding is whether we want a field_ (or even field_data_ just to make it more confusing!) prefix. Not employing that prefix, of course, has the chance of making the meaningful table length longer so I really like that and unless someone has a real compelling reason not to go with 4) as it is, I would like to close this discussion right here.

Doh, silly me. Stricter limits means no upgrade is possible, of course. Another novel I could have saved writing...
So yep, 4) it is.

Regarding "{$entity_type}__${table_name}" / ditching the "field_" prefix, I think I like that more and more as well. It does break the usual understanding of "prefix your tables with your module name", but this just means we use dynamic prefixes that other modules define... and the double underscore should safely avoid clashes. So IMO we can just go with it.
We can always reconsider in a followup, that would be a non API-breaking change.

4) sounds very good to me, with the new data/revision tables and the refactroing planned in #1998366: [meta] SQLite is broken, we might end up with the following:

node
node_revision
node_field_data
node_field_revision
node__body
node_revision__body

We are going to get inconsistent data/revision table couples in some situations, I guess:

node__very_long_field_name
node_revision__very_long_field_1234abcdef

- Reroll,
- fix code style remarks by @jibran in #2047777-65: Helper issue for "Switch from Field-based storage to Entity-based storage"
- rename Field::ID_MAX_LENGTH, the current limit is about field names again (and we still need one even if we truncate / hash table names),

Missed a change from #2054699: Ensure config entity id is set for computed composite ids when merging.
+ more code style remarks from @jibran in the helper issue

re @plach #109:

- generalizing to "non configurable fields" & FieldDefinitionInterface:
I wish we could go there, but honestly this feels both too much for this patch and really premature at this point.
The methods added to EntityStorageInterface are very much about reacting to the CRUD cycle of configurable fields - new $field created, $field updated, $instance deleted, $field deleted... Base fields have no observable CRUD cycle to react to, it's very unclear how housekeeping for fields defined in code would work.
+ base fields expose no schema for now, we don't have a unified mechanism to list all (base + configurable) fields of an entity bundle... Way too many unknowns for now.

- Code about $ids[] in doFieldLoad() :
Looks weird indeed... Will look into this

- FieldableEntityStorageController: yup, latest patch iterations have been doing that already

- hook_field_attach_load() being still around while field_attach_load() itself is gone.
Maybe @chx will have more insights on this.
The problem as I see it is that we have nothing in the current entity cycle hooks to replace this. It currently runs after values for configurable fields are fetched from storage, and before they are cached - and thus don't run again on next cache hit.
I guess that is more related to "what happens with field cache / entity cache", and so for now it is still needed.

re @fago #127

- hook_field_storage_pre_[load|insert|update]() still around:
Agreed, those do look weird now. They were kept because forum module uses them (insert & update).
I'll try to remove them.

Updated patch:
- Fixes for _update_8000_field_sql_storage_write().
- Reverts a needless change in FieldUpgradePathTest
- as per #154, cleans up code about $ids in doFieldLoad()

- Removes hook_field_storage_pre_*() as per #154
- Generates safe table names as per #146 option 4)
- Reverts tests that were changed to not generate long tablenames.
- Removes leftover 'storage' entries in field CMI files (tests, default config...)

More todos though :-(

- views integration needs more work, it's still based on the fact that a field can appear in several entity types, and the $sql_entity_types array we introduced earlier makes no sense anymore ("The array of SQL-based entity types where the field appears in" - a field appears in one single entity type, and that entity type uses DatabaseStorageController or not - hook_field_views*() need to run only if yes, and they operate on a single entity type).

- FieldInfo::getFieldMap() / field_info_field_map() still returns a list modeled after the old model:

field_foo:
  bundles:
    node: article, page
    other_entity_type: bundle_1, bundle_2
  type: image
field_bar:
  bundles: ...
  type: ...

That is very wrong, since field_foo as it appears in 'node' and 'other_entity_type' entity types are totally different fields with possibly different field types.

The field map needs to be keyed by entity_type first now:

node:
  field_foo:
    bundles: article, page
    type: image
  field_bar:
    bundles: ...
    type: ...
other_entity_type: 
  field_foo:
    bundles: bundle_1, bundle_2
    type: text

- $field->getBundles() still returns an array keyed by entity types, then bundles. The 1st level is not needed, will only ever be one entry. This change is not 100% strictly needed, but leaving it that way really gives the wrong idea about the field model now, so since we're already changing field_info_field() & friends, it's only consistent to change that too.
Also the implementation currently relies on FieldInfo::getFieldMap() above, so...
All code calling $field->getBundles() (or $field['bundles'] through the BC ArrayAccess layer) will then need to be updated accordingly.

I'm working on the first item (views integration) in the helper issue.
Help welcome for the other two :-)

I'm on number two - and quite possible on 3 as it's logical. (note, deleted the other comment first, should have edited, but oh well)

Cleaned up views integration.
Saving a testbot run here, it's green in the helper issue and swentel is going to post a patch for FieldInfo::getFieldMap() + $field->getBundles() soonish.

right, "no-no-test.patch"...
Manually cancelled testing then :-)

Refactoring of FieldInfo::getFieldMap() + $field->getBundles().

field_info_fields() cannot return a list fields keyed by field name anymore, there can be several fields with the same name in different entity types. Key by UUID like FieldInfo::getFields() instead.

Doh, forgot to upload the patch in #165.
The change is pushed though, and the patch will need a reroll after #2056133: Add db_copy_table_schema anyway - need to run, will do that later if noone beats me to it.

Thanks @chx

Missed one test assert in #165.
+ filter arrays 5.3 style, goddammit...

- Use table aliases instead of the real (truncated/hashed) table names in views data entries, so that they do not end up in views CMI files.
Lets us refine naming scheme later if needed without breaking saved views.
- This required some reorg in field_views_field_default_views_data(). Cleaned up while I was in there, that function has been through a lot of different hands since CCK D6 and is an awful mess in HEAD. All the lines in there are currently modified by the patch anyway, so this doesn't impact the patch size.
- Fixed broken join info (field type tables have no 'entity_type' column anymore, and do not require a join condition on it)

OK, I think we're almost there.

- I want to give a second look at the purge part - fieldPurgeData() / fieldValues() / fieldPurge. Something bugs me in there but I couldn't spend time on really nailing what so far. Will try to do that later today.
- Patch removes one stale test method in Drupal\field_sql_storage\FieldSqlStorageTest ("storage details"), but the rest of the class is still valid as tests for the code in FieldableEntityStorageControllerBase, so it needs to be moved to an active test class somewhere.
I'd be fine with pushing this to a followup to avoid patch size explosion if that's accepted, but it needs to happen, we don't want to lose that test coverage. Actually it would be good to do the job now to avoid bad surprises and make sure that the current code still passes, even if the actual moving around happens in an immediate followup.
- Fine with leaving dead field_sql_storage in place for now for patch size, but for the patch to be self consistent we need to at least remove the old hooks docs from field.api.php. Leaving those in there is really misleading for reviewers & for people using HEAD meanwhile.
- The issue summary still needs to be updated with a complete list of the changes (classes, interfaces, storage reorg, API changes...). Will try to do it later today too.

Reworked the purge area a bit:
- fieldPurge() was never called, the old hook_field_storage_*() hooks were called instead
- loading of data to purge produced invalid $items
- renamed a couple methods to be inline with the rest of the API (handleXXX(), doYYY()...)

Updated issue summary.

Updated the issue summary with the current approach and list of changes.

Updated for the current state of the patch

Oops, pasted too much stuff...

Reorg a bit

Adjustments

updating tags

typo

As per #173, final cleanup:
- Removed hook documentations
- Adapted tests that still made sense from field_sql_storage, moved them to Drupal\system\Tests\Entity\FieldSqlStorageTest
- Remove field_sql_storage.module.

In order to not further complicate reviews, two patches are provided:
- REVIEW-do-not-test.patch: the patch for review, without the cleanups above (i.e. same as #174)
- FULL.patch: the candidate patch for commit, including the cleanups above
- plus a REVIEW-to-FULL.interdiff.txt to show the difference between the two.

This is really great work, especially for updated issue summary!

Considering

Field API "fields" are now specific to an entity type, and can only be shared across bundles of a given entity type, but not across entity types.

I start to think, property -- 'entity_type' becomes unnecessary to field instance. It would be a duplicate property, since this property is the same shared with the field this instance built on.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/FieldSqlStorageTest.phpundefined
@@ -0,0 +1,426 @@
+    // Create a decimal 5.2 field and add some data.
+    $field = entity_create('field_entity', array(
+      'name' => 'decimal52',
+      'entity_type' => $entity_type,
+      'type' => 'number_decimal',
+      'settings' => array('precision' => 5, 'scale' => 2),
+    ));
+    $field->save();
+    $instance = entity_create('field_instance', array(
+      'field_name' => 'decimal52',
+      'entity_type' => $entity_type,
+      'bundle' => $entity_type,
+    ));

'entity_type' => $entity_type, seems duplicate for new instance imho:)

@smiletrl: no, entity_type is still needed in field instances, because both 'field_name' and 'bundle' are ambiguous by themselves.
There can be different fields with the same name in different entity types; there can be different bundles with the same name in different entity types.

What could be done here is to accept a 'field' value (with a $field object) in entity_create('field_instance').
Then you can simply specify a 'field' and 'bundle' and you're good. That's not completely trivial though, and is outside the scope of this patch.

Yeah, right, right. Sorry about this -- 'field_name' and 'bundle' are still ambiguous for an instance.

From the issue summary:

As was agreed with @chx,@Crell & @alexpott in NYCCamp mid-July 2013, a direct consequence of "the storage controller is in charge of storing the whole entity" is that Field API "fields" are now specific to an entity type, and can only be shared across bundles of a given entity type, but not across entity types. Different entity types means different controller classes, means different "data buckets".

What are the proposed benefits of this change? I'm asking because the patch in this issue appears to have been passing tests and even RTBC somewhere around comment #110, before this restriction was ever added.

We never supported cross-entity-type queries anyway, so this has no functional impact.
If anything, this gives site builders more liberty to name their fields, not stumbling on the same name being already used in a different entity type.
....
There can be two fields with the same name in different entity types, they're seen as completely different fields.

I disagree; I think there is substantial loss of functionality here.

In practice, fields can have many things "attached" to them, including theming (CSS based on the field name), business logic (code), and configuration set via the UI (e.g., think of Field Permissions as an example of mission-critical field-level configuration).

Before the patch, these all work very naturally; if you choose to reuse an existing field on a different entity type, all those things automatically come along with it.

After the patch, you can't reuse the field across entity types, but if you create a new field on a different entity type that has the same field name, you may inherit some of the attached behavior but not all of it. CSS/theming will be inherited. Business logic may or may not (depending on how the code was written). Configuration set via the UI presumably won't be.

So if you want the new field to behave the same as the equivalent field on the previous entity type, some of it might happen automatically (like it does now), but you might also have to rewrite some code or redo configuration by hand for the rest of it - a big loss in flexibility. And this is also very confusing; whereas the "reuse existing field" concept is very clear in the user interface, this would be more like "create a field with a particular string as the field name so that it can magically inherit some particular behaviors". Furthermore, since default machine names are based on the field label and there are a number of common field labels that sites often use ("Body", "Tags", "Image", etc.) there is a high chance of this happening by accident, when the site builder did not actually intend the fields to be related.

What are the proposed benefits of this change? I'm asking because the patch in this issue appears to have been passing tests and even RTBC somewhere around comment #110, before this restriction was ever added

That previous approach relied on brittle / unsustainable logic around "do those two entity types use the "same" field storage backend ?"
This just cannot fly, there's no "same field storage backend", there are just entity types and their - separate - storage controllers.
It was green because in practice our test suite didn't test the cases that would have been nasty.

This relied on storing "what 'family' of entity storage can you be attached to" (for some brittle definition of "family": 'sql', 'mongo'...) in the field CMI files - including some convoluted code flow that wrote this info back in the $field when the 1st $instance gets created, because you can only know that at the time you attach the first instance to an actual entity type, not at the time you create the field.
But we ship those in default configs. And the whole point of this issue is to make entity storage swappable (use mongo for nodes, for example). This directly conflicts: the storage used for an entity type is a runtime property, default config cannot assume / hardcode it.

This also resulted in much more complex and hard to predict logic around "which entity types can share fields, which cannot".

In short, this made the whole architecture really shaky - while there is no actual use case for "two fields in different entity types being the *same* storage bucket".

In practice, fields can have many things "attached" to them

Yes, they now need to be attached to [entity_type, field_name] separately, not just field_name. There should be no "magically sharing behavior because of same field name".
And true, CSS classes and field template overrides are an omission in this patch. They should now include the entity_type name.

#177 had a @todo left about testing "long table names".
Here it is (+ some type fixes).

This only affects the "FULL" patch, "REVIEW" patch is the same as in #177.

Thanks for that explanation. I guess I'd need to study this more to have a better perspective, but the way I'd have expected this issue to work is that the storage configuration would be entirely at the entity type level rather than the field level, and if the field is attached to more than one entity type, there's just more than one location in which the field data can be dumped into. So for a field shared between nodes and users (and if both of those entity types use the database for storage), instead of a single {field_data_some_field} table there would be two tables, {field_data_node_some_field} and {field_data_user_some_field}. Or if nodes and users have different storage then it would be in two different storage systems instead. But that doesn't mean that anything else about the field has to be separated out, so there would be no need to have restrictions on which types of entities a field can be shared with. It's not clear to me why that wouldn't work, but perhaps there is some reason I'm not seeing right now.

Yes, they now need to be attached to [entity_type, field_name] separately, not just field_name. There should be no "magically sharing behavior because of same field name".
And true, CSS classes and field template overrides are an omission in this patch. They should now include the entity_type name.

If the CSS/template issue is fixed and the "don't use field name alone in your code" rule is followed I suppose it's possible the magic sharing behavior would go away. But that does still mean the opposite, which is a big loss of functionality. Instead of just reusing a field when you want it to work the same way, you'd have to rebuild a duplicate field and possibly change your code. Given the explosion of entity types in Drupal 7 including some which can have fuzzy boundaries distinguishing them (e.g., nodes vs. fieldable panels panes) the ability to easily share fields between entity types is a major part of the architecture of many sites, and I think it would be a big loss.

If you do {field_data_node_some_field} and {field_data_user_some_field} then you already do not support sharing the field between entity types because it is not possible (OK, very hard but if one of them is not SQL then impossible) to retrieve all the field data in one query and so then why bother with pretending it's the same field? Indeed, if they are separate tables why they should be the same field at all?

I totally agree with @David_Rothstein that sharing fields is a valuable feature in D7. But as he points out, it's not so much the data sharing that is valuable (which is the part this issue makes impossible) but more the configuration sharing.

I think this problem could be solved in contrib, however, by providing something like 'field templates' that you can configure once in the void and then attach to different entities with the exact same configuration.

In fact, I've thought many times about writing such a module in D7 already because the same thing already happens for field instances. Often times a field will come with a myriad of instance settings which you often want to configure in the exact same way multiple times. A way to share (the configuration of (!)) instances is equally valuable sometimes, but completely impossible in D7.

So while it's true that this is a loss of functionality with just core, I think such a tool in contrib could fill this void quite well.

update remaining

Being the same, shared "field" has always been about being the same data set, and I'm really uncomfortable with the idea of reshuffling this at this point (one $field meaning different datasets depending on which entity type uses it)

And yes, sharing a field "just" to reuse configuration has always been a wrong case for field sharing IMO. This talks more about the lack of a "clone" functionality in the UI, similar to the one offered by views: create a *separate* copy, saving the hassle of filling the configuration forms again. In the absence of "clone", yes, sharing a field instead is very tempting, but this doesn't make it right.

Also note that the D7 behavior (the "namespace" of field names is "cross entity types") has also been noted as a pain point (see #118 above - "oh I can't name this field "field_the_name_that_fits', it's already taken by some other field in some other entity type"). As a direct example in core, the body field on comments had to be named 'comment_body' instead of 'body' because 'body' was already taken (and had a different definition) for nodes. Then we had to add an optional $field['entity_types'] property to avoid 'comment_body' showing up in the "reuse existing field" UI for nodes and vice versa...

Reroll.
Diff between REVIEW & FULL is the same as in #183.

"Cloning" wouldn't meet the same needs as field sharing since it doesn't address what happens when you want to change configuration after the field is already cloned... Nor does it go into your codebase and magically rewrite your code so that any behavior attached to the first field is applied to the cloned field as well :)

I think @tstoeckler's idea of field templates is interesting, though, and it could replace all the functionality that would be lost here. I'm still a little wary of removing a useful feature from core in favor of something that might theoretically exist in contrib (but doesn't yet), and also shared fields would still be around within an entity type so it might confuse people to have these two concepts "competing" with each other. Nonetheless, it could probably work.

The test coverage for field deletion must be... spotty :) I am not sure how yched rolls his review only patches but the changes are minimal. Thanks for removing getType, that was truly confusing, it's a fantastic API improvement.

Added entity_type in field CSS classes & theme hook suggestions.

More thoughts on "field name has a global meaning over all entity types" vs "field names are namespaced by entity types":
I really think that the latter makes more sense in terms of data modelling. Imagine building an SQL schema where a column definition has to be the same in every table where the column name appears - that's pretty much what the first option is. Then you need to invent prefixing strategies to make sure the column you want to use doesn't clash with a column a completely unrelated table (existing, or added by a module you'll enable in six months).

Field names should carry a fixed meaning only within one business domain - you can't predict how a contrib module you'll install in 6 months will name the fields on its entity type.
Modules attaching "behavior" just to a field name are not a use case IMO. The trend, already in D7, is to allow flexibility by letting the behavior vary at least by bundle. RDF module attaches semantics to field names by entity type & bundle, and it would be severely crippled if it didn't.
Even within Field API $field & $instance structures, the trend and feature requests since CCK 4.7 have always been "put most things at the $instance level, so that they can vary for each bundle". So that in the end, all that's left in $field is strictly what defines a database schema, since by definition this is the only thing that *can't* vary.
This is also why I'm not buying "sharing fields just to use the same $field definition" as a use case that should drive our model and APIs: there's really not much in $field, most things are in $instance / by bundle, and this is what really takes effort if for some reason you want to keep it synchronized across entity types or bundles. This is where a "clone", or a "default / override" model à la Views, could potentially be useful, bot not really at the $field level IMO.

@chx: "REVIEW-do-not-test" patches are just the sandbox, "FULL" patches are a local branch with the additional "cleanup" commits I manually rebase over the sandbox for now.

Reroll, plus should hopefully fix a couple fails from the previous reroll.