Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#18 | removedrupalcreatefield-instances-368639-18.patch | 5.83 KB | yched |
#15 | removedrupalcreatefield-instances-15.patch | 5.97 KB | puradata |
#8 | removedrupalcreatefield-instances-8.patch | 7.6 KB | puradata |
#5 | removedrupalcreatefield-instances-5.patch | 6.43 KB | puradata |
#3 | remove_drupalCreateField-3.patch | 2.33 KB | puradata |
Comments
Comment #1
puradata CreditAttribution: puradata commentedI 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.
Comment #2
yched CreditAttribution: yched commentedThanks 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.
Comment #3
puradata CreditAttribution: puradata commentedYes 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
Comment #4
yched CreditAttribution: yched commentedOK.
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.
Comment #5
puradata CreditAttribution: puradata commentedcombined all patches into one file also patched field\modules\text\text.test did search for drupalcreatefield and drupalcreatfieldinstance... all gone.
Comment #6
puradata CreditAttribution: puradata commentedComment #7
yched CreditAttribution: yched commentedThe 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 :
- 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 :
Minor : Looks like the $field_definition variable is not needed in :
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
Comment #8
puradata CreditAttribution: puradata commentedSorry last patch was a piece of garbage! This one fixes formatting errors, spelling errors and coding errors. Passes all 7 field simpletests.
Comment #10
puradata CreditAttribution: puradata commentedI don't get it. This test passed everyday since april 6 but failed april 12. No changes?
Comment #11
puradata CreditAttribution: puradata commentedneeds retesting
Comment #12
catchpuradata - 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.
Comment #13
puradata CreditAttribution: puradata commentedI 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.
Comment #14
yched CreditAttribution: yched commentedIndeed, patch needs a reroll.
Comment #15
puradata CreditAttribution: puradata commentedah I see said the blind man
rerolled
retested
Comment #16
Dries CreditAttribution: Dries commentedIt looks ok to me but I'll let yched or bjaspan approve this.
Comment #17
yched CreditAttribution: yched commentedIn text.test :
The first 'max_length' line is not needed.
Other than that, RTBC for me.
Comment #18
yched CreditAttribution: yched commentedJust rerolled for my comment above, and thus setting to RTBC. Don't credit me.
Comment #19
webchickBig 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?
Comment #20
yched CreditAttribution: yched commented"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 !