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

SUPESC-282 Fixed handling of response while empty through allowing null value #6

Conversation

michbeck
Copy link
Contributor

@michbeck michbeck commented Oct 6, 2021

Release Table

Module Release Type Constraint Updates
AdyenApi minor

Module AdyenApi

Change log

Fixes

  • Fixed handling of response while empty through allowing null value

@dereuromark
Copy link
Contributor

These changes on transfer level are not BC
is this supposed to be a major?

psalm-report.json Outdated Show resolved Hide resolved
@dereuromark
Copy link
Contributor

You removed the CI part that you initially wanted to fix
If possible would be nice to include that now again in GitHubActions
Or at least some of those, like PHPStan/CS etc

psalm-report.json Outdated Show resolved Hide resolved
@dereuromark dereuromark added bug Something isn't working ready-for-release labels Oct 8, 2021
tooling.yml Outdated Show resolved Hide resolved
composer.json Show resolved Hide resolved
class: PhpStan\DynamicType\ZedFactoryDynamicTypeExtension
tags:
- phpstan.broker.dynamicMethodReturnTypeExtension
- '#.+Generated\\Shared\\Transfer.+.#'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is very common pattern we should avoid those. use more specifics patterns here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are no transfers generated this is fine for now

But I would advise to generate the transfers as per standalone testing guide
This will be better long term, but can also be a follow up PR.

<property name="returnUrl" type="string" />
<property name="amount" type="AdyenApiAmount"/>
<property name="merchantAccount" type="string"/>
<property name="paymentMethod" type="array" singular="paymentMethodItem"/>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • This is a BC break.
  • By default the generated method will have a name addPaymentMethod() and now it will be changed to addPaymentMethodItem, all code that could have been written on the client side will crash.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible solutions are to roll back changes or singular has to be equal to name property in order to warranty the BC promise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see #6 (review)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it looks like one, but apparently the singular is already exactly this by other module
So this seems OK to release now in this verbose state.

Good that we make it more visible now :)

@spryker-release-bot spryker-release-bot merged commit 3e66888 into master Oct 13, 2021
@spryker-release-bot spryker-release-bot deleted the bugfix/supesc-282-issues-in-eco-adyen-integration-guide branch October 13, 2021 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready-for-release
6 participants