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.
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
Comment | File | Size | Author |
---|---|---|---|
#18 | 2399045-18.product_autocomplete_arguments.patch | 1.4 KB | rszrama |
| |||
#14 | 2399045-14.patch | 907 bytes | joelpittet |
|
Comments
Comment #1
rszrama CreditAttribution: rszrama commentedHow does it get called with no arguments for you?
Comment #2
blasthaus CreditAttribution: blasthaus commentedHey 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.
Comment #3
blasthaus CreditAttribution: blasthaus commentedTo 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
Comment #4
rszrama CreditAttribution: rszrama commentedOk, 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.
Comment #5
blasthaus CreditAttribution: blasthaus commentedFollowing up here. Would this work as a patch?
Comment #6
adwuk CreditAttribution: adwuk commentedHad the same errors reported on a site I manage. Will try the change suggested in #5 as it looks simple enough to resolve.
Comment #7
joelpittet@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.
Comment #8
blasthaus CreditAttribution: blasthaus commented@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'),
Comment #9
joelpittetRE#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.
Comment #10
madelyncruz CreditAttribution: madelyncruz commented#9 patch works.. thanks
Comment #11
joelpittetWhile 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?
Comment #12
mglamanIt could definitely use some work, here's the leading snippet from the page callback in taxonomy module
Comment #13
madelyncruz CreditAttribution: madelyncruz commented#9 works. Thank you. Hoping that they'll commit this patch.
Comment #14
joelpittetLet'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.
Comment #15
joelpittetComment #16
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedLooks good to me, I talked to @mglaman and he just posted the taxonomy function in #12 for reference.
Comment #17
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedComment #18
rszrama CreditAttribution: rszrama at Centarro commentedLight 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.
Comment #20
rszrama CreditAttribution: rszrama at Centarro commentedCommitted.
Comment #21
joelpittet@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.
Comment #22
rszrama CreditAttribution: rszrama at Centarro commentedOh, 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.