-
Notifications
You must be signed in to change notification settings - Fork 52
Use default value when DB_NAME constant is not defined
#203
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
DB_NAME constant is not defined
| * the default value of the constant in "wp-config-sample.php" and in Studio. | ||
| */ | ||
| if ( ! defined( 'DB_NAME' ) ) { | ||
| define( 'DB_NAME', 'database_name_here' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log a warning maybe? or would that not make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamziel At the moment, this seems to be a valid use case in Studio that probably shouldn't even generate warnings, I guess.
That said, I think I figured out a way to make the information schema DB_NAME independent, so I think this will be only temporary. The idea is to use temporary views on top of the stored information schema tables. They are naturally per-session and enable injecting any DB_NAME to the affected columns, they are low cost to create (they don't materialize + it can be lazy), and they could also unlock features like row count and auto-increment values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is not studio-specific, though, but distributed to all sites where the SQLite plugin is installed. Would that make a warning reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamziel the issue is not Studio-specific. It could happen on WoA sites, too, if user uses WordPress with SQLite plugin. WoA sites don't have database constants defined in wp-config.php file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I guess it makes sense for any sqlite-only site to skip the database constants.
This PR replaces #262. It is a simpler solution to the same problem: When a table reference targets an information schema table, we replace it with a subquery, injecting the configured database name dynamically: ```sql -- The following query: SELECT *, t.*, t.table_schema FROM information_schema.tables t -- Will be translated to: SELECT *, t.*, t.table_schema FROM ( SELECT `TABLE_CATALOG`, IIF(`TABLE_SCHEMA` = 'information_schema', `TABLE_SCHEMA`, 'database_name') AS `TABLE_SCHEMA`, `TABLE_NAME`, ... FROM _wp_sqlite_mysql_information_schema_tables AS tables ) t ``` The same logic will be applied to table references in JOIN clauses as well. --- With the new SQLite driver, we keep running into issues with missing and incorrect database name values (#197, #203, #226, #260, #261, and other issues). Thinking through this further, I came to the conclusion that to provide maximum flexibility and portability, it would be best to provide an API in the shape of: _"Mount `my-sqlite-file.sqlite` as `my-db-name`."_ Even if we consider adding multi-database support one day, it would still be great to allow mounting additional SQLite files under dynamic database names. I don't see any downsides to doing this, except maybe that we'll have to choose some database name in some scenarios, such as in database admin tools. However, in these cases we can use a reasonable default or the file name. This is a WIP pull request demonstrating the simplest approach. For it to be merged, I only need to resolve how the existing database name values should be treated.
This is needed for integration of the new SQLite driver in Studio.
We discovered that Studio doesn't use the default implemented in Playground, as it's using
wp-nowand booting Playground differently.As a hotfix, when there is no
DB_NAMEdefined at all, we'll fall back to the same value that Studio is using through an MU plugin with polyfills.A proper fix will be to make the SQLite driver independent of the
DB_NAMEconstant.