Hi - Terrific and powerful module!
I am wondering why after all these years Services responds with a 401 Unauthorized for missing arguments rather than 400 Bad Request?
Ex. If a route is declared as PUT /soup/:id, and one of the required arguments is data: { temperature: hot }, and that value is not provided, I get a 401 but would expect and want a 400.
Is it just me and my convention, or is a 400 really more appropriate? Note: This occurs bc of the line:
return services_error(t('Missing required argument @arg', array('@arg' => $argument_info['name'])), 401);
and to change this behavior (as far as I know!) is just updating 401 to 400 in includes/RESTServer.inc:
return services_error(t('Missing required argument @arg', array('@arg' => $argument_info['name'])), 400);
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 2880909-missing-args-response-code.patch | 1.1 KB | texas-bronius |
| #2 | required-args-handling-2880909-2.patch | 898 bytes | tyler.frankenstein |
Comments
Comment #2
tyler.frankenstein commentedAgreed. We'll want to get a change log record mentioned for this, because some clients may be specifically watching for the 401 after all these years ;) - and we can give them a heads up to the change this way.
Kyle, any thoughts/long-term-implications on making a change to this response code?
Comment #3
kylebrowning commentedYeah it could break clients. Not sure how to handle it, but I would push it out ASAP so that it doesnt have to come with a forced update later incase we get anymore SA's.
ON the other hand, im sure someone will miss this and it will break.
Im wondering if we tag a 7.x-4.x for Services.
An alternative oslution, is to self apply the patch themselves. until we do reelase a 4.x
Comment #4
texas-bronius commentedComment #5
texas-bronius commentedWell.. that was certainly fast to get attention and eyes on it: Thank you! I can also appreciate the challenge with maintaining existing implementations. A very-Drupal workaround could be $conf or variable preference, settable to future 400, defaulting to existing 401 behavior. How rigid you make it (fixed 400/401 toggle vs a free form nnn code) is something else to consider.
Comment #6
texas-bronius commentedThe attached patch against master (which I believe to be 7.x-3.x-dev?):
* Out of the box responds with 401 Unauthorized (no change in behavior for existing users)
* Provides a $conf['services_deprecated_missing_arg_code'] allowing a concerned user to set to 400 (Bad Request)
* Provides comments in code about this and how to implement
Plan to obsolete this crutch in 7.x-4.x or as you see fit.
Comment #7
kylebrowning commentedComment #8
texas-bronius commentedWoops - Attached is better. Using variable_get() seems more appropriate and still (as before) allows variable_set() and $conf manipulation in settings.php just the same.
Comment #9
kylebrowning commentedI actually prefer $conf, because variable_gets can be a massive overhead on requests.
Comment #10
texas-bronius commentedThanks for your reply @kylebrowning. I think I get what you're saying, and I thought so, too, at first. Then I took a look under the hood to see what's what. The variable_get() function is doing exactly what I did before but without me having to clutter your codebase with a stray
global $conf;in the middle of a function call:The only overhead with variables occurs when they're first -all- loaded from db into the global $conf array at some point in bootstrap. Recall that we can still set the value for this conf/variable either by vset or from $conf in settings.php. Here's more:
https://drupal.stackexchange.com/a/187670
Comment #11
marcingy commentedI like the look of this patch and yes no overhead unless set in db, and even then by this point in time https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/_dru... will have been called and db values merged into conf values.
Comment #12
marcingy commentedForgot to do this!
Comment #13
kylebrowning commentedGo ahead, im cool with this.
Comment #15
tyler.frankenstein commentedThe patch didn't apply cleanly to the latest 7.x-3.x, so I touched it up and committed it.