There are a couple calls to _field_invoke() and _field_invoke_default() that are not working correctly because they are receiving the wrong parameters. We do a direct function check based on the $op value such as this:

$function = $field['module'] . '_field_' . $op;

However there a calls to "prepare translation" and "delete revision" that should be "prepare_translation" and "delete_revision" like the rest of the hook calls in field.module.

CommentFileSizeAuthor
field_invoke_underscores.patch1.37 KBquicksketch

Comments

quicksketch’s picture

quicksketch
he/him
commented
Title: Delete revision and prepare translation field hooks are called incorrect (or not at all) » Delete revision and prepare translation field hooks are called incorrectly (or not at all)
yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

Duh :-/

bjaspan’s picture

bjaspan commented
Status: Reviewed & tested by the community » Needs work

Hook tests would be pretty easy to write for these two hooks following the pattern laid out in #489438: hook_field_create_field() is missing, using field_test_memorize(). Is it reasonable to ask that the hook invocations addressed by this issue be tested along with the patch for this issue?

Incidentally, these bugs would have been found by the tests for all Field API hooks requested in #370872: Tests for field_attach hooks.

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs work » Reviewed & tested by the community

It seems fair enough that the bug would be fixed separate of the tests, especially since there's a pending issue (since February) to test such functionality. Tests shouldn't hold up fixing of critical bugs, especially since it's been noted that we need to add tests for this functionality anyway. Can't we fix this then continue in the issue you noted?

bjaspan’s picture

bjaspan commented

The committer can decide.

quicksketch’s picture

quicksketch
he/him
commented

This patch is part of several bugs that need to be fixed for #391330-33: File Field for Core to even start working somewhat correctly. I'd just prefer not to have such a slew of patches applied on my sandbox (not to mention potential testers) that complicate further development.

drewish’s picture

drewish commented

subscribing.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

I committed the patch to CVS HEAD to de-block other issues, but I'm marking it as 'code needs work' so we follow-up with tests. Thanks!

  • Dries committed 436fc4f on 8.3.x 
    - Patch #520620 by quicksketch: delete revision and prepare translation...

  • Dries committed 436fc4f on 8.3.x 
    - Patch #520620 by quicksketch: delete revision and prepare translation...

  • Dries committed 436fc4f on 8.4.x 
    - Patch #520620 by quicksketch: delete revision and prepare translation...

  • Dries committed 436fc4f on 8.4.x 
    - Patch #520620 by quicksketch: delete revision and prepare translation...

  • Dries committed 436fc4f on 9.1.x 
    - Patch #520620 by quicksketch: delete revision and prepare translation...

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.