Implement new experimental SQLite integration module #547

aristath · 2022-09-29T12:55:35Z

Summary

This PR introduces a module to allow testing the SQLite implementation - as discussed in the performance team's Slack meeting this week.
For background, you can read the SQLite proposal on make.w.org

This is a continuous work in progress, but it is working well enough to be included as an experimental module. The issues are not so much with the SQLite implementation itself, as much as with the module's activation/deactivation and the overall User Experience during that process.

The code for the SQLite implementation was copied from https://github.com/aaemnnosttv/wp-sqlite-db/blob/master/src/db.php, which is a fork of the original work on the sqlite-integration plugin. It was then refactored a bit, coding-standards were applied, and an integration with the plugin was built.

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.

saas786 · 2022-10-10T06:07:26Z

It would be awesome if this implementation also encourages WordPress database to be abstracted out,
so we can could call secondary database at runtime.

It would help with situations like these:

Ref: xwp/stream#1360

Not sure if this comment is clear enough, but hoping it make sense and such level of abstraction can be implemented in core.

felixarntz

@aristath Thank you for this (massive) PR! I did a first pass and left some comments. Most of them are either about the activation/deactivation routine or following some Performance Lab conventions that are currently off here.

I'm certainly open to not addressing everything in this one PR, we could also see if we could break things down if you prefer that. The only thing that we would need to define clearly is what would be our MVP criteria for when this could launch (even as experimental module) in the Performance Lab plugin. But even for that MVP functionality, there is room for multiple PRs, we could for example create a feature branch for the module rather than working directly against trunk. LMKWYT.

modules/sqlite/integration/load.php

admin/load.php

modules/sqlite/integration/load.php

modules/sqlite/integration/wp-includes/sqlite/class-wp-pdo-engine.php

modules/sqlite/integration/wp-includes/sqlite/db.php

felixarntz · 2022-10-24T22:21:43Z

modules/sqlite/integration/load.php

+			),
+			file_get_contents( __DIR__ . '/db.copy' )
+		);
+		$wp_filesystem->put_contents( $destination, $file_contents );


As far as I understand, this is the only logic that runs when activating the module, correct? And then it would mean the user would land on a blank WP install?

Based on previous conversations we had on Slack, I would suggest that we also set up the database right away, so that it is at least a blank WordPress install with the Performance Lab plugin and the same modules enabled. This way it would make for a much better and less confusing user experience.

Would it be possible to call wp_install() here for the new SQLite database (at least if that database is empty)? If it's not possible within the initial request where a regular MySQL / MariaDB database is still used, we could also redirect to a custom endpoint that does this in a new request and then redirects the user back to the Performance Lab settings screen.

While this is initially going to be an experimental module and we're not going to get into the weeds of e.g. migrating a full DB, I still think a smoother user experience for this would be crucial, and the above suggestion may be feasible without too much effort.

As far as I understand, this is the only logic that runs when activating the module, correct? And then it would mean the user would land on a blank WP install?

Correct.

Based on previous conversations we had on Slack, I would suggest that we also set up the database right away, so that it is at least a blank WordPress install with the Performance Lab plugin and the same modules enabled. This way it would make for a much better and less confusing user experience.

If an SQLite database doesn't exist (ie, this is the 1st time the user activates the module), they will land on the WP-install screen so they can enter their admin username/password, sitename etc.
The copied db.php file then takes care of activating the performance-lab plugin, and activate the default modules + SQLite.

Would it be possible to call wp_install() here for the new SQLite database (at least if that database is empty)? If it's not possible within the initial request where a regular MySQL / MariaDB database is still used, we could also redirect to a custom endpoint that does this in a new request and then redirects the user back to the Performance Lab settings screen.

I just pushed a tweak that does that 👍
However, I was unable to achieve it in PHP so it was accomplished using a window.location.reload(true); call. That in turn, triggers WP to redirect to the login screen if a database exists - and then after login the user lands on the perflab settings - OR, if a database doesn't exist, WP redirects the user to the install screen.

While this is initially going to be an experimental module and we're not going to get into the weeds of e.g. migrating a full DB, I still think a smoother user experience for this would be crucial, and the above suggestion may be feasible without too much effort.

💯

@aristath

If an SQLite database doesn't exist (ie, this is the 1st time the user activates the module), they will land on the WP-install screen so they can enter their admin username/password, sitename etc.

Why do we need to require the user to do that though? They already provided that information when they set up the original DB, so I think it would be a much smoother experience if we didn't prompt them to the WP install screen, but rather called wp_install() for the new DB right away, based on the values that are in the current DB.

Related is my comment #547 (comment), we could right after wp_install() also run logic to mark the Performance Lab and the current modules as active in the new SQLite DB.

Maybe I'm missing something here that makes this infeasible though?

In the week that has passed since this comment, I tried multiple iterations and approaches to this. Unfortunately, I was unable to make it work... I agree that it would be better UX if the database was auto-populated.
However, no matter what I tried, there are issues because it's a different database, there are timing issues, we actually need to have 2 different database connections open, and overall it's a nightmare 😰 If I'm not getting an install screen, I'm getting database errors. Switching databases on the fly is no easy task 😞

Since the "copying" functionality will not be part of Core if and when this gets merged, I don't think that getting an install screen would actually be a blocker to get this in the plugin 🤔
If anything, it will allow us to ensure that the actual WP installation runs smoothly when a user wants to use SQLite on their site.

aristath · 2022-10-25T08:21:49Z

Thank you for all the comments @felixarntz! I addressed all feedback and pushed lots of tweaks as per your comments 👍

modules/database/sqlite/load.php

felixarntz · 2022-10-25T17:52:31Z

modules/sqlite/integration/db.copy

+			activate_plugin( '{PERFLAB_PLUGIN}' );
+		}
+	}
+);


@aristath

It's not so much about deactivating the plugin... The problem is that when switching databases, the plugin is not active (since the list of active plugins is a db option).

Makes sense. However based on our conversation in #547 (comment), I would think that we can find a way to address this problem at its root: If we can install the SQLite DB prior to switching, we could also make sure that

the Performance Lab plugin is immediately active in the SQLite DB as well

the same modules that are configured in the old DB are active in the SQLite DB

I think the PERFLAB_VERSION constant check at least makes it a bit better, but I'd prefer if we could do something like the above.

felixarntz

@aristath Thanks for all the updates. I left a bit of follow-up feedback.

aristath · 2022-11-01T12:57:56Z

Pushed another round of fixes & refactors 👍

OllieJones · 2022-11-08T22:14:06Z

Here's a small thing. When the php interpreter doesn't have sqlite3 installed (as mine didn't) the settings page said

sqlite3 is already part of your WordPress version and therefore cannot be loaded as part of the plugin.

That's not the right diagnosis, obviously, but it happens on every can_load returning false.

So, users should not forget to do these things (ubuntu) to make sure they have sqlite3.

`sudo apt install sqlite3

sudo apt install php-sqlite3

sudo apachectl restart`

More reviewing to come...

aristath · 2022-11-09T06:32:45Z

Here's a small thing. When the php interpreter doesn't have sqlite3 installed (as mine didn't) the settings page said
sqlite3 is already part of your WordPress version and therefore cannot be loaded as part of the plugin.
That's not the right diagnosis, obviously, but it happens on every can_load returning false.

Good point! I pushed a commit which will allow us to set the message to be displayed when the module can't be loaded, and added the right messages in this module 👍

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

aristath · 2022-12-12T08:51:38Z

your recent commit eb77632 introduces a fatal error that needs to be addressed.

🤦 During my tests it happened that I already had an sqlite db on wp-content/database - so on activation of the module it was skipping the database creation - which is why I missed this.
Thank you for catching that, and my apologies for missing it in the first place 🙏

Applied your fixes and advice, everything is working correctly again 👍

felixarntz

Thanks @aristath, all good now! 🎉

I left one minor comment below; I can make that change really quickly, I think it's cleaner like that.

felixarntz · 2022-12-12T17:13:39Z

modules/database/sqlite/install-functions.php

+	);
+}
+
+$GLOBALS['wpdb'] = new Perflab_SQLite_DB();


@aristath Can we bring this back into the db.php file? I think this one is a bit confusing to have in a file called *-functions.php. Also, having this in the db.php file is not problematic as it will not execute based on the early return. This is only critical for functions as they would still be loaded into the global scope.

felixarntz · 2022-12-12T17:44:52Z

🎉🎉🎉

MarekTP · 2023-02-03T06:13:21Z

Nice work, usefull for many "like static", thx. Some notes:

Vacuum called by wp-cron would be good.
Data synchronisation part, between MariaDB/MySQL and SQLite, is on roadmap or no?
[alphabetical ordering (collation) does not work correctly in languages with ascended letters [like á, č, ř], but it's property of SQLite minimalism]

leo852 · 2024-05-24T18:48:52Z

Is there any plans to add support for native cloudflare D1 support?

aristath added no milestone PRs that do not have a defined milestone for release [Type] Module Proposal labels Sep 30, 2022

aristath marked this pull request as ready for review September 30, 2022 10:55

aristath force-pushed the module/sqlite branch from 56397b9 to eac1fb7 Compare October 4, 2022 09:33

aristath changed the title ~~WIP - SQLite integration module~~ SQLite integration module Oct 5, 2022

aristath force-pushed the module/sqlite branch from eac1fb7 to ad43168 Compare October 5, 2022 14:00

aristath force-pushed the module/sqlite branch 2 times, most recently from e6e9615 to edda8bd Compare October 18, 2022 06:33

aristath requested review from JustinyAhin and felixarntz as code owners October 18, 2022 06:38

aristath force-pushed the module/sqlite branch from 64256b5 to c841a26 Compare October 19, 2022 09:19

felixarntz reviewed Oct 24, 2022

View reviewed changes

aristath force-pushed the module/sqlite branch from 0da3745 to 3a2e351 Compare October 25, 2022 08:09

felixarntz reviewed Oct 25, 2022

View reviewed changes

aristath force-pushed the module/sqlite branch from 92d1c8c to e0e5dd6 Compare November 1, 2022 10:52

aristath force-pushed the module/sqlite branch 2 times, most recently from 83cae70 to e4fce10 Compare November 8, 2022 06:18

aristath requested a review from OllieJones as a code owner November 8, 2022 06:18

aristath force-pushed the module/sqlite branch from e4fce10 to 161daa9 Compare November 8, 2022 07:39

mxbclang added the Needs Review label Nov 8, 2022

aristath force-pushed the module/sqlite branch from 1290cfb to d75d91c Compare November 9, 2022 06:30

aristath and others added 9 commits December 12, 2022 10:13

Update admin/load.php

77f294c

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Update modules/database/sqlite/load.php

ef10d2f

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Update modules/database/sqlite/load.php

3ae41f9

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

rename db.copy file

7a2647e

Split install functions to separate file

8f698b9

Merge remote-tracking branch 'origin/module/sqlite' into module/sqlite

aafe8ea

move file

fd3fb8d

Update modules/database/sqlite/activate.php

d64c36c

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Merge remote-tracking branch 'origin/module/sqlite' into module/sqlite

2dcf03e

felixarntz approved these changes Dec 12, 2022

View reviewed changes

felixarntz added 2 commits December 12, 2022 09:35

Merge branch 'trunk' into module/sqlite

14fae02

Move global wpdb adjustment back into db.php file.

2b2fad2

felixarntz changed the title ~~SQLite integration module~~ Implement new experimental SQLite integration module Dec 12, 2022

felixarntz merged commit 1697790 into trunk Dec 12, 2022

felixarntz deleted the module/sqlite branch December 12, 2022 17:44

felixarntz added [Type] Feature A new feature within an existing module and removed no milestone PRs that do not have a defined milestone for release [Type] Module Proposal labels Dec 12, 2022

felixarntz mentioned this pull request Dec 12, 2022

Make WP filesystem setup more robust to prevent potential errors #595

Merged

3 tasks

mxbclang added the Needs Testing label Dec 13, 2022

eliot-akira mentioned this pull request Dec 17, 2022

Propose PRs for the projects that are now patched ad-hoc WordPress/wordpress-playground#91

Open

xet7 mentioned this pull request Dec 21, 2022

WordPress testing official SQLite Support. Useful? sandstormports/wordpress-sandstorm#44

Open

adamziel mentioned this pull request Feb 5, 2023

SQLite: Support PHPUnit test suite #644

Closed

eclarke1 removed the Needs Testing label Mar 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new experimental SQLite integration module #547

Implement new experimental SQLite integration module #547

aristath commented Sep 29, 2022 •

edited

Loading

saas786 commented Oct 10, 2022

felixarntz left a comment

felixarntz Oct 24, 2022

aristath Oct 25, 2022

felixarntz Oct 25, 2022

aristath Nov 1, 2022

aristath commented Oct 25, 2022

felixarntz Oct 25, 2022

felixarntz left a comment

aristath commented Nov 1, 2022

OllieJones commented Nov 8, 2022 •

edited

Loading

aristath commented Nov 9, 2022

aristath commented Dec 12, 2022 •

edited

Loading

felixarntz left a comment

felixarntz Dec 12, 2022

felixarntz commented Dec 12, 2022

MarekTP commented Feb 3, 2023

leo852 commented May 24, 2024

Implement new experimental SQLite integration module #547

Implement new experimental SQLite integration module #547

Conversation

aristath commented Sep 29, 2022 • edited Loading

Summary

Relevant technical choices

Checklist

saas786 commented Oct 10, 2022

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz Oct 24, 2022

Choose a reason for hiding this comment

aristath Oct 25, 2022

Choose a reason for hiding this comment

felixarntz Oct 25, 2022

Choose a reason for hiding this comment

aristath Nov 1, 2022

Choose a reason for hiding this comment

aristath commented Oct 25, 2022

felixarntz Oct 25, 2022

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

aristath commented Nov 1, 2022

OllieJones commented Nov 8, 2022 • edited Loading

aristath commented Nov 9, 2022

aristath commented Dec 12, 2022 • edited Loading

felixarntz left a comment

Choose a reason for hiding this comment

felixarntz Dec 12, 2022

Choose a reason for hiding this comment

felixarntz commented Dec 12, 2022

MarekTP commented Feb 3, 2023

leo852 commented May 24, 2024

aristath commented Sep 29, 2022 •

edited

Loading

OllieJones commented Nov 8, 2022 •

edited

Loading

aristath commented Dec 12, 2022 •

edited

Loading