Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Assigned: Unassigned » fago
Status: Active » Needs review
FileSize
2.02 KB

Assigning to fago for review. There's no test yet. I am not sure whether self::$propertyDefinitions['entity'] = array('type' => 'entity' this is correct. The first entity, is that entity or term? Should we define both? The second entity, the value of 'type', what is the consequence of type? It's not too clear.

Status: Needs review » Needs work

The last submitted patch, 1839070_1.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

#1: 1839070_1.patch queued for re-testing.

fago’s picture

Assigned: fago » Unassigned
Issue tags: +Entity Field API

The approach looks good to me. Later on migrating directly to the EntityReferenceItem field is probably useful, but for now we need this to make it work with existing data structures.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Type/TaxonomyTermReferenceItem.php
@@ -0,0 +1,43 @@
+        'label' => t('Entity'),
+        'description' => t('The referenced entity'),

Label and description should talk of terms.

For testing, we'll need to use it on the test entity or now.

chx’s picture

Assigned: Unassigned » fago
FileSize
6.32 KB

This should be ready. Assigning back to fago for another review. It is necessary to test efq relationships with computed fields.

chx’s picture

FileSize
6.62 KB

Sigh, I uploaded an old version. This has more asserts (although the previous one also should be enough).

fago’s picture

Assigned: fago » Unassigned
FileSize
5.32 KB
7.26 KB

Thanks - looks good to me! I had a look at it and tried to improve it a bit:
- fixed some typos
- added some test assertions to check changes are reflected
- slightly improved the field-item code based upon recent results from #1778178: Convert comments to the new Entity Field API

fago’s picture

FileSize
600 bytes
7.26 KB

one more typo

Berdir’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermReferenceItemTest.phpundefined
@@ -0,0 +1,113 @@
+/**
+ * @file
+ * Definition of Drupal\taxonomy\Tests\TaxonomyTermReferenceItemTest.

Nitpick: Should be Contains now, and it's also being changed to not include the namespace here, but the issue to decide that is still at RTBC and there was no response to this.

Other than that, this looks great to me and is a nice example to convert the other fields. Have to go now, but can RTBC this tomorrow if someone can do a quick re-roll to fix that.

chx’s picture

FileSize
541 bytes
7.24 KB

Like this?

amitaibu’s picture

Not sure, but I think you should also implement hook_data_type_info()

chx’s picture

Re #11 not taxonomy's responsibility, it's field's http://api.drupal.org/api/drupal/core!modules!field!field.module/functio...

Berdir’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermReferenceItemTest.phpundefined
@@ -0,0 +1,113 @@
+/**
+ * @file
+ * Contains TaxonomyTermReferenceItemTest.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Type/TaxonomyTermReferenceItem.phpundefined
@@ -0,0 +1,79 @@
+/**
+ * @file
+ * Definition of Drupal\taxonomy\Type\TaxonomyTermReferenceItem.
+ */

Looks like @xjm has valid arguments against removing the namespace here. See #1487760-88: [policy, no patch] Decide on documentation standards for namespaced items and following. So let's use the Contains and the namespace. Sorry for the confusion.

chx’s picture

FileSize
7.25 KB
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Great, looks good to me.

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

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

fago’s picture

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

#14: terms_0.patch queued for re-testing.

fago’s picture

Status: Needs review » Reviewed & tested by the community

Looks like an unrelated test-fail. Requested a re-test and back to RTBC.

tim.plunkett’s picture

FileSize
2.75 KB
7.28 KB

I came here to review this, but that has been done! I added a couple doc/standards fixes.

chx’s picture

#19: drupal-1839070-19.patch queued for re-testing.

chx’s picture

These days we move so fast, a five day patch needs to be retested to make sure it applies still.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I was curious why the default profile's "Tags" vocabulary wasn't converted to this field type as part of this patch, but chx pointed out that because nodes aren't yet converted to EFAPI, that this patch won't take effect. That's also why it can't really be benchmarked right now. However, this unblocks issues in other areas, so...

Committed and pushed to 8.x. Thanks!

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