Since #1847596: Remove Taxonomy term reference field in favor of Entity reference is not going anywhere, we need another way of grouping reference-like field types together. Let's start with the taxonomy term reference field and follow-up with file and image.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Status: Active » Needs review
FileSize
24.27 KB

And here's the patch.

Status: Needs review » Needs work

The last submitted patch, 1965208.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
752 bytes
25.39 KB

This should fix it.

Berdir’s picture

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -335,3 +335,37 @@ function taxonomy_update_8006() {
+  if (!$fields = field_read_fields(array('type' => 'taxonomy_term_reference'), array('include_inactive' => 1))) {

This should probably use the versioned, update-safe field api functions.

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -335,3 +335,37 @@ function taxonomy_update_8006() {
+    if ($field['storage']['type'] != 'field_sql_storage') {
+      // Field doesn't use SQL storage, we cannot modify the schema.
+      continue;

That's a tricky one, if not we, then who's going to do it? :)

The changes themself are simple enough and make sense I think. We'll save more code as we'll start to move additional things into there (validation, schema possibly, field API stuff as it will get merged with Entity Field API).

We should probably do the same for image and file items.

amateescu’s picture

FileSize
619 bytes
25.37 KB

This should probably use the versioned, update-safe field api functions.

Yeah, I see that the Field API to CMI conversion didn't remove the data from {field_config} so I guess _update_7000_field_read_fields() should work at least until we get a 8000 version of it.

That's a tricky one, if not we, then who's going to do it? :)

The module that provides the alternate field storage? :/ Core can only do updates for the storage it provides and knows how to deal with..

We should probably do the same for image and file items.

Those are next, as I mentioned in the OP :)

Berdir’s picture

Yes, there are some follow-up field api issues about providing new upgrade path helpers and deciding when the upgrade should exactly happen. Fun stuff ;)

/me reads OP

Oh. ;)

Status: Needs review » Needs work

The last submitted patch, 1965208-5.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
700 bytes
25.47 KB

Ok, so that won't fly. How about just a @todo for now?

Status: Needs review » Needs work
Issue tags: -sprint, -Entity Field API

The last submitted patch, 1965208-8.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#8: 1965208-8.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +sprint, +Entity Field API

The last submitted patch, 1965208-8.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
723 bytes
28.01 KB

Rerolled and added a update requirement to be sure that we're running after field api has been converted to config.

Status: Needs review » Needs work

The last submitted patch, 1965208-12.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
27.77 KB

Most of that conflicted due the term ng patch. Will probably still fail installation.

Status: Needs review » Needs work

The last submitted patch, taxonomy-target-id-1965208-14.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
711 bytes
27.77 KB

This should fix that.

Status: Needs review » Needs work

The last submitted patch, taxonomy-target-id-1965208-16.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
442 bytes
28.2 KB

Weird. See interdiff. When those indexes were still there, the whole installation process just hang up within config_import_invoke_owner(), it comes until field.field.field_tags and then it just stops and doesn't continue.

@yched/@swentel, any clues as to what's going on there in terms of validation logic and pre/post save? This is.. interesting to debug ;)

Berdir’s picture

Discussed with @yched and it turns out that field api does throw an exception, I'm just not seeing it. When checking the ajax response with developer tools, it says "SQLSTATE[42000]: Syntax error or access violation: 1072 Key column 'field_tags_tid' doesn't exist in table: CREATE TABLE {field_data_field_tags} (" which is perfect.

Berdir’s picture

Re-roll, conflicted in taxonomy_field_presave()

yched’s picture

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -335,3 +347,38 @@ function taxonomy_update_8006() {
+  // @todo Switch to the update-safe version of this function when we have one.
+  if (!$fields = field_read_fields(array('type' => 'taxonomy_term_reference'), array('include_inactive' => 1))) {
+    return;

So, the conclusion in #1969662: Provide new _update_8000_*() CRUD helpers for Field and Instance definitions was that a specialized helper function for "read fields" would be of little interest as opposed to just iterating over raw config() data.

So the current plan is to *not* include helpers for "read existing field/instance definitions", since (unlike the "create" ops, that are a little more involved since they require pinging the storage backends) they would not really bring functionality specific to those Field API structures. If anything, a generic update-safe "EFQ on config records" could be useful for all config entities, but that's outside the scope of field API.

In short - maybe try to write this on top of raw config reads ? It doesn't seem it shouldn't be too hard.

amateescu’s picture

Something like this then?

Also rerolled for #1985138: New autocomplete taxonomy terms not previewable.

Berdir’s picture

Can't RTBC the upgrade path part but the other part of the patch looks fine to me. I did a few re-rolls but just fixed some tests. So if @yched or maybe @swentel can confirm the last interdiff, then this can be RTBC'd by them.

swentel’s picture

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -352,33 +352,36 @@ function taxonomy_update_8006() {
+  foreach (config_get_storage_names_with_prefix('field.field.') as $config_name) {
+    $field_config = config($config_name);
+    // Only update taxonomy reference fields that use the default SQL storage.
+    if ($field_config->get('type') == 'taxonomy_term_reference' && $field_config->get('storage.type') == 'field_sql_storage') {
+      $field_id = $field_config->get('id');
+      $field = array(
+        // _field_sql_storage_tablename() still relies on the 'field_name'
+        // property.
+        'field_name' => $field_id,
+        'deleted' => FALSE,
+      );

I would do something like this:

$field = new Field(config($config_name)->get());

It's a pattern we're using in other places as well see http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

This will also guarantee we don't have to touch this again when removing the BC layer.

amateescu’s picture

That looks much better indeed.

amateescu’s picture

@swentel pointed out in IRC that we need to update the indexes value in the field config too.

amateescu’s picture

@swentel also wants to play nice with other modules that might have overriden those indexes :)

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Sweet, thanks :)

Berdir’s picture

Issue tags: -sprint, -Entity Field API

Status: Reviewed & tested by the community » Needs work

The last submitted patch, taxonomy-target-id-1965208-27.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +sprint, +Entity Field API

The last submitted patch, taxonomy-target-id-1965208-27.patch, failed testing.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
27.35 KB
fago’s picture

Issue tags: -sprint, -Entity Field API

Status: Reviewed & tested by the community » Needs work
Issue tags: +sprint, +Entity Field API

The last submitted patch, taxonomy-target-id-1965208-33.patch, failed testing.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

I thought I linked this here before, but apparently not. Here's the FileItem/ImageItem equivalent of this patch: #1996714: Convert FileItem and ImageItem to extend EntityReferenceItem

Status: Reviewed & tested by the community » Needs work

The last submitted patch, taxonomy-target-id-1965208-33.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
27.35 KB

Rerolled.

das-peter’s picture

Looks pretty good to me, couldn't find anything to nag about.
However, I'm not sure what's ok to change in the views code and what's not.
As far as I understand the following changes only affect "meta data" and are backward compatible because the 'base field' still is'tid', right?

+++ b/core/modules/taxonomy/taxonomy.views.inc
@@ -346,7 +346,7 @@ function taxonomy_field_views_data($field) {
-    $data[$table_name][$field['field_name'] . '_tid']['relationship'] = array(
+    $data[$table_name][$field['field_name'] . '_target_id']['relationship'] = array(
       'id' => 'standard',
       'base' => 'taxonomy_term_data',
       'base field' => 'tid',
@@ -380,7 +380,7 @@ function taxonomy_field_views_data_views_data_alter(&$data, $field) {

@@ -380,7 +380,7 @@ function taxonomy_field_views_data_views_data_alter(&$data, $field) {
       'id' => 'entity_reverse',
       'field_name' => $field['field_name'],
       'field table' => _field_sql_storage_tablename($field),
-      'field field' => $field['field_name'] . '_tid',
+      'field field' => $field['field_name'] . '_target_id',
amateescu’s picture

Status: Needs review » Reviewed & tested by the community

'base field' is the taxonomy id property, and yes, that remains unchanged. The only thing this patch changes is the name of the taxonomy reference field column.

Back to rtbc per #28.

yched’s picture

No need to wait for it, but when #1969728: Implement Field API "field types" as TypedData Plugins lands, this change will in practice mean "the 'taxonomy field type' plugin extends the "entity referrence field type' plugin", which makes very much sense.

In other words, +1.

fago’s picture

Issue tags: -sprint, -Entity Field API
Berdir’s picture

moshe weitzman’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +sprint, +Entity Field API

The last submitted patch, taxonomy-target-id-1965208-37.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
27.28 KB

YAR. (yet another reroll)

Status: Needs review » Needs work
Issue tags: -sprint, -Entity Field API

The last submitted patch, taxonomy-target-id-1965208-46.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +sprint, +Entity Field API
swentel’s picture

Status: Needs review » Reviewed & tested by the community

Finally!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs another YAR...

curl https://drupal.org/files/taxonomy-target-id-1965208-46.patch | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 27937  100 27937    0     0  16583      0  0:00:01  0:00:01 --:--:-- 18863
error: patch failed: core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/widget/TaxonomyAutocompleteWidget.php:52
error: core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/widget/TaxonomyAutocompleteWidget.php: patch does not apply
error: patch failed: core/modules/taxonomy/taxonomy.module:994
error: core/modules/taxonomy/taxonomy.module: patch does not apply
amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
27.38 KB

YARRR!

effulgentsia’s picture

Priority: Normal » Major
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Type/TaxonomyTermReferenceItem.php
@@ -9,11 +9,12 @@
-class TaxonomyTermReferenceItem extends FieldItemBase {
+class TaxonomyTermReferenceItem extends EntityReferenceItem {

I think this is crucial for rest.module to be able to handle exporting and importing of content with term reference fields (without writing new custom code for them), so raising to major.

amateescu’s picture

Issue tags: -Needs reroll
FileSize
27.71 KB
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Berdir’s picture

Issue tags: -sprint

Removing sprint tag.

Status: Fixed » Closed (fixed)

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

yched’s picture