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

changes to enable mollie support #16

Closed
wants to merge 1 commit into from

Conversation

magnolia61
Copy link
Contributor

With these changes Mollie is finally working for us.
Reintroduced: getReturnSuccessUrl /
Included: require_once 'CRM/Core/Session.php';
And further some changes to the Mollie library itself.

@eileenmcnaughton
Copy link
Owner

I'm thrilled this is working for you!

In terms of merging the PR:

  1. The changes within the Mollie package need to be done via composer. I see you have been communicating to get them merged upstream. I'm happy to pull in from your fork of the Mollie package rather than the master one while that is resolved but you need to submit a patch to composer.json to do that. (generally people do the composer.json patch PRs & I run composer & commit the results).

The composer.json patch includes where your git repo is ie.

https://github.com/eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor/blob/master/composer.json#L24

and this line would be changed to dev-master instead of 3.0 where your branch is named master (I think)

https://github.com/eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor/blob/master/composer.json#L36

You should put that & the processor.mgd file in one PR.

I'm less confortable with the other changes as they affect other processors. I don't understand why the require_once would be needed - if feels like a symptom of another problem - although harmless in itself.

Changing the return url would affect other processors & remove the '2 chances' that they currently have. On the flip side I think Mollie still obviously requires it for some reason.

I think you are at the point where it is finally working for you & that is great and I actually think that leaving this PR open for now is a valid option. We haven't ironed out everything that would allow me to see it as entirely done & dusted - but with this PR here there is definitely enough information for people to get it going and the next person might be able to resolve those last issues.

@eileenmcnaughton
Copy link
Owner

I'm going to close this because it's stale, but also because I think Mollie might work without extra changes now. I discovered a composer issue causing some Mollie updates to not be in the code base & so the underlying issue is either fixed or moved now...

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

Successfully merging this pull request may close these issues.

None yet

2 participants