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

Make compatible with adyen/module-payment 9.13.0 #93

Merged
merged 3 commits into from
Jan 21, 2025

Conversation

JustinElst
Copy link
Contributor

@JustinElst JustinElst commented Jan 14, 2025

Description:
https://github.com/Adyen/adyen-magento2/pull/2819has made changes to the namespace of HeaderDataBuilder which is used in this module here
This PR implements the HeaderDataBuilderInterface to be compatible with 9.13.0 of Module Payment

Fixes:

@JustinElst JustinElst requested a review from a team as a code owner January 14, 2025 12:31
@JustinElst JustinElst changed the title Make compatible 9 13 0 Make compatible with adyen/module-payment 9.13.0 Jan 14, 2025
@khushboo-singhvi khushboo-singhvi linked an issue Jan 15, 2025 that may be closed by this pull request
@khushboo-singhvi
Copy link
Contributor

Hey @JustinElst, Thank you for the PR as contribution. It is much appreciated. Can you please also check the related unit test which makes the pipeline fail. After that I think we can merge the PR.

@JustinElst
Copy link
Contributor Author

JustinElst commented Jan 15, 2025

Hi @khushboo-singhvi,

It seems that your code-review environment is checking out the 'develop' branch and tests that.
Look at this line... and it installs adyen/module-payment 9.13.0 here.

The develop branch will fail the tests because its not compatible with adyen/module-payment 9.13.0, hence this pull request 🎉

I created a pull request to possibly fix the checkout ref, check it out and re-run the tests for this pull-request: #95

@JustinElst
Copy link
Contributor Author

JustinElst commented Jan 15, 2025

There should go some thought into module upgrade path for this module, because 1.2.2 is not compatible with adyen/module-payment 9.13.0 but composer update will just install 9.13.0 for everyone. And after this merge this module will not be compatible with any adyen/module-payment < 9.13.0.
I did change the require within composer.json (in this pull request) but i'm not sure if people will run into trouble this way 😉

I'm thinking, should this pull-request mark a breaking change and a 1.3.0 or 2.0.0 release?
And in a separate release (1.2.3) of this module add a constraint to composer.json like:

    "adyen/module-payment": ">=9.6.1 <9.13.0",

Or, will this be handled by just releasing this as 1.3.0?

⚠️ Needs some speed?
People using this module and running a composer update will just install adyen/module-payment 9.13.0 and it works like a charm, until trying to checkout with an Adyen payment method (silently fails in some cases?), so this could very quickly lead to missed income. (if the merchant has not tested the checkout flow)

@khushboo-singhvi
Copy link
Contributor

Hey @JustinElst,

Thank You for all the suggestions and the contributions. Yes, we are aware of this concern, and we truly appreciate you bringing it up. To ensure a smooth upgrade path, we are planning to address this by releasing a new version shortly. Here's the approach we're considering:

Versioning for Compatibility:

  • We'll mark this as a breaking change and release it as 1.3.0 to signal the shift in compatibility with adyen/module-payment >= 9.13.0.

Mitigating Risks:

  • By implementing this versioning strategy, merchants using the module will avoid silent failures due to inadvertent upgrades. We'll also emphasize the importance of testing the checkout flow after updates.

Speedy Release:

  • We understand the urgency of this situation and are prioritizing the release to minimize the impact on merchants and their operations, most probably tomorrow is the day.

Thanks again for highlighting this! Keep an eye out for the release updates.

Regards!
Khushboo

@khushboo-singhvi khushboo-singhvi merged commit 0d29538 into Adyen:develop Jan 21, 2025
2 of 4 checks passed
@candemiralp candemiralp mentioned this pull request Jan 21, 2025
@candemiralp candemiralp added the Fix Indicates a bug fix label Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Indicates a bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not compatible with v9.13.0 of Adyen/adyen-magento2
3 participants