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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

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
FileSize
25.58 KB
12.49 KB

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

andypost’s picture

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

andypost’s picture

Fixed FieldInfoTest

andypost’s picture

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
FileSize
24.47 KB
24.31 KB
3.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
FileSize
24.49 KB
2.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

@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
FileSize
24.51 KB

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
FileSize
1.92 KB
24.56 KB

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

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
FileSize
27.02 KB
4.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

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

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

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.