Backport the SQLite rewrite from the standalone plugin #677

aristath · 2023-03-16T07:30:45Z

Summary

This PR backports the SQLite rewrite from the standalone plugin.
The rewrite of the implementation uses a Lexer and a Translator for the queries, and offers significantly better security as well as compatibility with a wider array of MySQL queries.

Relevant technical choices

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

modules/database/sqlite/db.copy.php

felixarntz

@aristath I didn't review all the actual code, since it's a whole lot and it's already part of the SQLite standalone plugin, so probably not needed anyway.

One small feedback below for integrating this into the main repo, since right now we still have the modules around (which will change in the future of course).

modules/database/sqlite/wp-includes/sqlite/class-wp-sqlite-db.php

Co-authored-by: Jonny Harris <[email protected]>

aristath · 2023-04-10T07:47:00Z

I faced a lot of issues with the activation/deactivation routines... A plugin doesn't work the same as a module, and plugins need to use different hooks to activate/deactivate things etc.
As a result, in order to keep things simple I reverted all the changes regarding the plugin activation/deactivation and only backported the actual database-engine changes from the standalone plugin.
Once we have a better idea on how standalone plugins will be added & deployed from this repo I can do a followup PR with the plugin-specific files.

mukeshpanchal27

Thanks @aristath, for the update. I quickly review the PR and found some nitpicks; other than that, I don't have any more feedback. We need more eyes on this so we can merge.

modules/database/sqlite/can-load.php

mukeshpanchal27 · 2023-04-11T05:03:54Z

modules/database/sqlite/wp-includes/sqlite/class-wp-sqlite-db.php

@@ -133,11 +158,10 @@ public function print_error( $str = '' ) {
 		wp_load_translations_early();

 		$caller = $this->get_caller();
-		$caller = $caller ? $caller : __( '(unknown)', 'performance-lab' );
+		$caller = $caller ? $caller : '(unknown)';


Translation is missing on this file. Check other instances for the same.

This was on purpose.
These are not user-facing messages and translating them is not necessary. These strings are intended for error-logging only.
If there is an error in the database, in some scenarios it happens before the translations functions have even loaded. Using the __() function here was throwing a fatal error and prevented the actual DB error from getting logged.
Translations are only required for user-facing strings, so we removed them from strings that are intended for error-logging and are never shown to users.

modules/database/sqlite/constants.php

Co-authored-by: Mukesh Panchal <[email protected]>

felixarntz

@aristath This looks almost good to go, just a few points of feedback for consideration.

felixarntz · 2023-04-19T18:10:09Z

modules/database/sqlite/constants.php

-	if ( defined( 'PERFLAB_SQLITE_DB_DROPIN_VERSION' ) ) {
-		define( 'DATABASE_TYPE', 'sqlite' );
+if ( ! defined( 'DB_ENGINE' ) ) {
+	if ( defined( 'SQLITE_DB_DROPIN_VERSION' ) ) {


Based on the code above in line 10-12, we would need to use PERFLAB_SQLITE_DB_DROPIN_VERSION instead, as above you set that constant based on the other one. Which one is considered the source of truth? Given the standalone plugin uses SQLITE_DB_DROPIN_VERSION, we should probably use that.

felixarntz · 2023-04-19T18:11:27Z

modules/database/sqlite/constants.php

+// Backwards-compatibility.
+if ( ! defined( 'PERFLAB_SQLITE_DB_DROPIN_VERSION' ) && defined( 'SQLITE_DB_DROPIN_VERSION' ) ) {
+	define( 'PERFLAB_SQLITE_DB_DROPIN_VERSION', SQLITE_DB_DROPIN_VERSION );
+}


See my other comment, we also need to ensure backward compatibility the other direction, especially as I'm assuming SQLITE_DB_DROPIN_VERSION will be our main constant going forward.

Suggested change

}

}

if ( ! defined( 'SQLITE_DB_DROPIN_VERSION' ) && defined( 'PERFLAB_SQLITE_DB_DROPIN_VERSION' ) ) {

define( 'SQLITE_DB_DROPIN_VERSION', PERFLAB_SQLITE_DB_DROPIN_VERSION );

}

felixarntz · 2023-04-19T18:18:51Z

modules/database/sqlite/site-health.php

 */
-function perflab_sqlite_plugin_filter_debug_data( $info ) {
-	$database_type = defined( 'DATABASE_TYPE' ) && 'sqlite' === DATABASE_TYPE ? 'sqlite' : 'mysql';
+function sqlite_plugin_filter_debug_data( $info ) {


Why remove the doc block here? Not having a doc block is worse than having one :)

Additionally, we should be careful about prefixing our code properly. sqlite_plugin_ works well as a prefix, since that makes it very clear it's for the plugin and won't conflict when that code is eventually part of WordPress core. But we shouldn't use just sqlite_ or wp_sqlite_, because those are at risk for conflicts.

felixarntz · 2023-04-19T18:20:24Z

modules/database/sqlite/wp-includes/sqlite/class-wp-sqlite-db.php

@@ -11,14 +11,14 @@
 *
 * It also rewrites some methods that use mysql specific functions.
 */
-class Perflab_SQLite_DB extends wpdb {
+class WP_SQLite_DB extends wpdb {


See my above comment, I don't think it's a good idea to use the WP_SQLite_ prefix. This code is not part of core (yet), so we shouldn't use a prefix that is preferably reserved for WordPress core.

How about SQLite_Plugin_DB or SQLite_Database_Integration_DB or something else that clearly scopes this code to the SQLite plugin or module?

felixarntz · 2023-04-19T18:20:58Z

modules/database/sqlite/wp-includes/sqlite/class-wp-sqlite-query-rewriter.php

+/**
+ * The query rewriter class.
+ */
+class WP_SQLite_Query_Rewriter {


Same comment above applies here and elsewhere on the other classes introduced here. Shouldn't we use a safer plugin-specific prefix for our code?

aristath · 2023-04-25T16:00:46Z

Closing this one, see #661 (comment)

aristath added [Type] Bug An existing feature is broken no milestone PRs that do not have a defined milestone for release [Focus] Database labels Mar 16, 2023

aristath requested a review from OllieJones as a code owner March 16, 2023 07:30

aristath mentioned this pull request Mar 16, 2023

SQLite: Error in prepare_query with DELETE / delete_expired_transients #650

Closed

aristath requested a review from felixarntz March 16, 2023 07:36

spacedmonkey reviewed Mar 20, 2023

View reviewed changes

modules/database/sqlite/db.copy.php Outdated Show resolved Hide resolved

felixarntz reviewed Mar 30, 2023

View reviewed changes

modules/database/sqlite/wp-includes/sqlite/class-wp-sqlite-db.php Outdated Show resolved Hide resolved

felixarntz removed the no milestone PRs that do not have a defined milestone for release label Apr 3, 2023

felixarntz added this to the 2.2.0 milestone Apr 3, 2023

aristath force-pushed the sqlite-rewrite-backport branch from adeccf5 to c8acf14 Compare April 5, 2023 07:55

mukeshpanchal27 mentioned this pull request Apr 6, 2023

Migrate SQLite integration plugin back into monorepo for development #661

Closed

aristath and others added 8 commits April 10, 2023 09:08

Backport the SQLite rewrite

590a89a

Update modules/database/sqlite/db.copy.php

93ae4dc

Co-authored-by: Jonny Harris <[email protected]>

backport more changes

64df26d

Update "package" file docs

1bc9ee0

Update since docs

26f72ab

fix more since docs

073ef34

add back module headers

8ad6a79

Add back the can-load file

4652949

aristath force-pushed the sqlite-rewrite-backport branch from b4c1f6f to 4652949 Compare April 10, 2023 06:08

aristath added 2 commits April 10, 2023 09:24

add fallbacks for constants

68ec7a1

more backward-compatibility fixes

5cfa86d

aristath requested a review from JustinyAhin as a code owner April 10, 2023 06:27

aristath added 6 commits April 10, 2023 09:34

Plugin is no longer based on Kojima Toshiyasu's implementation

8948766

remove more file headers which no longer apply

6489f32

Fix activation/deactivation

ac0dad1

Revert plugin changes

d7e7538

rename file to what it used to be

35abe35

remove debug-crosscheck

560f4df

mukeshpanchal27 reviewed Apr 11, 2023

View reviewed changes

mukeshpanchal27 linked an issue Apr 11, 2023 that may be closed by this pull request

SQLite: Error in prepare_query with DELETE / delete_expired_transients #650

Closed

mukeshpanchal27 modified the milestones: 2.2.0, 2.3.0 Apr 11, 2023

aristath and others added 3 commits April 11, 2023 08:44

Update modules/database/sqlite/constants.php

710eddf

Co-authored-by: Mukesh Panchal <[email protected]>

Update modules/database/sqlite/can-load.php

8dc116f

Co-authored-by: Mukesh Panchal <[email protected]>

Merge branch 'trunk' into sqlite-rewrite-backport

5e6d474

felixarntz reviewed Apr 19, 2023

View reviewed changes

aristath closed this Apr 25, 2023

felixarntz removed this from the 2.3.0 milestone Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport the SQLite rewrite from the standalone plugin #677

Backport the SQLite rewrite from the standalone plugin #677

aristath commented Mar 16, 2023 •

edited by felixarntz

Loading

felixarntz left a comment

aristath commented Apr 10, 2023

mukeshpanchal27 left a comment

mukeshpanchal27 Apr 11, 2023

aristath Apr 11, 2023

felixarntz left a comment

felixarntz Apr 19, 2023

felixarntz Apr 19, 2023

felixarntz Apr 19, 2023

felixarntz Apr 19, 2023

felixarntz Apr 19, 2023

aristath commented Apr 25, 2023

-}
+}
+if ( ! defined( 'SQLITE_DB_DROPIN_VERSION' ) && defined( 'PERFLAB_SQLITE_DB_DROPIN_VERSION' ) ) {
+	define( 'SQLITE_DB_DROPIN_VERSION', PERFLAB_SQLITE_DB_DROPIN_VERSION );
+}

Backport the SQLite rewrite from the standalone plugin #677

Backport the SQLite rewrite from the standalone plugin #677

Conversation

aristath commented Mar 16, 2023 • edited by felixarntz Loading

Summary

Relevant technical choices

Checklist

felixarntz left a comment

Choose a reason for hiding this comment

aristath commented Apr 10, 2023

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

mukeshpanchal27 Apr 11, 2023

Choose a reason for hiding this comment

aristath Apr 11, 2023

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz Apr 19, 2023

Choose a reason for hiding this comment

felixarntz Apr 19, 2023

Choose a reason for hiding this comment

felixarntz Apr 19, 2023

Choose a reason for hiding this comment

felixarntz Apr 19, 2023

Choose a reason for hiding this comment

felixarntz Apr 19, 2023

Choose a reason for hiding this comment

aristath commented Apr 25, 2023

aristath commented Mar 16, 2023 •

edited by felixarntz

Loading