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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

crotown’s picture

Assigned: Unassigned » crotown

OK, I'll give it a try...

crotown’s picture

Status: Active » Needs review
FileSize
2.28 KB

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.

bjaspan’s picture

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]
bjaspan’s picture

Status: Needs review » Reviewed & tested by the community

forgot to change status

Dries’s picture

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.

crotown’s picture

Status: Reviewed & tested by the community » Needs work

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.

bjaspan’s picture

@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. :-)

yched’s picture

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.

crotown’s picture

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...

crotown’s picture

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...

webchick’s picture

Status: Needs work » Needs review

Marking needs review.

bjaspan’s picture

Status: Needs review » Needs work
FileSize
11.54 KB

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. :-)

crotown’s picture

Status: Needs work » Needs review
FileSize
2.49 KB

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.

bjaspan’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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.

bjaspan’s picture

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

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...

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

webchick’s picture

Great job, crotown! :) Welcome to the core developer hall of fame!

crotown’s picture

Thank you, webchick. It's a milestone! Oh, and thanks for all the education you've given me through the Lullabot podcast.

Status: Fixed » Closed (fixed)

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