Sub issue of #2014671: [META] Convert all field types to plugins.

Looks like poor field_test.module got forgotten, so here's a start. Have moved a few things over, validation isn't adjusted yet and some crazy parts with that memorize stuff aren't either. I think it makes sense to re-think that a bit. Looks like basic crud tests worked, let's see about the others.

Related change notice https://drupal.org/node/2064123

Files: 
CommentFileSizeAuthor
#41 interdiff.txt564 bytesandypost
#41 field-test-type-2083803-41.patch27.12 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,840 pass(es).
[ View ]
#39 interdiff.txt982 bytesandypost
#39 field-test-type-2083803-39.patch27.23 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,915 pass(es).
[ View ]
#38 interdiff.txt2.06 KBandypost
#38 field-test-type-2083803-38.patch27.23 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es).
[ View ]
#35 interdiff.txt4.23 KBandypost
#35 field-test-type-2083803-35.patch27.02 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,835 pass(es).
[ View ]
#33 field-test-type-2083803-33.patch24.57 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,766 pass(es).
[ View ]
#33 field-test-type-2083803-33-interdiff.txt786 bytesBerdir
#31 field-test-type-2083803-31.patch24.56 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,836 pass(es).
[ View ]
#31 field-test-type-2083803-31-interdiff.txt1.92 KBBerdir
#28 field-test-type-2083803-28.patch24.51 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,307 pass(es), 11 fail(s), and 0 exception(s).
[ View ]
#20 drupal8.field-system.2083803-20.patch24.49 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.field-system.2083803-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 interdiff.txt2.6 KBandypost
#13 drupal8.field-system.2083803-13.patch24.49 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,730 pass(es).
[ View ]
#8 interdiff.txt3.89 KBandypost
#8 drupal8.field-system.2083803-8.patch24.31 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,228 pass(es).
[ View ]
#8 drupal8.field-system.2083803-noinstall-8.patch24.47 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,858 pass(es).
[ View ]
#6 interdiff.txt3.2 KBandypost
#6 drupal8.field-system.2083803-6-noinstallfile.patch27.2 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#6 drupal8.field-system.2083803-6.patch25.22 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 59,353 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
#5 interdiff.txt4.1 KBandypost
#5 drupal8.field-system.2083803-5.patch27.56 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 59,251 pass(es), 50 fail(s), and 34 exception(s).
[ View ]
#4 interdiff.txt3.87 KBandypost
#4 drupal8.field-system.2083803-4.patch24.3 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#3 interdiff.txt12.49 KBandypost
#3 drupal8.field-system.2083803-3.patch25.58 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 59,328 pass(es), 49 fail(s), and 35 exception(s).
[ View ]
#1 field-test-types-2083803-1.patch18.54 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,362 pass(es), 86 fail(s), and 51 exception(s).
[ View ]

Comments

Berdir’s picture

StatusFileSize
new18.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,362 pass(es), 86 fail(s), and 51 exception(s).
[ View ]

Here's a first patch.

Status:Needs review» Needs work

The last submitted patch, field-test-types-2083803-1.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new25.58 KB
FAILED: [[SimpleTest]]: [MySQL] 59,328 pass(es), 49 fail(s), and 35 exception(s).
[ View ]
new12.49 KB

Filed separate issue to fix broken test #2087449: Fix broken tests FieldAttachOtherTest->testFieldAttachCache()
Fixed tests: ShapeItemTest, FieldValidationTest

andypost’s picture

StatusFileSize
new24.3 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new3.87 KB

Reverted back changed test about 'default_value_function' - that's needed
Removed hook_field_load - prepareCache() already there

andypost’s picture

StatusFileSize
new27.56 KB
FAILED: [[SimpleTest]]: [MySQL] 59,251 pass(es), 50 fail(s), and 34 exception(s).
[ View ]
new4.1 KB

Fixed FieldInfoTest

andypost’s picture

StatusFileSize
new25.22 KB
FAILED: [[SimpleTest]]: [MySQL] 59,353 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
new27.2 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new3.2 KB

All tests are passed locally.
1) FieldAttachOtherTest.php changed according #2087449: Fix broken tests FieldAttachOtherTest->testFieldAttachCache()
2) field_test_widget_multiple_validate moved to public static method TestFieldWidgetMultiple::multipleValidate()

PS drupal8.field-system.2083803-6-noinstallfile.patch test the need in install file

Status:Needs review» Needs work

The last submitted patch, drupal8.field-system.2083803-6.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new24.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,858 pass(es).
[ View ]
new24.31 KB
PASSED: [[SimpleTest]]: [MySQL] 59,228 pass(es).
[ View ]
new3.89 KB

A bit more clean-up

amateescu’s picture

Status:Needs review» Needs work
+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.php
@@ -327,4 +327,50 @@ function testWidgetDefinition() {
+  /**
+   * Returns field info definition.
+   */
+  protected function field_test_field_info() {

Why don't you get the field plugin definition like everyone else and duplicate all the data here? :)

andypost’s picture

Suppose better to rename this to expectedFieldInfo()

+++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.php
@@ -23,7 +23,7 @@ public static function getInfo() {
+    $field_test_info = $this->field_test_field_info();
     $info = \Drupal::service('plugin.manager.entity.field.field_type')->getDefinitions();
     foreach ($field_test_info as $t_key => $field_type) {

@@ -283,7 +283,7 @@ function testFieldMap() {
+    $info = $this->field_test_field_info();
     foreach ($info as $type => $data) {

@@ -327,4 +327,50 @@ function testWidgetDefinition() {
+  protected function field_test_field_info() {
+    return array(
+      'test_field' => array(

because this config used to validate result of getDefinition()

Berdir’s picture

  1. +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/field/field_type/TestItem.php
    @@ -0,0 +1,153 @@
    +  public function delete() {
    +    // In D7 it was hook_field_delete().
    +    field_test_memorize('field_test_field_delete', array($this->getEntity()));
    +  }

    I don't think we need to document how it was called in 7.x..

  2. +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/field/field_type/TestItem.php
    @@ -0,0 +1,153 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function isEmpty() {
    +    $value = $this->get('value')->getValue();
    +    return empty($value);
    +  }

    Can be written as return empty($this->value).

Berdir’s picture

About method name, I'd suggest getExpectedFieldTypeDefinition() or something similar with getExpected..()

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new24.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,730 pass(es).
[ View ]
new2.6 KB

addressed

Status:Needs review» Needs work
Issue tags:-Field API

The last submitted patch, drupal8.field-system.2083803-13.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
Issue tags:+Field API
Berdir’s picture

Interdiff looks fine to me, so +1 from my side. A bit surprised that we apparently don't have any tests remaining that required the memorize stuff for load/insert/update, but fine with me if that went out somewhere.

Wondering if we should add something else to the list of arguments we put in there, it used to contain the items too, so maybe add $this or $this->getValue() (easier to serialize) to it?

@yched?

andypost’s picture

I'd prefer to keep conversion issues in the scope, so filed follow-up #2089369: Extend tests for field hooks

Berdir’s picture

Not saying we need more tests.

I'm just saying that I *thought* we had more tests for this and I was confused to not see more test fails. But fine with me, pretty sure we have enough implicit test coverage of those methods. If anything, we should write a unit test for this kind of test when we can mock everything we need to at some point.

andypost’s picture

andypost’s picture

StatusFileSize
new24.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.field-system.2083803-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

@yched we needs your approval!
merge HEAD

Berdir’s picture

Issue tags:-Field API

Status:Needs review» Needs work

The last submitted patch, drupal8.field-system.2083803-20.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
Issue tags:+Field API
andypost’s picture

patch still applies

Berdir’s picture

Berdir’s picture

Issue tags:-Field API

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

The last submitted patch, drupal8.field-system.2083803-20.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new24.51 KB
FAILED: [[SimpleTest]]: [MySQL] 58,307 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

Re-roll.

swentel’s picture

Status:Needs review» Reviewed & tested by the community

Looks good, another critical down if green!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, field-test-type-2083803-28.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new1.92 KB
new24.56 KB
PASSED: [[SimpleTest]]: [MySQL] 58,836 pass(es).
[ View ]

This should fix the test fails.

swentel’s picture

+++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Validation/Constraint/TestFieldConstraint.php
@@ -0,0 +1,40 @@
+ * Contains \Drupal\field_test\Plugin\Validation\Constraint\TestField.

Nitpick, should probably be \TestFieldConstraint no ? Or do we do this different for constraint classes ?

Berdir’s picture

StatusFileSize
new786 bytes
new24.57 KB
PASSED: [[SimpleTest]]: [MySQL] 58,766 pass(es).
[ View ]

Fixed that. No, constraint classes are not that special :)

swentel’s picture

Status:Needs review» Reviewed & tested by the community

Great!

andypost’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new27.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,835 pass(es).
[ View ]
new4.23 KB

I think the removed hook_field_load should not be referenced
Also there's no need in .install file

Berdir’s picture

+++ /dev/null
@@ -1,63 +0,0 @@
-/**
- * Implements hook_install().
- */
-function field_test_install() {
-  // hook_entity_info_alter() needs to be executed as last.
-  module_set_weight('field_test', 1);
-}

I explicitly didn't remove the .install file because it has this hunk in there, which (even if not necessary anymore, not sure), has nothing to do with this and shouldn't be removed together with it?

Berdir’s picture

Also not sure why the getValue() change is necessary? But then !$this->isEmpty() would make more sense I think and the test should probably reference the fully namespaced classname.

andypost’s picture

StatusFileSize
new27.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es).
[ View ]
new2.06 KB

Fixed to full namespace.

+++ /dev/null
@@ -1,63 +0,0 @@
-function field_test_install() {
-  // hook_entity_info_alter() needs to be executed as last.
-  module_set_weight('field_test', 1);
-}

Was added in #565480: TF #2: Multilingual field handling and no more needed because field translation is changed

andypost’s picture

StatusFileSize
new27.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,915 pass(es).
[ View ]
new982 bytes

And missed isEmpty()

Berdir’s picture

@install stuff: Ok, it's quite possible that it's no longer used, but it's not related to this issue and it only removes 50% of it, there's still the actual hook implementation. I'd suggest to not touch that here :)

andypost’s picture

StatusFileSize
new27.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,840 pass(es).
[ View ]
new564 bytes
swentel’s picture

Status:Needs review» Reviewed & tested by the community

Ok, let's get this in now, good catches andypost!

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.