Skip to content

NEW Add method to check if a class is ready for db queries#11784

Merged
emteknetnz merged 1 commit intosilverstripe:6from
creative-commoners:pulls/6/reusable-db-check
Jul 20, 2025
Merged

NEW Add method to check if a class is ready for db queries#11784
emteknetnz merged 1 commit intosilverstripe:6from
creative-commoners:pulls/6/reusable-db-check

Conversation

@GuySartorelli
Copy link
Copy Markdown
Member

@GuySartorelli GuySartorelli commented Jul 16, 2025

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

Replaces #10276

I've resurrected this old PR because I was about to reimplement "is the DB ready?" checks yet again in #11783.... I'd much rather just reuse existing logic instead.

Parent issue

Comment thread tests/php/ORM/DataObjectSchemaTest.php
@beerbohmdo
Copy link
Copy Markdown
Contributor

Maybe this should be a new issue but it is somewhat related: For me the existing Security::database_is_ready sometimes failes with an error.

I don't know why but the $schema->tableName($class) returns null in some cases (presumably a race condition with the cache) and ClassInfo::hasTable($table) don't check for that, what results into an error.

@GuySartorelli
Copy link
Copy Markdown
Member Author

GuySartorelli commented Jul 16, 2025

@beerbohmdo Please open a new issue about that bug - a patch for that likely needs to target CMS 5 first, while this new method is for CMS 6. I'll also want some more details about how to reproduce it etc - all of which is best captured in an issue.

Copy link
Copy Markdown
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Feels like this method should not be on DataObjectSchema, which "Provides dataobject and database schema mapping functionality"

Seems like this method should be on DB instead which has "Use this class for interacting with the database."

Comment thread src/ORM/DataObjectSchema.php Outdated
Comment thread src/ORM/DataObjectSchema.php
@GuySartorelli
Copy link
Copy Markdown
Member Author

GuySartorelli commented Jul 17, 2025

Feels like this method should not be on DataObjectSchema, which "Provides dataobject and database schema mapping functionality"

Seems like this method should be on DB instead which has "Use this class for interacting with the database."

In the original PR I had it in DB - but that felt wrong because DB doesn't know anything about DataObject, and the logic here is explicitly checking if the database is ready _based on the schema of the DataObject`_.
See #10041 (comment)

In other words, while we are using DB to "interact with the database" as per that class's intention, we are only doing so to check if the database is ready according to "database schema mapping" as per the DataObjectSchema class's intention.

This effectively makes the functionality from
`Security::database_is_ready()` reusable for any arbitrary DataObject
subclass.
@GuySartorelli GuySartorelli force-pushed the pulls/6/reusable-db-check branch from 1da3d13 to cd42486 Compare July 17, 2025 21:26
@emteknetnz
Copy link
Copy Markdown
Member

emteknetnz commented Jul 18, 2025

Seems like DBSchemaManager would be a better place for this?

That already has a private property Database $database - which presumably is the same object (singleton??) that DB::get_conn() uses, which is used in things like DB::is_ready().

DB::get_schema() (used by DB::field_list()) presumably returns the same (singleton?) DBSchemaManger?

@GuySartorelli
Copy link
Copy Markdown
Member Author

DBSchemaManager doesn't know about DataObject schema specifically though.
Note that the implementation that I've currently got calls the following DataObjectSchema methods:

  • tableName()
  • databaseFields()

I think it makes more sense for a higher-level class (DataObjectSchema, which knows about the DataObject) and can call down to lower-level API as needed, rather than having it in a lower-level class (DBSchemaManager which is for abstracting out DB schema management to different SQL server implementations and doesn't know anything about DataObject) call up to higher-level API.

@emteknetnz emteknetnz merged commit 9885b46 into silverstripe:6 Jul 20, 2025
12 checks passed
@emteknetnz emteknetnz deleted the pulls/6/reusable-db-check branch July 20, 2025 22:37
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