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

[13.x] Refactor model config option #1100

Merged
merged 1 commit into from
Mar 16, 2021
Merged

[13.x] Refactor model config option #1100

merged 1 commit into from
Mar 16, 2021

Conversation

driesvints
Copy link
Member

This will remove the model config option in favor of setting a static property on the Cashier class. This will make setting the custom model for the billable entity the same as we do it in the rest of Cashier and the same as we do it in all our other first party packages. It also makes the new Models namespace of Laravel 8 the default one.

@taylorotwell taylorotwell merged commit ee7d4c2 into master Mar 16, 2021
@taylorotwell taylorotwell deleted the billable-model branch March 16, 2021 14:03
@garygreen
Copy link

garygreen commented Feb 10, 2023

@driesvints With these changes it means Cashier classes and files needs to be loaded every single request. The benefit of having it part of a config is that the service provider can defer loading of the settings as and when Cashier is needed.

@driesvints
Copy link
Member Author

I'm not sure what you mean with loading classes and files on every single request? Isn't that done for every class from a package through composer autoloading anyway?

@garygreen
Copy link

Isn't that done for every class from a package through composer autoloading anyway?

If you take look at output from get_included_files() you can see all the classes loaded in memory for a request.

image

And because Cashier has reference BaseStripeClient::DEFAULT_API_BASE it then loads Stripe's php-sdk classes:

image

A lot of classes are traits attached to the User model, so unavoidable that they load (unless a $user->cashier() approach was taken)`. But of the 14 classes at least 5 don't need to be loaded into memory each request.

Ideally the CashierServiceProvider should be a deferrable provider and only registers with the IOC. Then as the application needs Cashier it will defer loading of that provider.

With a Cashier:: approach to setting configs, it means the app is forced to load Cashier and subsequent files into memory every request. This is wasteful when it's usually not needed on most requests. Using config based options avoids the need to call Cashier:: to set configs. It's also the reason why there is a config:cache option because that puts all the settings in one file which is less i/o.

@driesvints
Copy link
Member Author

I really don't think that affects you so much.

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.

3 participants