Enable default base uri to be overridden #137
Conversation
|
Thanks for this - GH Actions looks to be degraded currently so it might take some time to run tests. Is there a way we can add tests for this change too please? |
Codecov Report
@@ Coverage Diff @@
## master #137 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 237 239 +2
===========================================
Files 82 82
Lines 1178 1183 +5
===========================================
+ Hits 1178 1183 +5
Continue to review full report at Codecov.
|
src/Connector/Connector.php
Outdated
| @@ -39,6 +44,10 @@ class Connector implements ConnectorInterface | |||
| */ | |||
| public function __construct(array $config) | |||
| { | |||
| $this->baseUri = ConnectorInterface::BASE_URI; | |||
| if (getenv('ACQUIA_CLOUD_API_BASE_URI')) { | |||
| $this->baseUri = getenv('ACQUIA_CLOUD_API_BASE_URI'); | |||
There was a problem hiding this comment.
While reading this and looking through ConnectorInterface::BASE_URI, I was really starting to wonder why this wouldn't be taken one step further. Thing is, its really hard to mock a environment variable in tests if you wanted to write a unittest for Connector.
Wouldn't it make more sense to add a optional and secundary constructor parameter here?
public function __construct(array $config, $base_uri = NULL)
That would decouple this from environment variables entirely and put the responsibility of what to connect to, upon the consumer of the API.
There was a problem hiding this comment.
Thing is, its really hard to mock a environment variable in tests if you wanted to write a unittest for Connector.
I've successfully tested environment variables in a unit test over here. I think I pulled this as an example from somewhere in Symfony as they do similar over there.
https://github.com/typhonius/acquia_cli/blob/master/tests/AcquiaCliApplicationTest.php#L47-L81
Wouldn't it make more sense to add a optional and secundary constructor parameter here?
As the usecase here is for a tiny subset of the users of the library, I'm not sure what the best practice would be. 99.9% of users would never change this, but I'm really not sure whether it would be better injected or as an environment var. If there's precedence somewhere or a similar library that we can follow then that would help guide the approach.
grasmash
changed the title
|
@typhonius How's this? |
|
Thanks @grasmash at a glimpse this looks great. Let me merge it over the next couple days. |
|
Did this ever actually work for any of y'all? I don't see how it could have, since you would have still been authenticating against the production Accounts endpoint. You wouldn't be authenticated against the stage realm, so how could you be making stage API calls? |
This enables testing of non production instances of Cloud API.