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

NEW: Add method to check if a class is ready for db queries. #10276

Open
wants to merge 1 commit into
base: 4
Choose a base branch
from

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Apr 8, 2022

This effectively makes the functionality from Security::database_is_ready() reusable for any arbitrary DataObject
subclass.

Replaces #10041

Parent issue

This effectively makes the functionality from
Security::database_is_ready() reusable for any arbitrary DataObject
subclass.
@maxime-rainville
Copy link
Contributor

While I can see the value in this, we already have a bunch of method doing this in different way: silverstripe/silverstripe-graphql#445 (comment)

I would rather have a some agreement on the best way to do it and make sure our code base is updated to use the correct approach everywhere. silverstripe/silverstripe-graphql#445 (comment)

@GuySartorelli
Copy link
Member Author

Makes sense. How do we go about doing that? I gather that the RFC process is unrefined at best....

@maxime-rainville
Copy link
Contributor

I don't think we need an RFC for this. Maybe create a parent issue and details some of the places where we need this functionality and some of the approaches we've taken in the past. Maybe document clearly why this is needed as well.

Addressing the problem will probably involved implementing the best approach,moving away from using the bad approaches and deprecating a bunch of methods.

@GuySartorelli
Copy link
Member Author

Added a parent issue.

@LiamKearn
Copy link
Contributor

Not useful for technical debt that doesn't call into the database with the ORM to early in a request boot in a project I am not a part of :-)

In all seriousness though +1 for this.

*/
public function tableIsReadyForClass(string $class): bool
{
if (!is_subclass_of($class, DataObject::class)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
if (!is_subclass_of($class, DataObject::class)) {
// Bail if there's no active database connection yet
if (!DB::connection_attempted() || !DB::is_active()) {
return false;
}
if (!is_subclass_of($class, DataObject::class)) {

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