Support multi-column field types in base tables

+1, thanks for opening the issue.

Small note on "This works because, on load, new $entity_class(array('field_name' => 'value')) automatically assumes "delta 0, first property".".

What actually happens is that the value gets through the setValue() chain of the field item list class and then field item, so they have full control on how to use it, reference fields for example support both the entity and the id being passed in like that.

Two use cases in core right now could use this (that I know of), term descriptions and user signatures. Both have a issue to make it a configurable field and term description might still make it, wondering if we should just keep it as a base field, as it has some rather special behavior, like being added to comments and not rendered on the user like other fields.

Thanks as well for opening this issue!

Two more relevant use cases for core are:

• a generic "internal path" field, for which we need three columns: route_name, route_parameters and options. All these "properties" are really only used together in order to construct a link. This field type will be used by two entity types in core: menu links and shortcuts
• a "tree/hierarchical field" (probably extending entity_reference) for which the menu_link implementation will group together the p1, p2.. p9 columns

Based on the two points above, I think it's worthless to spend more time working on the menu_link conversion to the new API until this issue is completed.

Furthermore, the biggest benefit of this is that we'll be able to use any "configurable" field as a base field, if we know from the start that it will be single-valued, thus completing the unification of the the two APIs on the storage front.

I think that's, in a nutshell, what I described to @yched over our beer conversation two weeks ago :)

If the p1 to p9 stuff is up for discussion, I'd be willing to take a crack at rewriting it using tree based logic, where you store left and right ids etc. that would remove the 9 parents limit and mean you could query a hierarchy in one go. Is so let me know and I'll file a new issue.

@larowlan, my plan for that is in #1915056-2: Use entity reference for taxonomy parents. As you can see there, replacing the current materialized path implementation of menu links *might* be up for discussion, but we're quite far from reaching step 6 in there and it all needs to start with this issue.

I'll try to work on this tomorrow if no one else takes it until then :)

Yes, user signature works exactly the same way, just like node bodies used to in 6.x. For user signatures, it was a security fix that added the format.

If we convert those to a single field (base or configurable), we e.g. also fix file usage management within them that is provided by the wysiwyg editor stuff.

I do :)

This should do it.

And a small self review:

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
@@ -230,13 +230,7 @@ public static function baseFieldDefinitions($entity_type) {
     $properties['description'] = array(
       'label' => t('Description'),
       'description' => t('A description of the term'),
-      'type' => 'string_field',
-    );
-    // @todo Combine with description.
-    $properties['format'] = array(
-      'label' => t('Description format'),
-      'description' => t('The filter format ID of the description.'),
-      'type' => 'string_field',
+      'type' => 'field_item:text_long',
     );

WIN!!!!11

No dreditor :(

- Do we need to be more error-prone in the first snippet when getting the values? What if someone has bla and bla__value? I don't know...

- We did discuss if we need getFieldPropertyNames() at some point, I think it overlaps with other methods. Maybe use getPropertyDefinitions()?

- I think user signatures would be a better and easier proof of concept, less special cases with forum.module and I still hope that #569434: Remove taxonomy term description field; provide description field for forum taxonomy will happen, because it makes sense for those.

agreed that we can't support bla and bla__something, just wondering about better error handling for that case, right now, something very weird would happen if you'd do that :)

We have $field_items, so we also have a single field item and could call offsetGet(0)->getPropertyDefinitions() (#2110467: Add first(), get($index) and possibly other methods to ListInterface would make that first()->getPropertyDefinitions(), please review :))

Just wanted to point it out, if you think getFieldPropertyNames() is ok to use for now, then I'm fine with that too.

@Berdir, discussed a bit with @yched in IRC and while we could throw an exception in the case where we have both "bla" and "bla__value" columns, I don't think it adds much value here since we already have a plan to make the schema generation dynamic, so there'll be no place for human error anymore.

About user signatures being a better PoC, I tried to convert them and I ended up in some misterious typed data validation errors, so I will open a followup (postponed on this one) to figure out the problem there.

Also, this patch actually makes #569434: Remove taxonomy term description field; provide description field for forum taxonomy easier, just remove the base field definition, remove code from the form controller that handles it and provide it as a default config.

Here's an updated patch with more comments and variable renames that try to provide some background for the new code.

Posted the patch for user signatures in #1548204-16: Remove user signature and move it to contrib and postponed it on this one.

1. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -281,11 +281,21 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
   +      // Skip the item delta and item value levels but let the field assign
   +      // the value as suiting. This avoids unnecessary array hierarchies and
   

   Maybe the comment should say that we skip the levels if possible now, as it cannot always skip the property level any more.

2. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -349,9 +359,21 @@ protected function attachPropertyData(array &$entities, $revision_id = FALSE) {
   +          else {
   +            foreach ($data_column_names as $data_column_name) {
   

   Looping over the columns multiple times here seems in-efficient.

   Instead of looking up the schema, should we look whether there is data in the "$name__$property" instead?

3. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -744,17 +766,41 @@ protected function savePropertyData(EntityInterface $entity, $table_key = 'data_
   +      // @todo Reconsider the usage of getFieldPropertyNames() after
   +      // https://drupal.org/node/2144327.
   +      foreach ($field_items->getFieldDefinition()->getFieldPropertyNames() as $property_name) {
   

   This should just do array_keys($field_items[0]->getPropertyDefinitinos()) for now as else it creates new item objects anyway....

4. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
   @@ -744,17 +766,41 @@ protected function savePropertyData(EntityInterface $entity, $table_key = 'data_
          // If we are creating a new entity, we must not populate the record with
          // NULL values otherwise defaults would not be applied.
   

   what? defaults are not applied on schema level, but on entity / field api level. This is unrelated though.

5. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TokenReplaceTest.php
   @@ -88,7 +88,7 @@ function testTaxonomyTokenReplacement() {
   +    $tests['[term:description]'] = check_markup($term1->description->value, $term1->description->format);
   

   Should be $term1->description->processed

6. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TokenReplaceTest.php
   @@ -103,7 +103,7 @@ function testTaxonomyTokenReplacement() {
   +    $tests['[term:description]'] = check_markup($term2->description->value, $term2->description->format);
   

   Same here.

7. +++ b/core/modules/taxonomy/taxonomy.pages.inc
   @@ -76,7 +76,7 @@ function taxonomy_term_feed(Term $term) {
   +  $channel['description'] = check_markup($term->description->value, $term->description->format, '', TRUE);
   

   ->processed

8. +++ b/core/modules/taxonomy/taxonomy.tokens.inc
   @@ -108,7 +108,7 @@ function taxonomy_tokens($type, $tokens, array $data = array(), array $options =
   +          $replacements[$original] = $sanitize ? check_markup($term->description->value, $term->description->format, '', TRUE) : $term->description->value;
   

   again

1. Fixed.
2. I wrote it like this because we don't know the property names without instantiating a lot of objects, while the foreach + strstr() is basically "free" compared to that.
3. Ok, let's not argue too much over this, Berdir said the same thing and @yched and I don't agree, but we'll sort it out in the linked @todo. So fixed.
4. Yeah.. unrelated, ask @plach :)
5. Fixed all ->processed.

#20.2 still bugs me. It's not only that is in-efficient, what bugs me most is that we have to in which table it's stored. We shouldn't have to do that, but have a clear mapping based on the field definition (translatability, revisionability).

We'll need #2144631: Add a revisionable key to field definitions for being able to do that first though. thoughts?

I agree that the logic should eventually be based on field definition properties, so how about this interdiff? If we postpone this issue, the dependency stack is getting too high IMO :)

Yes, I think that would be reasonable

25: 2142549-25.patch queued for re-testing.

I'm trying to follow along here, but I'm having trouble understanding if this issue is like the effort Dave Reid is putting into multifield : https://drupal.org/project/multifield

Multifield intends to provide a better (IMO) architectural solution to field collection by providing a system driven way of handling the schema of your administratively controlled field. It does this by creating field tables with multiple columns. So on the surface it sounds like the same effort you're doing here (but for Drupal 7).

My question is:
If this API is available to developers can we build our own compounded fields with it? For example I tend to want to develop a field to handle all parts of an individual slide.

25: 2142549-25.patch queued for re-testing.

@cosmicdreams, this patch is not about building "compounded" fields, it's about letting you use fields that store data across multiple columns as base fields (stored in the base table(s) of the entity type).

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermViewBuilder.php
@@ -27,7 +27,7 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
       $display = $displays[$entity->bundle()];
       if (!empty($entity->description->value) && $display->getComponent('description')) {
         $entity->content['description'] = array(
-          '#markup' => check_markup($entity->description->value, $entity->format->value, '', TRUE),
+          '#markup' => check_markup($entity->description->value, $entity->description->format, '', TRUE),
           '#prefix' => '<div class="taxonomy-term-description">',
           '#suffix' => '</div>',
         );

Should this also switch to processed?

I assume we don't have any tests coverage with term descriptions and actually using a filter, because it's broken now.

processed/TextProcessed respects the text_processing setting, which defaults to FALSE, which means that this always uses check_plain() now.

We need to set the setting in the base field definition and we might need to add basic test coverage for this.

Also, this will not make the term description patch easier, because that *removes* all the custom code around that, which will then conflict with this quite bit. There might also be references in templates and similar things that could be affected, not sure, check the patch there. But I'm fine if we can move forward somehow, the other issue is currently blocked on the taxonomy maintainer (@xjm).

Thanks for catching that, fixed and added a test.

Rerolled after #2047229: Make use of classes for entity field and data definitions.

:)

+++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
@@ -281,11 +281,21 @@ protected function mapFromStorageRecords(array $records, $load_revision = FALSE)
+        if ($field = strstr($name, '__', TRUE)) {
+          $property_name = substr($name, strpos($name, '__') + 2);
+          $entities[$id][$field][Language::LANGCODE_DEFAULT][$property_name] = $value;

@@ -349,9 +359,24 @@ protected function attachPropertyData(array &$entities, $revision_id = FALSE) {
+              if (($field = strstr($data_column_name, '__', TRUE)) && $field === $field_name) {
+                $property_name = substr($data_column_name, strpos($data_column_name, '__') + 2);
+                $entities[$id][$field_name][$langcode][$property_name] = $values[$data_column_name];
+              }

Minor, but could we use $field_name or similar instead of $field here? $field is usually an object.

Entity field api team never sleeps :)

<3

Marking as a beta blocker since this is blocking having translatable titles for menu links

Committed/pushed to 8.x, thanks!