Skip to content
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

Pass ServiceBuilder construct config into children #816

Merged
merged 1 commit into from
Dec 28, 2017
Merged

Pass ServiceBuilder construct config into children #816

merged 1 commit into from
Dec 28, 2017

Conversation

jdpedrie
Copy link
Contributor

@jdpedrie jdpedrie commented Dec 26, 2017

So I've been investigating the report in #766, and found that the constructor $config is passed to services ONLY when no child $config is set.

For instance:

use Google\Cloud\ServiceBuilder;

putenv('GOOGLE_APPLICATION_CREDENTIALS=/path/to/keyfile1.json');

$cloud = new ServiceBuilder([
    'keyFilePath' => '/path/to/keyfile2.json'
]);

// uses `keyfile2.json`
$datastore = $cloud->datastore();

// uses `keyfile1.json`
$datastore = $cloud->datastore([
    'namespaceId' => 'myApp'
]);

This strikes me as incorrect, as it prevents e.g. SpeechClient from setting a languageCode and using ServiceBuilder-level credentials. My proposed solution simply merges ServiceBuilder and factory method configuration, preferring keys given later.

If accepted, this PR closes #766.

edit: for a demonstration, pull the PR and run vendor/bin/phpunit --group=servicebuilder. Then reset the changes to src/Core/ServiceBuilder.php to the current master branch. Undoing changes to ServiceBuilder will expose errors on SpeechClient.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 26, 2017
@jdpedrie jdpedrie merged commit 599ef81 into googleapis:master Dec 28, 2017
@jdpedrie jdpedrie deleted the servicebuilder-config branch December 28, 2017 17:00
@jdpedrie jdpedrie mentioned this pull request Dec 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UNAUTHENTICATED
3 participants