Skip to content
This repository has been archived by the owner on Jul 17, 2024. It is now read-only.

Fix #60 - rewrite for createClient() #63

Closed
wants to merge 1 commit into from
Closed

Fix #60 - rewrite for createClient() #63

wants to merge 1 commit into from

Conversation

cjcox17
Copy link

@cjcox17 cjcox17 commented May 13, 2019

First off, great work to the creators and or maintainers. I've relied on this project for a few years on numerous projects, it's been awesome, I'm glad that I can contribute here.

The issue started when key_file was allowed to be an array, the code was changed and we ran into the original array_merge issue #57 & #59 that I had created. The error was that array_merge would throw an exception that its given parameters could not be null. The fix was to write an is_array check, and if it failed then it would set $keyFile to an empty array.

While this fixed the array_merge issue, it did not fix the underlying problem, which was that the code, currently, will always return a new StorageClient with a key_file. Even if it is null, as long as the key 'key_file' exists the constructor array, the Google\Cloud\Storage\StorageClient will try to look for it, and it will fail throwing null errors on missing or incorrect data. This prevents and breaks all built-in service accounts from working without explicitly setting a .json file.

Now the key_file parameter (while it may work if a file path is passed into it, not sure) is actually for passing in the config in an array directly (like it says in the laravel-google-cloud-storage readme), and key_file_path is for the file path of the json file that Google\Cloud\Storage\StorageClient->Google\Cloud\Core\ClientTrait::getKeyFile() will read and run a json_decode(file_get_contents(key_file_path) on to get an array of parameters.

I rewrote createClient() to do the following (in pseudo code below)

`
get from config key_file_path
if is_string(key_file_path) and key_file_path is not empty()
Give us our storage client and tell Google to use our key_file_path file

if that fails

get from config key_file
if is_array(key_file) and key_file_path is not empty()
Give us our storage client and tell Google to use our key_file array as its config

if that fails

Give us our storage client and tell google we don't have any configuration to give it
`

Now that since Google didn't receive any configuration (given key_file_path and key_file are null), it searches on its own (GOOGLE_APPLICATION_CREDENTIALS, well known OS specific files, and built-in service accounts). With this patch, everything works as expected from the README.

That should bring this issue to a close. Sorry if my php-cs-fixer formatted the code base. It was automatic. I updated the readme to reflect the changes in the config, to match the new code, as well as updating the changelog.

* Support key_file_path and key_file separately (conforms with GCS class)

* Fix issue where in every case a key_file was given, breaking built-in service accounts

* Updated README with key_file, and key_file_path
@cjcox17
Copy link
Author

cjcox17 commented May 13, 2019

Turns out, you can pass null into key_file for the StorageClient and \Google\Cloud\Storage will still look for credentials on the server and not throw an error.

The issue before is that there was an array_merge(project_id, []) so that key_file contained a value which caused it to fail. Since it always defaulted null (in the config) in the case of Google App Engine or Google Compute Engine instances, the fix for the array_merge null issue was to make it an empty array and still merge it with the project_id, causing it to still fail

@cjcox17 cjcox17 closed this by deleting the head repository Feb 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant