This causes PHP warning and notices:
Warning: Missing argument 1 for taxonomy_autocomplete() in taxonomy_autocomplete() (line 79 of .../modules/taxonomy/taxonomy.pages.inc).
Notice: Undefined variable: field_name in taxonomy_autocomplete() (line 80 of .../modules/taxonomy/taxonomy.pages.inc).
Notice: Undefined variable: term_matches in taxonomy_autocomplete() (line 126 of .../modules/taxonomy/taxonomy.pages.inc).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

starsinmypockets’s picture

sub

swentel’s picture

Status: Active » Needs review
FileSize
856 bytes

Patch attached.

c960657’s picture

Is this still an issue in D8 after #93854: Allow autocompletion requests to include slashes has been fixed?

fangel’s picture

Swentel: The $matches array is never used, only $term_matches is - so you can simplify the code further by removing $matches..

You can see my patch in the more-or-less duplicate issue #665420: taxonomy autocomplete throws notice on non match

xjm’s picture

Title: Prevent taxonomy/autocomplete path from direct access » Notices in taxonomy_autocomplete()
Status: Needs review » Needs work
Issue tags: +Needs tests, +Novice

The patch here also needs to be rerolled for the core/ change at least, and we should add a test.

Tagging as novice for two tasks:

  • Reroll the patch and remove the unused $matches variable as suggested above. (See the patch in #665420: taxonomy autocomplete throws notice on non match.)
  • Add an automated test. (The test can probably be a simple matter of visiting the path and ensuring that there are no notices.) The test should fail when applied on its own, and pass when combined with the patch here. You can upload both the tests-only and combined patches in the issue to expose the test failure for reviewers.
nanotube’s picture

Assigned: Unassigned » nanotube

I will take a stab at this, give me some time to figure out how to write the test for this.

xjm’s picture

@nanotube -- Are you still looking into this issue? If not we'll unassign it for someone else to take a crack at it. Thanks!

swentel’s picture

Assigned: nanotube » swentel

I'll take it this weekend.

xjm’s picture

Thanks swentel. Since this is your patch to begin with that make sense. In general though, could you give the assignee maybe a day or to two reply, just in case they're about to post something? (Especially for novice-tagged issues.) Thanks!

swentel’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
865 bytes

Two patches, first one with test alone, this will fail. Second one with fix, should pass.

oriol_e9g’s picture

Status: Needs review » Needs work

We need to document the new optional variable:

 * @param $field_name
 *   The name of the term reference field.

Should be:

 * @param $field_name
 *   (optional) The name of the term reference field.
swentel’s picture

I'm not sure about that, since without a field_name this function simply does nothing. The patch simply makes sure no notices are triggered, let's get some more opinions on this.

oriol_e9g’s picture

Status: Needs work » Needs review

Yes, you're right.

xjm’s picture

I'm not sure about that, since without a field_name this function simply does nothing. The patch simply makes sure no notices are triggered, let's get some more opinions on this.

Hmm, good point. In that case we should document that fact in the parameter documentation, I think.

nanotube’s picture

Thanks swentel! I got tied up. I thought the test result was supposed to be "[ ]", never got it to pass on my end.

JvE’s picture

Re-rolled for changes to tests in D8.
And to bump the issue.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs backport to D6, +Novice, +Needs backport to D7

The last submitted patch, drupal-taxonomy-autocomplete-notices-combined-1242602-16.patch, failed testing.

swentel’s picture

Version: 8.x-dev » 7.x-dev
Assigned: swentel » Unassigned

This isn't relevant anymore for D8

star-szr’s picture

Thanks @swentel! Backporting the patch from #16 for Drupal 7 would be a great novice task.

Edit: Instructions here.

babruix’s picture

Status: Needs work » Needs review
FileSize
2.3 KB

Attached backport patch for D7.

Status: Needs review » Needs work
Issue tags: -Needs backport to D6, -Novice, -Needs backport to D7

The last submitted patch, drupal-taxonomy-autocomplete-notices-combined-1242602-21.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D6, +Novice, +Needs backport to D7
oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community

Go with this! It's only a notice removal!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/054c8ab

This also has the "needs backport to D6" tag, so if someone wants to reopen it for backport please feel free. (However, it's my impression that non-security-related patches have not been committed to Drupal 6 for a long time.)

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