Currently the mapFromStorageRecords() / mapToStorageRecord() methods only allow direct mapping of db column names to field names.
This works because, on load, new $entity_class(array('field_name' => 'value')) automatically assumes "delta 0, first property".
But this forbids using field types with several properties for base fields, which in turn blocks "single set of field types for base fields & configurable fields".

Discussed with @fago and @plach in Vienna:
We should support both:
- the current default mapping - if "column name = a field name" -> the value gets assigned to delta 0, first property
- a mapping with property names - if "column name = [a field name]__[a property name]" -> the value gets assigned to delta 0, the right property

Comments

Berdir’s picture

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

amateescu’s picture

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

larowlan’s picture

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.

amateescu’s picture

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

yched’s picture

@Berdir #1: yes, basically "single set of field types for base & config fields" & "widgets / formatters on base fields" mean that the various "Turn hardcoded X into a field" tasks do not necessarily mean "turn it into a configurable field". Still makes sense for node body, for example (auto-created as a reasonable default on new node types, but the site builder might want to remove it), but not necessarily for others...

(+ Hah, just realized that Term.php currently uses two separate base fields for "description" and "description format". Nice :-p)

Berdir’s picture

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.

yched’s picture

Do we want to convert one of those (term description/format, user signature/format) in this issue here, as a proof that the patch works ?

amateescu’s picture

I do :)

amateescu’s picture

Status:Active» Needs review
StatusFileSize
new13.46 KB
FAILED: [[SimpleTest]]: [MySQL] 58,397 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

This should do it.

amateescu’s picture

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

Status:Needs review» Needs work

The last submitted patch, 9: 2142549.patch, failed testing.

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new16.85 KB
PASSED: [[SimpleTest]]: [MySQL] 59,028 pass(es).
[ View ]
new3.4 KB
Berdir’s picture

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.

yched’s picture

- not sure about supporting bla & bla__second property. I'd say this adds confusion ?

- getFieldPropertyNames() / getPropertyDefinitions() :
You can use getFieldPropertyNames() on a mere FieldDefinition.
For getPropertyDefinitions() you need an instanciated FieldItem object - do we have one at this point ?

This area is a bit of a mess right now...
- for base fields, $definition->getFieldPropertyNames() creates a one-off FieldItem and calls its getPropertyDefinitions()
- for config fields, $definition->getFieldPropertyNames() uses the static FieldItem::schema()
This is not exactly the same thing, computed values are missing in the latter (but no biggie for our case here)

We know we want to move the static schema() method up from ConfigFieldItemInterface to FieldItemInterface at some point, and this is the info we'd need here (we're dealing with storage). I know @amateescu started by doing that right now as part of this patch, and then preferred to go simpler.

So maybe keep the code in based on getFieldPropertyNames() for now and move it to schema() later on ?

Berdir’s picture

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.

yched’s picture

So yeah, this area about property names / schema columns is really fuzzy ATM.
What we need here is the schema info rather than the properties info, but using the properties info should work for now.

I opened #2144327: Make all field types provide a schema() for sorting this out, and meanwhile, we can move forward here using whatever solution based on properties, preferably with a "@todo reconsider after https://drupal.org/node/2144327" ?

amateescu’s picture

StatusFileSize
new19.37 KB
PASSED: [[SimpleTest]]: [MySQL] 59,011 pass(es).
[ View ]
new5.02 KB

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

yched’s picture

@amateescu: Thanks!

Works for me, but I'll leave some time for others to chime in before pushing RTBC

amateescu’s picture

Posted the patch for user signatures in #1548204-16: Convert user signature into a normal field and postponed it on this one.

fago’s picture

Status:Needs review» Needs work
  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

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new19.21 KB
FAILED: [[SimpleTest]]: [MySQL] 59,089 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
new4.79 KB
  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.

The last submitted patch, 21: 2142549-21.patch, failed testing.

amateescu’s picture

StatusFileSize
new20.54 KB
PASSED: [[SimpleTest]]: [MySQL] 59,130 pass(es).
[ View ]
new1.95 KB
fago’s picture

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

amateescu’s picture

StatusFileSize
new20.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,011 pass(es).
[ View ]
new959 bytes

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

The last submitted patch, 25: 2142549-25.patch, failed testing.

fago’s picture

Yes, I think that would be reasonable

fago’s picture

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

The last submitted patch, 25: 2142549-25.patch, failed testing.

cosmicdreams’s picture

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.

amateescu’s picture

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

amateescu’s picture

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

Berdir’s picture

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

amateescu’s picture

StatusFileSize
new21.22 KB
PASSED: [[SimpleTest]]: [MySQL] 59,019 pass(es).
[ View ]
new2.49 KB

Thanks for catching that, fixed and added a test.

amateescu’s picture

StatusFileSize
new20.78 KB
PASSED: [[SimpleTest]]: [MySQL] 59,092 pass(es).
[ View ]
yched’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermViewBuilder.php
@@ -24,10 +24,11 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
+      // @todo Remove this when base fields will be able to use formatters.

Minor - grammar: "... when base fields are able to use formatters" ;-)

Also, we should include the issue URL (that would probably be #2144919: Allow widgets and formatters for base fields to be configured in Field UI rather than #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title)

yched’s picture

@cosmicdreams: right, this is not about compound fields, but about allowing base fields to use the same set of field types than configurable fields, instead of currently having a de-facto, non-written restriction on field types that only store one "column/property".

This being said, and generally speaking, Dave Reid's approach for multifield should be much easier to code in D8 with Field types / widgets / formatters as OO classes - multifield is basically OO composition, the real challenge is shaping a good UI for creation and configuration. But yeah, unrelated to the task here.

amateescu’s picture

StatusFileSize
new20.82 KB
PASSED: [[SimpleTest]]: [MySQL] 58,994 pass(es).
[ View ]
new867 bytes

:)

yched’s picture

Status:Needs review» Reviewed & tested by the community

OK, RTBC then ?

plach’s picture

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

yched’s picture

Oh, yup, fully agreed with #40 - I thought I had commented on that in an earlier review, but apparently it got lost before I pressed "submit"...

amateescu’s picture

StatusFileSize
new20.85 KB
PASSED: [[SimpleTest]]: [MySQL] 59,016 pass(es).
[ View ]
new1.76 KB

Entity field api team never sleeps :)

yched’s picture

OK, you're going to hate me...

+++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
@@ -372,7 +372,7 @@ protected function attachPropertyData(array &$entities, $revision_id = FALSE) {
-              if (($field = strstr($data_column_name, '__', TRUE)) && $field === $field_name) {
+              if (($field_column_name = strstr($data_column_name, '__', TRUE)) && $field_column_name === $field_name) {

$field_column_name is misleading, that var contains the part before the __, that is a field name.
$field_name itself is already taken, so maybe $data_field_name ?

amateescu’s picture

StatusFileSize
new20.85 KB
PASSED: [[SimpleTest]]: [MySQL] 59,033 pass(es).
[ View ]
new1.06 KB

<3

yched’s picture

<3 and confirming RTBC :-)
Thanks!

pwolanin’s picture

pwolanin’s picture

Issue tags:+beta blocker

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

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Closed (fixed)

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