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

Remove possibility to add Stripe as a payment method when package is missing #14805

Conversation

jakubtobiasz
Copy link
Contributor

@jakubtobiasz jakubtobiasz commented Feb 10, 2023

Q A
Branch? 1.12
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets closes #10655
License MIT

I've added a compiler pass to PayumBundle to remove Stripe's Type when stripe/stripe-php package is not present. I had no idea how to test it, and I've noticed the other passes are not tested either. The logic is simple, so I decided it is not such a big deal to left it as is.

@jakubtobiasz jakubtobiasz added the Bug Confirmed bugs or bugfixes. label Feb 10, 2023
@jakubtobiasz jakubtobiasz requested a review from a team as a code owner February 10, 2023 15:30
@jakubtobiasz jakubtobiasz changed the title Remove possibility to add Stripe as a payment method when package is … Remove possibility to add Stripe as a payment method when package is missing Feb 10, 2023
@jakubtobiasz jakubtobiasz force-pushed the SYL-2677-stripe-available-without-strip-installation branch 2 times, most recently from 28ef3ad to 1b10554 Compare February 14, 2023 07:28
@jakubtobiasz jakubtobiasz force-pushed the SYL-2677-stripe-available-without-strip-installation branch from 1b10554 to df124bc Compare February 14, 2023 07:36
@GSadee GSadee merged commit 8600fcf into Sylius:1.12 Feb 14, 2023
@GSadee
Copy link
Member

GSadee commented Feb 14, 2023

Thank you, Jakub! 🥇

@Prometee
Copy link
Contributor

@jakubtobiasz I'm afraid this compiler pass is not checking the right thing. It did not work because the if statement is testing for the existence of a stripe/stripe-php class instead of the payum/stripe Stripe Checkout's Payum gateway factory.

The reason for that, is because the Stripe Checkout's Payum gateway factory is inside payum/stripe which was previously required by payum/payum. When payum/payum was replaced by payum/core the Stripe Checkout's Payum gateway factory was removed in the same time, letting the php library stripe/stripe-php alone in the dark.

What can be done is to replace the dev requirement of stripe/stripe-php by payum/stripe, same for sylius/sylius-standard and finally fix the UnregisterStripeGatewayTypePass by replacing the if statement with this :

        if (class_exists(StripeCheckoutGatewayFactory::class)) {
            return;
        }

@jakubtobiasz jakubtobiasz deleted the SYL-2677-stripe-available-without-strip-installation branch October 31, 2023 19:36
NoResponseMate added a commit that referenced this pull request Nov 9, 2023
…-checkout-nvp` (Prometee)

This PR was merged into the 1.13 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.12                  |
| Bug fix?        | yes                                                       |
| New feature?    | no                                                       |
| BC breaks?      | no                                                       |
| Deprecations?   | no |
| Related tickets | none                      |
| License         | MIT                                                          |

See this comment for more explanations : #14805 (comment)

This PR also add a way to not use `payum/paypal-express-checkout-nvp` and suggestion to install stripe or paypal packages

Commits
-------
  Allow to not use PayPal Express NVP
  Check the Payum factory instead of the Stripe library class
  Add Paypal and Stripe suggestions to the SyliusPayumBundle
  Add return type
  Re-add payum/stripe and remove stripe lib from require-dev
  Move payum gateways to require-dev
  Add PAYPAL convert action service ID to the removed services
  Clean requirements
  Fix coding standard
  Update composer-require-checker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bugs or bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants