Posted by bjaspan on June 12, 2009 at 4:02am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | crotown |
| Status: | closed (fixed) |
Issue Summary
We have hook_field_create_instance() but no hook_field_create_field(). This should be a simple fix. Make sure to include a test case, probably by implementing field_test_field_create_field() and verifying that it is called.
This is a good issue for someone just getting into Field API.
Comments
#2
OK, I'll give it a try...
#3
Here is my patch that adds the missing invocation of the hook to field_create_field and adds a test to the "Field CRUD tests" to test that the hook actually gets called on a successful call to field_create_field(). The patch includes bjaspan's field_test_memorize() function from #367753: Bulk deletion.
#4
This looks great. RTBC.
One minor point: In general, when you create core patches, you want to use (or, rather, the core committers want you to use) the cvs diff -p or -F^fc option. This includes info about which functions are being patched, which is helpful when reviewing. See http://drupal.org/patch/create. In my ~/.cvsrc file, I have
diff -u -F^[fc]#5
forgot to change status
#6
Aren't we testing the invocation of other fields hooks? It looks like this _memorize() function introduces a new pattern not used for other field hook tests.
#7
At the moment we are not testing that the other field hooks are being invoked. I agree that it makes sense to add those tests now that we have the ability to do so easily using _memorize(). I will go ahead and add the tests.
#8
@dries: No, we are not currently testing the other field hooks or, AFAIK, any other module's hooks in this way. I'm certainly happy to have them tested, but I suggest that we not impose to much scope-creep on this small issue given that a Field API (and Drupal) newcomer, whom we want to encourage to remain involved, is working on it.
OTOH, more scope creep == more cred for Steve. :-)
#9
There's one place in Field tests where we are currently testing hook invocation : hook_field_load() - although we don't only check it's being called, but that it has the intended effect.
#10
Yes, I found 11 of the hooks in field.api.php are actually used in tests. I've implemented the other 39 to just call _memorize and am planning to work checks into existing testing code were those hooks should now get called. I used to think this issue should have been a 1, now I'm wondering if the rating is rising above 2...
#11
Here is the patch with 38 more hook implementations added for the untested hooks. I got stuck adding tests and would like some help from the community. Adding an automated test for hook_field_presave (the first as-yet untested hook) might show me the way forward on the other 37...
#12
Marking needs review.
#13
Okay, this issue/situation has gotten a bit unnecessarily out of hand. There is still work to do, but less than it appears. :-)
Back in #6, Dries wrote, "Aren't we testing the invocation of other fields hooks?" I thought he wrote "Are we testing...", thus implying that we should be. In fact, I now realize he was asking, "I think we are already testing other field hooks using a method other than this field_test_memorize() method, so why don't we use that existing method to test hook_field_create_field() instead of this new one?" The answer to his question is, "No, prior to this patch, we are not explicitly testing any field hooks (except those implicitly/accidentally tested because they perform actual operations that are tested, like some of text.module's hooks); this patch adds the first explicit field hook test, for hook_field_create_field()."
In short, I no longer think Dries was asking to scope-creep this issue to include tests for all Field API hooks. So the patch in #3 could still be RTBC.
However, the patch in #3 only verifies that hook_field_create_field() is invoked the correct number of times (once), instead of verifying the arguments it is passed. I'd rather see the arguments verified, too. So I think the patch is not quite RTBC.
I've attached a new patch based on #11 that does what Steve asked: It implements tests for field_attach_presave(), which prior to this patch is not tested at all, anywhere. It tests that the hooks hook_field_presave() and hook_field_attach_presave() are both called (since that is actually all that field_attach_presave() does). Steve, you can use this as an example to test what is passed to hook_field_create_field(). Then, I suggest removing everything NOT related to field_create_field() from the patch and re-submitting it.
We can then create a separate issue to add explicit tests for all field hooks, based on the patch in #11/this one.
Questions welcome. :-)
#14
Thanks, Barry for the testing code and opinion on straightening this out. Attached is a new patch based on the patch in #3 that also checks that the correct arguments are passed to the hook, not just that it is called.
I agree that we should open separate issue for adding test cases for the remaining 37 hooks to test.
#15
Looks good!
#16
This patch is too over my head for my current fatigue level (plus Dries looks like he has it), but function field_test_memorize is missing PHPDoc. Let's add a line to explain what's going on there.
Otherwise, code style looks good.
#17
Your wish is my command.
Clearly, field_test_memorize() is a pattern that can be useful for many modules outside Field API, but let's not hold up this bugfix for that...
#18
Committed to CVS HEAD. Thanks.
#19
Great job, crotown! :) Welcome to the core developer hall of fame!
#20
Thank you, webchick. It's a milestone! Oh, and thanks for all the education you've given me through the Lullabot podcast.
#21
Automatically closed -- issue fixed for 2 weeks with no activity.