We added drupalCreateField() and drupalCreateFieldInstance() in drupalWebTestCase, but our actual tests mostly use direct calls to field_create_field/instance().
We should either use those functions, or remove them completely if they're not fit / not actually needed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

puradata’s picture

Status: Active » Needs work
FileSize
3.84 KB

I changed some of the field_create_field calls to dupalCreateField calls, likewise for field_create_instance to drupalCreateFieldInstance. IMHO the drupalCreateField drupalCreateFieldInstance(which I’ll refer to the the drupal test calls) are not very useful. There was no mention of any other discussion or issue that lead up to the creation of the “drupal test calls”, so I don’t know why they were created. I am a drupal newbie and just starting to helping out with the field’s in core project. (need all the direction I can get) The inability to specify more field properties seems too limiting for the follow reasons:.
1. Fields
Fields cannot be changed after they are created. Therefore tests with a different cardinality than the default 1 cannot be run.
2. Field instances
Field instances can be modified with a call to field_update_instance. But if you want to specific more properties than the ones named as arguments for drupalCreateFieldInstance($field_name, $widget_type, $formatter_type, $ bundle) you have to call with field_update_instance() which takes an instance argument the same as field_create_instance() .
Another more subtle issue with instances. Quite often In testing, one wants to define a group of properties to test and refer them later. For example, maybe to verify that the properties were really created or to test for duplicates. Using a “definition instance “ array is a handy structure in this case. And it can also be used with field_create_instance()

All of the calls to field_create_instance() and field_create_field () that were not converted to the “drupal test calls” were for the above reasons. More calls were not converted than were.

yched’s picture

Thanks for looking into this !

drupalCreateField() does accept a $settings parameter which lets you specify all the properties of a $field.
But overall I think your analysis is correct. drupalCreateField() only brings the ability to omit an explicit field name, while drupalCreateFieldInstance() has no added value and limits the range of settings you can actually specify.
+ if we wanted to be consistent, we'd need a drupalUpdateFieldInstance() as well.

For their defense, those functions were created very early during the 'Fields in Core' Srpint, while the CRUD API itself was probably not fully shaped.
I'm in favor of removing them now.

puradata’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
2.33 KB

Yes I was wrong about drupalCreateField() not providing a way to change all possible properties. (newbieitis) My analysis is a lot weaker, but I still vote for removing drupalCreateField() and drupalFieldInstance(), mostly to achieve closure. Here I submit two patches one removes the calls in field.test and the other removes the functions drupalCreateField() and drupalCreateFieldInstance() file drupal_web_test_case.php

yched’s picture

Status: Needs review » Needs work

OK.
Please roll the patches in one single file. This is needed if we want the test bot to run, and simplifies the task of reviewers and committers.
Also, you'll find other calls to drupalCreateField/Instance() in modules/field/modules/text/text.test. I don't think there are other occurrences, but you might want to do a full test search to be sure.

puradata’s picture

combined all patches into one file also patched field\modules\text\text.test did search for drupalcreatefield and drupalcreatfieldinstance... all gone.

puradata’s picture

Status: Needs work » Needs review
yched’s picture

Status: Needs review » Needs work

The patch seems to have tabs / spaces issues. Make sure you setup your code editor to use 2 spaces and no tabs for indentation. That's important for any patch you might produce in the future :-)

There are several issues in this code block :

+	$field_definition = array(
+        'field_name' => drupal_strtolower($this->randomName()),
+		'max_lenght' => 3,
+		'type' => 'text'
+		)
+      );

- Extra closing parenthesis - parse error ? ;-)
- According to drupal coding style standards (http://drupal.org/coding-standards), there are a couple commas missing
- Typo : max_lenght
- The $field structure is not correct : max_length should be inside a 'settings' array
A fixed version would be something like :

+ $field_definition = array(
+   'field_name' => drupal_strtolower($this->randomName()),
+   'type' => 'text'
+   'settings' => array(
+     'max_length' => 3,
+   ),
+ );

Minor : Looks like the $field_definition variable is not needed in :

-    $this->field = $this->drupalCreateField('test_field');
+    $field_definition = array(
+      'field_name' => drupal_strtolower($this->randomName()),
+      'type' => 'test_field',
+    );
+    $this->field = $field_definition;
+    field_create_field($this->field);

Did you run the tests and see how they come up ? No need to run the full drupal test suite, just running the Field tests will give you a good notion of where the patch stands and detect bugs. The test bot will take care of running the whole suite

puradata’s picture

Status: Needs work » Needs review
FileSize
7.6 KB

Sorry last patch was a piece of garbage! This one fixes formatting errors, spelling errors and coding errors. Passes all 7 field simpletests.

Status: Needs review » Needs work

The last submitted patch failed testing.

puradata’s picture

I don't get it. This test passed everyday since april 6 but failed april 12. No changes?

puradata’s picture

Status: Needs work » Needs review

needs retesting

catch’s picture

puradata - usually that means another patch got in and sent yours out of date - you'll need to cvs update locally, try applying it there, and re-roll.

puradata’s picture

I updated my files locally and reapplied the patch. It worked fine just like it did on april 6. I would to get simpletest to try it again.

yched’s picture

Status: Needs review » Needs work

Indeed, patch needs a reroll.

puradata’s picture

Status: Needs work » Needs review
FileSize
5.97 KB

ah I see said the blind man
rerolled
retested

Dries’s picture

It looks ok to me but I'll let yched or bjaspan approve this.

yched’s picture

Status: Needs review » Needs work

In text.test :

+    $this->field = array(
+      'field_name' => drupal_strtolower($this->randomName()),
+      'max_length' => $max_length,
+      'type' => 'text',
+      'settings' => array(
+        'max_length' => $max_length,
+      )
+    );

The first 'max_length' line is not needed.

Other than that, RTBC for me.

yched’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.83 KB

Just rerolled for my comment above, and thus setting to RTBC. Don't credit me.

webchick’s picture

Title: use drupalWebTestCase::drupalCreateField/Instance ? » Remove drupalCreateField/Instance in favour of standard API functions
Status: Reviewed & tested by the community » Fixed

Big yay for having a single API we can call from either tests or field modules or wherever. This patch is a nice clean-up. Thanks, puradata and yched!

Committed to HEAD, and adjusting issue title accordingly.

I don't think we need upgrade docs for this cos I somehow doubt anyone was writing tests with this function outside of core in the past 2 months, but what do you think?

yched’s picture

"I doubt anyone was writing tests with this function outside of core in the past 2 months" : yes, highly unlikely. CCK and Barry's PBS module are most probably the only D7 field-related contrib module. CCK D7 has no tests right now, and PBS tests didn't use those two functions.

Thanks puredata !

Status: Fixed » Closed (fixed)

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