Problem/Motivation

Calling /commerce_product/autocomplete with no additional arguments generates the following PHP warning in dblog:

Warning: Missing argument 2 for commerce_product_autocomplete() in commerce_product_autocomplete() ...

Proposed resolution

Maybe a new access callback that just checks if there are sufficient arguments and return MENU_NOT_FOUND if not?

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama’s picture

Category: Bug report » Support request
Status: Active » Postponed (maintainer needs more info)

How does it get called with no arguments for you?

blasthaus’s picture

Hey Ryan, it's from a bot that seems to know that path is open ('access callback' => TRUE), most likely to see if you are running Drupal Commerce. So yeah it's just doing a GET on /commerce_product/autocomplete.

blasthaus’s picture

Category: Support request » Bug report
Status: Postponed (maintainer needs more info) » Active

To reproduce, just call /commerce_product/autocomplete and you get 3 php warnings and 4 notices:

Warning: Missing argument 1 for commerce_product_autocomplete()
Warning: Missing argument 2 for commerce_product_autocomplete()
Warning: Missing argument 3 for commerce_product_autocomplete()
Notice: Undefined variable: field_name in commerce_product_autocomplete() (line 726...
Notice: Undefined variable: entity_type in commerce_product_autocomplete() (line 727...
Notice: Undefined variable: field_name in commerce_product_autocomplete() (line 727...
Notice: Undefined variable: bundle in commerce_product_autocomplete() (line 727...

field_name phishing can also be done on this trajectory with a valid sku, i.e. /node/body/node/sku

rszrama’s picture

Ok, great - thanks for the info. We modeled this after the taxonomy autocomplete menu item, so I wonder if core has since implemented a fix for the similar issue. At the very least we can kill those warnings just by including default values in the function parameters and testing them for validity in the page callback.

blasthaus’s picture

Following up here. Would this work as a patch?

<?php
function commerce_product_autocomplete($entity_type = '', $field_name = '', $bundle = '', $string = '') {
  if (empty($entity_type) || empty($field_name) || empty($bundle)) {
    return MENU_NOT_FOUND;
  }
  //...
}
?>
adwuk’s picture

Had the same errors reported on a site I manage. Will try the change suggested in #5 as it looks simple enough to resolve.

joelpittet’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.57 KB

@rszrama should commerce_product/autocomplete get some kind of access control?

@blasthaus I think that I good start, here a patch for at least that.

blasthaus’s picture

@joelpittet We'd still need the default empty values for the function args to avoid the first 3 PHP warnings.

Also the patch looks like the last 2 diffs statements are just text-wrap related and should not be included.

The function here is modeled after taxonomy module.

The difference being, taxonomy deals only with the taxonomy_term_data table and this function can accept any Drupal entity args ($entity_type, $field_name, $bundle valid or not) in addition to providing a fully open path unlike taxonomy's hook_menu : 'access arguments' => array('access content'),

joelpittet’s picture

FileSize
915 bytes
1.76 KB

RE#8 Regarding the coding standards 80 character wrapping on that same function, I'd rather leave that in unless the maintainer has a problem with it.

Thanks for the point about the parameter defaults, I missed. that.

madelyncruz’s picture

#9 patch works.. thanks

joelpittet’s picture

Status: Needs review » Needs work

While this doesn't affect commerce, commerce_discount is (ab-)using these autocompletes.

commerce_product/autocomplete/commerce_product/0/0

Maybe we can let the bundle go to default, or possibly better the default for the field will allow?

mglaman’s picture

It could definitely use some work, here's the leading snippet from the page callback in taxonomy module

function taxonomy_autocomplete($field_name = '', $tags_typed = '') {
  // If the request has a '/' in the search text, then the menu system will have
  // split it into multiple arguments, recover the intended $tags_typed.
  $args = func_get_args();
  // Shift off the $field_name argument.
  array_shift($args);
  $tags_typed = implode('/', $args);

  // Make sure the field exists and is a taxonomy field.
  if (!($field = field_info_field($field_name)) || $field['type'] !== 'taxonomy_term_reference') {
    // Error string. The JavaScript handler will realize this is not JSON and
    // will display it as debugging information.
    print t('Taxonomy field @field_name not found.', array('@field_name' => $field_name));
    exit;
  }
madelyncruz’s picture

#9 works. Thank you. Hoping that they'll commit this patch.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
907 bytes

Let's go with this more specific approach so it doesn't break commerce_discount.

@mglaman what is #12 refering to?

It would be nice to remove the first parameter but that would break some BC, silly the entity_type is an option as it's for commerce_product autocomplete.

joelpittet’s picture

Issue tags: +commerce-sprint
smccabe’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, I talked to @mglaman and he just posted the taxonomy function in #12 for reference.

smccabe’s picture

rszrama’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.4 KB

Light bulb came on in the sprint room - default arguments will be sufficient, but we can really move the calls to the field info API later on in the function to also improve performance and avoid requiring the isset() checks anyways thanks to the argument count check. Patch attached.

  • rszrama committed b4cc209 on 7.x-1.x authored by joelpittet
    Issue #2399045 by joelpittet, rszrama: commerce_product_autocomplete...
rszrama’s picture

Status: Needs review » Fixed

Committed.

joelpittet’s picture

@rszrama the early return in #14 was intended in my case to avoid giving any chance for bots hiting that page and getting any further than needed into that function and avoiding processing (and in my case causing notices in logs for no good reason).

I may re-open this if the logs start showing bots stuff again.

rszrama’s picture

Oh, no worries, I tested it quite a bit to make sure there'd be no notices in the logs from it. Even as it was before, if you browsed to commerce_product/autocomplete/1/1/1 it would've proceeded through the function past your early exit.

Status: Fixed » Closed (fixed)

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

  • rszrama committed b6674ec on 7.x-1.x authored by mglaman
    Issue #2399045 follow-up by mglaman: minor tweak to autocomplete return...

Status: Fixed » Closed (fixed)

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