Checkout openapi generator upgrade - #937
ashwaniarya-adyen wants to merge 28 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request upgrades the library's PHP requirement to ^8.1, updates dependencies, and introduces core services, configurations, and a large set of auto-generated model classes for the BinLookup and Checkout APIs. Key feedback includes addressing a critical missing import of AdyenException in BaseService.php that would cause runtime failures, fixing a regex range bug and a float-to-int precision issue in HeaderSelector.php, and adding error handling for fopen along with fixing a falsy stream-reading loop in ObjectSerializer.php. Additionally, it is recommended to adopt PHP 8.1's str_contains instead of strpos and correct a parameter type mismatch in HeaderSelector.php.
I am having trouble creating individual review comments. Click here to see my feedback.
src/Adyen/BaseService.php (3)
The AdyenException class is defined in the Adyen\\Exception namespace. Since BaseService is in the Adyen namespace, throwing AdyenException without importing it will result in a runtime Class \"Adyen\\AdyenException\" not found error. Please add a use statement to import Adyen\\Exception\\AdyenException.
namespace Adyen;
use Adyen\Exception\AdyenException;src/Adyen/HeaderSelector.php (99)
In PHP's PCRE, an unescaped - between two characters in a character class defines a range. Here, +-^ defines a range from + (ASCII 43) to ^ (ASCII 94), which matches many unintended characters (such as /, @, A-Z, etc.). To match a literal -, it should be escaped as \\-.
return preg_match('~^application/(json|[\w!#$&.+\-^_]+\+json)\s*(;|$)~', $searchString) === 1;src/Adyen/HeaderSelector.php (176)
Casting the result of a float multiplication to (int) can lead to incorrect values due to floating-point precision issues in PHP. For example, (int)(\"0.58\" * 1000) evaluates to 579 instead of 580. Using round() before casting to (int) ensures correct rounding.
'weight' => (int) round($outputArray[2] * 1000),src/Adyen/Model/BinLookup/ObjectSerializer.php (461-465)
If fopen fails to open the file, it returns false. Writing to or closing a boolean value will trigger PHP warnings/errors. You should handle the failure of fopen explicitly.\n\nAdditionally, the loop condition while ($chunk = $data->read(200)) will terminate prematurely if a chunk of the stream is exactly \"0\", because \"0\" is falsy in PHP. The loop should check against '' explicitly.
$file = fopen($filename, 'w');
if ($file === false) {
throw new \RuntimeException("Failed to open file for writing: " . $filename);
}
while (($chunk = $data->read(200)) !== '') {
fwrite($file, $chunk);
}
fclose($file);src/Adyen/BaseService.php (52)
Since PHP 8.1 is now required, you can use the more readable and type-safe str_contains() function instead of strpos() !== false.
if (str_contains($url, "pal-")) {src/Adyen/BaseService.php (60-62)
Since PHP 8.1 is now required, you can use the more readable and type-safe str_contains() function instead of strpos() !== false.
if (str_contains($url, "checkout-")) {
// Add live prefix for Checkout endpoints
if (str_contains($url, "possdk")) {src/Adyen/HeaderSelector.php (194)
The $currentWeight parameter is declared as float &$currentWeight, but it is always treated as an integer (initialized to 1000 and updated by getNextWeight(), which returns int). It should be typed as int &$currentWeight to maintain type consistency and avoid implicit float coercions.
private function adjustWeight(array $headers, int &$currentWeight, bool $hasMoreThan28Headers): array| @@ -169,9 +171,9 @@ use {{modelPackage}}\ObjectSerializer; | |||
| * @deprecated | |||
| {{/isDeprecated}} | |||
| */ | |||
| public function {{operationId}}({{^exts.x-group-parameters}}{{#allParams}}{{dataType}} ${{paramName}}{{^isBodyParam}}{{^required}} = {{#defaultValue}}{{{.}}}{{/defaultValue}}{{^defaultValue}}null{{/defaultValue}}{{/required}}{{/isBodyParam}}, {{/allParams}}{{#servers}}{{#-first}}?int $hostIndex = null, array $variables = [], {{/-first}}{{/servers}}?\{{invokerPackage}}\RequestOptions $requestOptions = null{{/exts.x-group-parameters}}{{#exts.x-group-parameters}}$associative_array{{/exts.x-group-parameters}}): {{#returnType}}{{.}}{{/returnType}}{{^returnType}}void{{/returnType}} | |||
| public function {{operationId}}({{^exts.x-group-parameters}}{{#pathParams}}{{dataType}} ${{paramName}}, {{/pathParams}}{{#bodyParams}}{{dataType}} ${{paramName}}, {{/bodyParams}}{{#formParams}}{{#required}}{{#notRequiredOrIsNullable}}?{{/notRequiredOrIsNullable}}{{dataType}} ${{paramName}}, {{/required}}{{/formParams}}{{#queryParams}}{{#required}}{{#notRequiredOrIsNullable}}?{{/notRequiredOrIsNullable}}{{dataType}} ${{paramName}}, {{/required}}{{/queryParams}}{{#formParams}}{{^required}}?{{dataType}} ${{paramName}} = {{#defaultValue}}{{{.}}}{{/defaultValue}}{{^defaultValue}}null{{/defaultValue}}, {{/required}}{{/formParams}}{{#queryParams}}{{^required}}?{{dataType}} ${{paramName}} = {{#defaultValue}}{{{.}}}{{/defaultValue}}{{^defaultValue}}null{{/defaultValue}}, {{/required}}{{/queryParams}}{{#servers}}{{#-first}}?int $hostIndex = null, array $variables = [], {{/-first}}{{/servers}}?\{{invokerPackage}}\RequestOptions $requestOptions = null{{/exts.x-group-parameters}}{{#exts.x-group-parameters}}$associative_array{{/exts.x-group-parameters}}): {{#returnType}}{{.}}{{/returnType}}{{^returnType}}void{{/returnType}} | |||
There was a problem hiding this comment.
Earlier, parameters were generated in the same order as the API specification. This exposed the optional headers as a PHP method argument before the required request body, causing PHP 8.4 warnings and making it behave as required. The template now excludes header parameters from method arguments, places required parameters before optional ones, and handles headers through RequestOptions
| @@ -241,7 +241,7 @@ class {{classname}} {{#parentSchema}}extends {{{parent}}}{{/parentSchema}}{{^par | |||
|
|
|||
| {{/parentSchema}} | |||
| {{#vars}} | |||
| $this->setIfExists('{{name}}', $data ?? [], {{#defaultValue}}{{{defaultValue}}}{{/defaultValue}}{{^defaultValue}}null{{/defaultValue}}); | |||
There was a problem hiding this comment.
The initial v7 generation automatically assigned default values from the API specification when creating a model. This caused fields that the user did not provide, to be included in outgoing requests. The previous Checkout version left unset fields as null and omitted them during serialization. This change restores that behavior.
|
|
||
| // initialize service | ||
| $service = new \Adyen\Service\Checkout\PaymentsApi($client); | ||
| $client = $this->createMockSerializerClient($jsonFile, $httpStatus); |
There was a problem hiding this comment.
These tests previously created generated Checkout APIs using the legacy Adyen\Client. The v7-generated APIs now require an Adyen\Configuration and a Guzzle HTTP client separately. The tests now extend BaseTest and use its existing helpers to create both objects.
| { | ||
| $json = file_get_contents($jsonFile); | ||
| $json = $jsonFile !== null ? file_get_contents($jsonFile) : ''; |
There was a problem hiding this comment.
This test helper previously required a JSON file. Some API calls return no response body, such as a successful delete request with status 204. The helper now allows no JSON file and uses an empty response instead.
| $service = new RecurringApi($client); | ||
|
|
||
| $service->deleteTokenForStoredPaymentDetails("123"); | ||
| $service->deleteTokenForStoredPaymentDetails("paymentMethodId", "shopperReference", "merchantAccount"); |
There was a problem hiding this comment.
This test previously passed only the stored payment method ID. In Checkout v72, the shopper reference and merchant account are also required, so the test now provides all three values.
| // And assert that the result is equal to a deep json encode/decode | ||
| #$this->assertEquals($result->toArray(), json_decode(json_encode($result->jsonSerialize()), true)); | ||
| $this->assertEquals(ObjectSerializer::sanitizeForSerialization($result), json_decode(json_encode($result->jsonSerialize()), true)); | ||
| $this->assertEquals( |
There was a problem hiding this comment.
This test previously compared an object with an array, so it failed even when both contained the same data. It now compares two arrays and correctly checks that toArray() returns the same data as JSON serialization.
| {{/vars}} | ||
| {{#discriminator}} | ||
|
|
||
| // Initialize discriminator property with the model name. | ||
| $this->container['{{discriminatorName}}'] = static::$openAPIModelName; | ||
| if (!array_key_exists('{{discriminatorName}}', $data ?? [])) { |
There was a problem hiding this comment.
The first v7 generation always replaced the model discriminator after loading constructor data. For example, a supplied payment method type of paypal was replaced by CheckoutPaymentMethod. The generated model name is now used only when the input does not already contain a type.
| * | ||
| * @var string | ||
| */ | ||
| protected string $libraryName = self::LIB_NAME; |
There was a problem hiding this comment.
The initial v7 configuration lost this information, so this PR adds the library name and version to Configuration.
| public function getUserAgent(): string | ||
| { | ||
| return $this->userAgent; | ||
| $suffix = self::LIB_NAME . '/' . self::LIB_VERSION; |
There was a problem hiding this comment.
The initial v7 generation used OpenAPI-Generator/1.0.0/PHP as the User-Agent. This PR restores the SDK identity:
adyen-php-api-library/{version}
If an application name is configured, it is placed before the library identity.
| @@ -670,9 +669,13 @@ use {{modelPackage}}\ObjectSerializer; | |||
| $requestOptions | |||
| ); | |||
|
|
|||
| $headers['adyen-library-name'] = $this->config->getLibraryName(); | |||
There was a problem hiding this comment.
Compared with the initial generation, every request now includes adyen-library-name and adyen-library-version. This restores the identification headers used by the Adyen PHP library.
| * @param object|null $requestModel | ||
| * @return object|null | ||
| */ | ||
| protected function injectApplicationInfo(?object $requestModel): ?object |
There was a problem hiding this comment.
Restored the previous behavour. The initial v7 generation dropped it. The injectApplicationInfo() helper restores the old behavior, it handles applicationInfo set as an array or a model object and leaves any other merchant-provided fields untouched - here
| // for model (json/xml) | ||
| {{#bodyParams}} | ||
| if (isset(${{paramName}})) { | ||
| ${{paramName}} = $this->injectApplicationInfo(${{paramName}}); |
There was a problem hiding this comment.
The request model is passed through injectApplicationInfo() before it is serialized. This ensures supported request bodies contain applicationInfo.adyenLibrary without adding the same logic manually to every generated operation.
| */ | ||
| public function setEnvironment(string $environment): self | ||
| { | ||
| if (!in_array($environment, [Environment::TEST, Environment::LIVE], true)) { |
There was a problem hiding this comment.
The initial v7 changes accepted any environment string. This PR changes to only test and live are accepted. This prevents unsupported values.
| token: ${{ secrets.ADYEN_AUTOMATION_BOT_ACCESS_TOKEN }} | ||
| develop-branch: main | ||
| version-files: src/Adyen/Client.php README.md | ||
| version-files: src/Adyen/Client.php src/Adyen/Configuration.php README.md |
There was a problem hiding this comment.
The release workflow now updates both Client.php and Configuration.php when creating a new release, until we removed Client.php completely
| /** | ||
| * @covers \Adyen\BaseService::injectApplicationInfo | ||
| */ | ||
| public function testInjectApplicationInfo() |
There was a problem hiding this comment.
Added test cases for injecting ApplicationInfo
| * | ||
| * @var array{name: string, version: string}|null | ||
| */ | ||
| protected ?array $adyenPaymentSource = null; |
There was a problem hiding this comment.
Earlier , adyenPaymentSource, externalPlatform, and merchantApplication were stored in the client configuration and added to applicationInfo, added them in Configuration
| @@ -303,7 +305,7 @@ class {{classname}} {{#parentSchema}}extends {{{parent}}}{{/parentSchema}}{{^par | |||
| } else { | |||
| $nullablesSetToNull = $this->getOpenAPINullablesSetToNull(); | |||
| $index = array_search('{{name}}', $nullablesSetToNull); | |||
| if ($index !== FALSE) { | |||
| if ($index !== false) { | |||
There was a problem hiding this comment.
Changes FALSE to lowercase false to follow the project PHP coding standard. This is formatting only and does not change the behavior.
|
Previously, most Checkout operations were tested in one large Kept model-specific behavior in The common setup is kept in Where it makes sense for an operation, test three common ways of using the generated models: OpenAPI Generator 7 also creates several versions of each API operation. Testing every version for every endpoint would repeat the same generated template logic, so used representative operations: The async error tests are present but currently skipped because error handling is being addressed separately in a different ticket. |
8f8e89d to
8b61c56
Compare
82a3216 to
8172c38
Compare
|



Checkout openapi generator upgrade