Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#17 | field-hook-field-create-489438-16.patch | 4.41 KB | bjaspan |
#14 | field-hook-field-create-489438-14.patch | 2.49 KB | crotown |
#13 | field-hook-field-create-489438-13.patch | 11.54 KB | bjaspan |
#11 | field-hook-field-create-489438-11.patch | 9.57 KB | crotown |
#3 | field-hook-field-create-489438-3.patch | 2.28 KB | crotown |
Comments
Comment #2
crotown CreditAttribution: crotown commentedOK, I'll give it a try...
Comment #3
crotown CreditAttribution: crotown commentedHere 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.
Comment #4
bjaspan CreditAttribution: bjaspan commentedThis 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
Comment #5
bjaspan CreditAttribution: bjaspan commentedforgot to change status
Comment #6
Dries CreditAttribution: Dries commentedAren'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.
Comment #7
crotown CreditAttribution: crotown commentedAt 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.
Comment #8
bjaspan CreditAttribution: bjaspan commented@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. :-)
Comment #9
yched CreditAttribution: yched commentedThere'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.
Comment #10
crotown CreditAttribution: crotown commentedYes, 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...
Comment #11
crotown CreditAttribution: crotown commentedHere 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...
Comment #12
webchickMarking needs review.
Comment #13
bjaspan CreditAttribution: bjaspan commentedOkay, 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. :-)
Comment #14
crotown CreditAttribution: crotown commentedThanks, 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.
Comment #15
bjaspan CreditAttribution: bjaspan commentedLooks good!
Comment #16
webchickThis 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.
Comment #17
bjaspan CreditAttribution: bjaspan commentedYour 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...
Comment #18
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #19
webchickGreat job, crotown! :) Welcome to the core developer hall of fame!
Comment #20
crotown CreditAttribution: crotown commentedThank you, webchick. It's a milestone! Oh, and thanks for all the education you've given me through the Lullabot podcast.