Skip to content
This repository has been archived by the owner on Feb 2, 2022. It is now read-only.

Payment method as array #210

Merged

Conversation

chris-redbeed
Copy link

I replace the comma string with an array.
The comma string don't work and the Class only allowed stings.

With this changes it is possible to set up multiple method correctly.

@sandervanhooft
Copy link
Contributor

A lot of people will be using the current default config for this, so we'll need to provide something that's backwards compatible.

When processing the method from the config, how about:

If null: convert to empty array []
If string: remove spaces, explode(',')

Then process the array.

Copy link
Contributor

@sandervanhooft sandervanhooft left a comment

Choose a reason for hiding this comment

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

Almost there :), see my review comments.

Also, don't forget to add a unit test for the trait.

Please test these cases:

$method = ' , ideal    , bancontact '; // results in ['ideal', 'bancontact']
$method = null; // results in []

src/Traits/PaymentMethodString.php Outdated Show resolved Hide resolved
src/FirstPayment/FirstPaymentBuilder.php Outdated Show resolved Hide resolved
src/Plan/Plan.php Outdated Show resolved Hide resolved
Copy link
Contributor

@mmachatschek mmachatschek left a comment

Choose a reason for hiding this comment

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

Please see my comments in #203. The Billable trait needs to be updated too, in order to correctly work with the updated firstPaymentMethod return value in this PR, which now is a array instead of a string

I'm specifically pointing to line 78 in Billable::class

@chris-redbeed
Copy link
Author

@sandervanhooft FYI: Currently I didn't run the unit tests because I need to set up a test env. with the mandatory IDs (Payment IDs, Customer IDs) for the test. I will let you know after I run the tests.

@sandervanhooft
Copy link
Contributor

Please see my comments in #203. The Billable trait needs to be updated too, in order to correctly work with the updated firstPaymentMethod return value in this PR, which now is a array instead of a string

I'm specifically pointing to line 78 in Billable::class

Not sure I get your point. Can you elaborate on this?

@sandervanhooft
Copy link
Contributor

Please see my comments in #203. The Billable trait needs to be updated too, in order to correctly work with the updated firstPaymentMethod return value in this PR, which now is a array instead of a string
I'm specifically pointing to line 78 in Billable::class

Not sure I get your point. Can you elaborate on this?

All cleared up now in #203.

@sandervanhooft
Copy link
Contributor

sandervanhooft commented Jul 15, 2020

  • Currently, the PlanContract states that method should return a string. People are relying on this, so we should keep this compatible.

  • Also, the FirstPaymentBuilder::getMolliePayload is now returning a Collection instance instead of an array.

@sandervanhooft
Copy link
Contributor

Let's treat this as a breaking change (BC) and adopt it in v2.

This also means we can simplify it further by changing everything into array, no need for backwards compatibility.

@sandervanhooft sandervanhooft self-requested a review July 21, 2020 16:18
@salmanhijazi
Copy link

How to bypass this for now to allow multiple methods?

@sandervanhooft sandervanhooft changed the base branch from develop to v2-develop December 11, 2020 11:59
@sandervanhooft sandervanhooft merged commit 4974d41 into laravel:v2-develop Dec 11, 2020
@sandervanhooft
Copy link
Contributor

Merged into v2-develop, thanks @chris-redbeed !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants