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

Removed Phoenix_Moneybookers #1044

Merged
merged 5 commits into from
Jul 6, 2020
Merged

Removed Phoenix_Moneybookers #1044

merged 5 commits into from
Jul 6, 2020

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Jun 16, 2020

Ref #988

Should we keep it? If required ... https://github.com/simkea/Phoenix_Moneybookers ...

TODO: setup script to delete config entries (seperate PR)

@sreichel sreichel mentioned this pull request Jun 16, 2020
@Flyingmana
Copy link
Contributor

I think Iam missing the reason why this module should be removed.
isnt it part of the core for a very long time now and probably used by many?

Besides, the linked Repository has an outdated version, the marketplace already had newer versions.
https://github.com/OpenMageModuleFostering/phoenix_moneybookers/commits/master

@colinmollenhour
Copy link
Member

It can always be added back as a separate extension if anyone is actually using it but I wouldn't assume that because it exists it is being used, necessarily.. I think a more important consideration is do we want to support it? If it isn't compatible with SAQ A I think we should remove it. OpenMage is "use at your own risk" anyway, but it is still good to strongly discourage use of payment methods that are not SAQ A by not including them in core, IMHO.

@sreichel sreichel marked this pull request as draft June 16, 2020 20:08
@sreichel
Copy link
Contributor Author

Imho it is just shipped with core, but a community extension that has not been updated for years. Is this still maintained - after M1 EOL? There is no public repo to track or report issues, so we have to support it on our own. Do we want this?

Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

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

+1 for removing

@Flyingmana
Copy link
Contributor

I see, as it belongs to a service which is not existing anymore, removing is ok.

@sreichel sreichel marked this pull request as ready for review June 17, 2020 20:55
@sreichel
Copy link
Contributor Author

sreichel commented Jun 26, 2020

+1 for removing

There is a aprrove-button .... ;)

@sreichel sreichel added hold and removed hold labels Jun 26, 2020
Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

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

Ther is a aprrove-button .... ;)

Haha, yes.. I hadn't actually looked at the diff yet, though..

@sreichel sreichel removed this from the Release 19.4.7 milestone Jun 27, 2020
@colinmollenhour colinmollenhour merged commit 7b286ef into OpenMage:1.9.4.x Jul 6, 2020
@sreichel sreichel deleted the cleanup/dead/moneybookers branch July 6, 2020 23:12
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 20, 2020
edannenberg pushed a commit to edannenberg/magento-lts that referenced this pull request Aug 24, 2020
@viable-hartman
Copy link
Contributor

I know a little to this conversation, but wouldn't we also want to delete: skin/frontend/base/default/images/moneybookers and skin/adminhtml/default/default/images/moneybookers when getting rid of this extension?

@sreichel
Copy link
Contributor Author

sreichel commented Sep 9, 2020

I know a little to this conversation, but wouldn't we also want to delete: skin/frontend/base/default/images/moneybookers and skin/adminhtml/default/default/images/moneybookers when getting rid of this extension?

Yep. There is already a PR ... #1161

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

Successfully merging this pull request may close these issues.

4 participants