-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Check if the param is an array before trying to access it #1762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
Thanks for the contribution @negoziator. Could you provide an example of where this is causing an error? I'm rather surprised it hasn't been seen before. |
src/Google/Service/Resource.php
Outdated
| $uriTemplateVars = array(); | ||
| $queryVars = array(); | ||
| foreach ($params as $paramName => $paramSpec) { | ||
| if (gettype($paramSpec) !== 'array') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm interested to see that you started with is_array before switching. What was the issue with is_array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally i like the is_array over the gettype, but i actually didn't think that is_array was compatible with php 5.4
I can change it back to is_array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_array has been available since PHP 4. Definitely prefer it over gettype.
|
@jdpedrie I closed this PR, since i did some digging. I found out that it was actually the "Colors" class that have a fun unexpected null behavior. I'll open another PR, that takes care of the root-cause instead. |
|
@jdpedrie I've opened another PR :) |
|
Copying my comment from the other pull request since I had to close it. :( @negoziator unfortunately, this repository contains generated code and can't be modified directly. I'm curious about this though:
When I run similar code, I don't see it casting anything to null: function foo($arg = []) {
$other = [];
print_r(array_merge($other, $arg));
}
foo();
// Array
// (
// )I think there might be an issue a bit deeper which we need to keep looking for. If it happens that there is a generator problem, we can work on addressing it there, but I'm not entirely sure that's the case yet. Let's continue the discussion and research in the original PR since I think it's likely a fix will originate there. :) |
|
So i created a snippet here, where i try to explain the issue. See the snippet here, and let me know what you think :) https://web.tinkerwell.app/#/snippets/fe2fc249-1a4e-4ed8-a005-42134f5bd7ac |
|
@jdpedrie ping :) I know the snippet is handling it, where it cant't be handled. I'm not sure how to fix the root cause, since it's in the |
|
It seems like a global issue of each method accepting any type when it only actually works with an array value. Ideally the generator should be updated to typehint In the interim, why not avoid sending |
Sometimes the request's parameters will end up with the last element is an integer.
That will result in a PHP error.
You cannot access an array with a type Integer
Tested on: PHP 7.4