Skip to content

Move first query builder instantiation out of init and into Model.boot() #623

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

Conversation

stoutput
Copy link
Contributor

This allows for logic affecting the query builder (such as db connection resource injection) to be injected by observers' "booting" events

This allows for logic affecting the query builder (such as db connection
resource injection) to be injected by observer's "booting" event
@josephmancuso
Copy link
Member

hmm interesting. Tests seem to pass 👍

@josephmancuso
Copy link
Member

was there any additional manual testing you did to prove this works?

@stoutput
Copy link
Contributor Author

The docs say that the tests were written without need for a db (https://github.com/MasoniteFramework/orm/blob/2.0/CONTRIBUTING.md#running-tests), but in my experience, it wasn't working without configuring databases. There's probably some step I was missing. I have another branch I'm working on trying to get local tests passing with docker containers: https://github.com/stoutput/orm/tree/fix-local-tests. My branch was working a week or so ago but I made some changes along the way that broke it. It's almost there, and would be nice to not have to rely on a local MySQL instance.

@stoutput
Copy link
Contributor Author

I would have liked to add a test case asserting that the DB config wasn't accessed until after the "booting" event had fired, but otherwise, I wouldn't expect this to break any existing tests. We're lazily instantiating it the first time we try to access it, during the boot method, rather than eagerly in __init__.

@josephmancuso
Copy link
Member

Agreed. I just wanna take a deeper look at this tonight and make sure theres no hidden assumptions in the architecture that would break this.

@josephmancuso josephmancuso merged commit 743c885 into MasoniteFramework:2.0 Apr 12, 2022
@stoutput stoutput deleted the instantiate-builder-after-booting-event-is-fired branch April 12, 2022 15:20
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