Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#19 | drupal-1839070-19.patch | 7.28 KB | tim.plunkett |
#19 | interdiff.txt | 2.75 KB | tim.plunkett |
#14 | terms_0.patch | 7.25 KB | chx |
#10 | terms_0.patch | 7.24 KB | chx |
#10 | interdiff.txt | 541 bytes | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedAssigning 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.Comment #3
chx CreditAttribution: chx commented#1: 1839070_1.patch queued for re-testing.
Comment #4
fagoThe 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.
Label and description should talk of terms.
For testing, we'll need to use it on the test entity or now.
Comment #5
chx CreditAttribution: chx commentedThis should be ready. Assigning back to fago for another review. It is necessary to test efq relationships with computed fields.
Comment #6
chx CreditAttribution: chx commentedSigh, I uploaded an old version. This has more asserts (although the previous one also should be enough).
Comment #7
fagoThanks - 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
Comment #8
fagoone more typo
Comment #9
BerdirNitpick: 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.
Comment #10
chx CreditAttribution: chx commentedLike this?
Comment #11
amitaibuNot sure, but I think you should also implement hook_data_type_info()
Comment #12
chx CreditAttribution: chx commentedRe #11 not taxonomy's responsibility, it's field's http://api.drupal.org/api/drupal/core!modules!field!field.module/functio...
Comment #13
BerdirLooks 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.
Comment #14
chx CreditAttribution: chx commentedComment #15
BerdirGreat, looks good to me.
Comment #17
fago#14: terms_0.patch queued for re-testing.
Comment #18
fagoLooks like an unrelated test-fail. Requested a re-test and back to RTBC.
Comment #19
tim.plunkettI came here to review this, but that has been done! I added a couple doc/standards fixes.
Comment #20
chx CreditAttribution: chx commented#19: drupal-1839070-19.patch queued for re-testing.
Comment #21
chx CreditAttribution: chx commentedThese days we move so fast, a five day patch needs to be retested to make sure it applies still.
Comment #22
webchickI 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!