Skip to content
This repository was archived by the owner on Jun 2, 2025. It is now read-only.

Add a hotfix for missing DB_NAME constant #44

Closed
wants to merge 1 commit into from
Closed

Add a hotfix for missing DB_NAME constant #44

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions wp-includes/sqlite/db.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,20 @@
require_once __DIR__ . '/class-wp-sqlite-db.php';
require_once __DIR__ . '/install-functions.php';

/*
* Fallback to a default database name if "DB_NAME" is not defined.
*
* This can happen when importing a WordPress backup that doesn't include the
* "DB_NAME" constant definition.
*
* TODO: Remove this once addressed in WordPress Playground, which is a more
* appropriate place to fix this. In the SQLite integration plugin, it is
* better to require the "DB_NAME" constant to be defined to avoid issues
* such as storing an incorrect database name in the information schema.
Comment on lines +59 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just make this change in Playground in that case and skip adding temporary solutions to the SQLite plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bgrgicak I'm not sure what that will involve. Do you think it's a small and quick fix? I'm not yet familiar enough with the Playground codebase to have an idea of what needs to be done — do we do something similar in Playground already? Will it reuse a Blueprint step logic? Etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be quick to do. We can update the bootWordPress function and define the constant if it's missing, similarly to how we add the WP_HOME constant.

Or we could add it to preloadSqliteIntegration, where we define SQLITE_MAIN_FILE.

Copy link
Contributor

@adamziel adamziel Apr 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed with Bero. It would be something like this in boot.ts:

	php.defineConstant('WP_HOME', options.siteUrl);
	php.defineConstant('WP_SITEURL', options.siteUrl);

	// Run "before database" hooks to mount/copy more files in
	if (options.hooks?.beforeDatabaseSetup) {
		await options.hooks.beforeDatabaseSetup(php);
	}

	// @TODO Assert WordPress core files are in place

	if (options.sqliteIntegrationPluginZip) {
+		if ( ! isConstantDefined('DB_NAME') ) {
+			php.defineConstant('DB_NAME', 'wordpress');
+		}
		await preloadSqliteIntegration(
			php,
			await options.sqliteIntegrationPluginZip
		);
	}

We don't have the isConstantDefined implementation. I don't think we can boot WP at this point yet and just call defined( "DB_NAME" ) so we might need to statically analyze wp-config.php. Here's one way to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, we already have some static analysis PHP code in the Playground repo, we might need to refactor it to also have a separate function just for checking the constant's presence:

https://github.com/Automattic/wordpress-playground-private/blob/53d691faa17193c49a40aa2a747f032c505d34f2/packages/playground/blueprints/src/lib/steps/rewrite-wp-config-to-define-constants.php#L70

Copy link
Contributor

@adamziel adamziel May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@JanJakes we would need to check which constants are already defined find and only add the DB_NAME constant if it's not already a part of wp-config.php. rewrite_wp_config_to_define_constants has all the relevant logic in it, it just doesn't do the thing we need here. It seems like we need to split out get_constants_defined_in_wp_config( $code ) or is_constant_defined_in_wp_config( $code, $constant ).

An important aspect to consider is what will happen when we backup/export the site. Which solution would include the DB_NAME correctly set?

The only way to include the DB_NAME with the site export seems to be updating the wp-config.php.

Copy link
Contributor Author

@JanJakes JanJakes May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@adamziel What if the wp-config.php already contains some conditional for defining the constant:

if ( ! getenv( ... ) ) {
    define( DB_NAME, ... );
}

Would is_constant_defined_in_wp_config return true or false, and how would we rewrite it? I guess we could assume the likely 99% scenario, and just say "if there's any define( DB_NAME, ... ) anywhere in the file, we don't inject it, otherwise we do".

Copy link
Contributor

@adamziel adamziel May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

At first, I was tempted to say we could cover those cases with if(!defined('DB_NAME', ...)) before require ABSPATH . "wp-settings.php";. I wonder if that's true, though. If a site only defines a DB_NAME conditionally, they seem to have a very specific intention for what they want to do, e.g. run multiple WordPress instances from a single directory. Adding another definition would likely collide with that intent. I'd say, in those cases, we should just bale, throw a meaningful error, and let them handle it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamziel I agree. To summarize, I think we should only inject the default DB_NAME value if there is no define( 'DB_NAME', ... ) at all in the config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's continue the discussion in https://github.com/Automattic/wordpress-playground-private/pull/140.

*/
// phpcs:ignore WordPress.WP.CapitalPDangit.MisspelledInText
$db_name = defined( 'DB_NAME' ) ? DB_NAME : 'wordpress';

/*
* Debug: Cross-check with MySQL.
* This is for debugging purpose only and requires files
Expand All @@ -59,7 +73,9 @@
$crosscheck_tests_file_path = dirname( __DIR__, 2 ) . '/tests/class-wp-sqlite-crosscheck-db.php';
if ( defined( 'SQLITE_DEBUG_CROSSCHECK' ) && SQLITE_DEBUG_CROSSCHECK && file_exists( $crosscheck_tests_file_path ) ) {
require_once $crosscheck_tests_file_path;
$GLOBALS['wpdb'] = new WP_SQLite_Crosscheck_DB( DB_NAME );
$GLOBALS['wpdb'] = new WP_SQLite_Crosscheck_DB( $db_name );
} else {
$GLOBALS['wpdb'] = new WP_SQLite_DB( DB_NAME );
$GLOBALS['wpdb'] = new WP_SQLite_DB( $db_name );
}

unset( $db_name );