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

Add support for stripe keys in config/env by location and tests for #28 #47

Merged
merged 4 commits into from
Mar 12, 2021

Conversation

pdbreen
Copy link
Contributor

@pdbreen pdbreen commented Mar 11, 2021

Laravel Cashier Stripe usage is confirmed - #28. Made it a bit more robust and added tests.

Comment on lines +52 to +60
public function getStripePublishableAttribute($value)
{
return $value ?? $this->getStripeConfigKey('publishable');
}

public function getStripeSecretAttribute($value)
{
return $value ?? $this->getStripeConfigKey('secret');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These attribute getters (and the getStripeConfigKey() helper) is the key logic. If a DB value is set for either of these attributes, its used. Otherwise, it goes to the config. Within the config, it searches for a location specific entry (keyed by location abbreviation). If not configured, then a default key within the config is used. If that's not configured, then there are no stripe keys available and null is returned.

Copy link
Member

Choose a reason for hiding this comment

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

This is an excellent solution!

@pdbreen
Copy link
Contributor Author

pdbreen commented Mar 11, 2021

The fields could be removed from the DB entirely and this would all still work. So, I'll leave that decision to you.

*
* If no prefix is provided, STRIPE_PUBLISHABLE_KEY, STRIPE_SECRET_KEY env values are used
*/
'default' => LocationPaymentSetting::stripeEnvKeyPair(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also - I'm not sure if using a function to generate the key pairs makes this easier or possibly harder to understand. So, happy to pull it for being more explicit if you want.

Copy link
Member

Choose a reason for hiding this comment

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

I like that.

@pdbreen pdbreen changed the title Add support for stripe keys in config/env by location Add support for stripe keys in config/env by location and tests for #28 Mar 11, 2021
@drewroberts
Copy link
Member

The fields could be removed from the DB entirely and this would all still work. So, I'll leave that decision to you.

@pdbreen Let's go ahead and remove them from the database and I don't think we need that table anymore.

Copy link
Member

@drewroberts drewroberts left a comment

Choose a reason for hiding this comment

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

Thank you! 🐲

@drewroberts drewroberts merged commit a18b3e3 into main Mar 12, 2021
@drewroberts drewroberts deleted the pdbreen/feature/stripe-in-config branch March 12, 2021 07:05
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.

2 participants