stock-api-libphp
stock-api-libphp copied to clipboard
Added ability to specify x-request-id header
Added ability to specify x-request-id header.
There are a lot of code changes here for one new header. Can you summarize all the changes and how you implemented this? Some of the changes are only formatting. Also, I sense this is more work than is needed. There is already support for request-id, but it is always static. It seems you could add an optional parameter in this line, and therefore not need all the other changes: https://github.com/adobe/stock-api-libphp/blob/99a6786ca6b92cebab1f274be7a4a765250a59d7/src/Utils/APIUtils.php#L36
public static function generateCommonAPIHeaders(CoreConfig $config, string $access_token = null) : array
{
$request_id = static::getUUID();
if ($access_token !== null) {
$access_token = 'Bearer ' . $access_token;
}
$headers = [
'headers' => [
'x-api-key' => $config->getApiKey(),
'x-product' => $config->getProduct(),
'Authorization' => $access_token,
'x-request-id' => $request_id,
],
];
return $headers;
}
@chfabbro thanks for the review!
To summarize:
-
I've added property and methods
setRequestId
andgetRequestId
to all the request classes (extracting therequestId
andlocale
field to an abstract class to avoid code duplication). -
I've added overriding of common headers retrieved from
APIUtils::generateCommonAPIHeaders
to set thex-request-id
header in all the Client classes. (I wasn't able to find a good and backward compatible approach to avoid code duplication in this case)if (!empty($request->getRequestId())) { $headers['headers']['x-request-id'] = $request->getRequestId(); }
-
Code formatting was done automatically by the IDE - unnecessary spaces removed from empty lines - let me know if you'd like this formatting to be removed from the pull request.
I didn't add an additional parameter to generateCommonAPIHeaders
as I thought this method responsibility is to provide default headers (conclusion based on method name), also if that parameter will be added - all client classes will need to be updated anyway to pass the parameter.
Let me know if I should add an additional optional $requestId
parameter to generateCommonAPIHeaders
to avoid adding of if
conditions to each client class.
Hmm... I get multiple errors when I try to run the tests. Were you able to run tests, or were you not able to install astock/phpcs-psr2-stock
? Unfortunately that is an internal library--yet another reason why Adobe is not good at open source! (Note that there is no automation with public Github and our repos, so no tests are run. You must run them yourself before submitting. Our internal Adobe git repo has integrated testing, but that is private to Adobe.)
@chfabbro if you confirm the changes, I will adjust the tests and add test coverage for the introduced functionality.
I will need to wait for @slaanesh to review. He is the final approver of any changes to public Git, and created the rules for how submissions get approved. He is also much more familiar with PHP and code style than I am.
@chfabbro tests are fixed and new functionality test coverage is provided.
I did also fix the AdobeStock\Api\Test\LicenseFactoryTest::downloadAssetRequestShouldReturnValidRequestObject
that is failing on master branch.
@sivaschenko Thanks! I sent you an offline Slack message. I will get a dev resource to help me review both of your PRs. I will also add you to the internal JIRA tickets I am creating for these.
Hi @chfabbro the formatting changes are removed from the pull request