Comments

plach’s picture

Issue tags: +node ng
berdir’s picture

Status: Active » Needs review
StatusFileSize
new6.01 KB

A first patch. Add the item classes and converted a basic test to it.

Status: Needs review » Needs work

The last submitted patch, list-field-item-1839064-2.patch, failed testing.

berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new8.29 KB

This should fix the test failure.

berdir’s picture

This patch adds a dependency to options.module on number and text. Conceptually, that makes sense to me, because this module basically provides extended versions of the integer/float/text field types.

The other question is here if we need more tests. We're not adding new field item type classes, so IMHO adding more unit tests for those would be pointless. As said above, there are a lot of tests for allowed values, callbacks and similar things that I'm already in the process of converting to NG in #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest.

das-peter’s picture

Status: Needs review » Reviewed & tested by the community

This looks pretty good to me, and I know I need the 'field item class' definitions for #1818556: Convert nodes to the new Entity Field API.
So even if I just mainly did a visual review I'm bold enough to set this RTBC.

plach’s picture

Priority: Normal » Critical

Raising priority since this is blocking a critical task: #1818556: Convert nodes to the new Entity Field API.

fago’s picture

Patch looks good the me as well. I agree that we want this dependencies to be able to re-use the existing fields.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Being forced to introduce a dependency on number module is a little unfortunate, but otherwise this looks good, and allows us to make progress on the node patch.

Committed and pushed to 8.x. Thanks!

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