https://github.com/WordPress/performance/pull/547 Skip to content Toggle navigation Sign up * Product + Actions Automate any workflow + Packages Host and manage packages + Security Find and fix vulnerabilities + Codespaces Instant dev environments + Copilot Write better code with AI + Code review Manage code changes + Issues Plan and track work + Discussions Collaborate outside of code + Explore + All features + Documentation + GitHub Skills + Blog * Solutions + For + Enterprise + Teams + Startups + Education + By Solution + CI/CD & Automation + DevOps + DevSecOps + Case Studies + Customer Stories + Resources * Open Source + GitHub Sponsors Fund open source developers + The ReadME Project GitHub community articles + Repositories + Topics + Trending + Collections * Pricing [ ] * # In this repository All GitHub | Jump to | * No suggested jump to results * # In this repository All GitHub | Jump to | * # In this organization All GitHub | Jump to | * # In this repository All GitHub | Jump to | Sign in Sign up {{ message }} WordPress / performance Public * Notifications * Fork 54 * Star 241 * Code * Issues 120 * Pull requests 17 * Actions * Projects 7 * Security * Insights More * Code * Issues * Pull requests * Actions * Projects * Security * Insights 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. Pick a username [ ] Email Address [ ] Password [ ] [ ] Sign up for GitHub 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 Jump to bottom Implement new experimental SQLite integration module #547 Merged felixarntz merged 129 commits into trunk from module/sqlite Dec 12, 2022 Merged Implement new experimental SQLite integration module #547 felixarntz merged 129 commits into trunk from module/sqlite Dec 12, 2022 +5,444 -3 Conversation 206 Commits 129 Checks 23 Files changed 22 Conversation This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters Show hidden characters aristath Copy link Member @aristath aristath commented Sep 29, 2022 * edited 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. Sorry, something went wrong. 8 lkraav, saas786, PatelUtkarsh, lukecav, tlaverdure, Soean, rilwis, and simos reacted with thumbs up emoji 7 mukeshpanchal27, felixarntz, srtfisher, PatelUtkarsh, tlaverdure, joldeski, and Soean reacted with hooray emoji 1 simos reacted with rocket emoji All reactions * 8 reactions * 7 reactions * 1 reaction @aristath aristath added no milestone PRs that do not have a defined milestone for release [Type] Module Proposal A new module proposal [Focus] Database Issues related to the Database focus area labels Sep 30, 2022 @aristath aristath marked this pull request as ready for review Sep 30, 2022 @aristath aristath force-pushed the module/sqlite branch from 56397b9 to eac1fb7 Compare Oct 4, 2022 @aristath aristath changed the title [DEL:WIP - SQLite integration module:DEL] [INS:SQLite integration module:INS] Oct 5, 2022 @aristath aristath force-pushed the module/sqlite branch from eac1fb7 to ad43168 Compare Oct 5, 2022 @saas786 Copy link saas786 commented Oct 10, 2022 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. 2 Soean and saas786 reacted with thumbs up emoji All reactions * 2 reactions Sorry, something went wrong. @aristath aristath force-pushed the module/sqlite branch 2 times, most recently from e6e9615 to edda8bd Compare Oct 18, 2022 @aristath aristath requested review from JustinyAhin and felixarntz as code owners Oct 18, 2022 @bethanylang bethanylang added this to In progress in [Focus] Database Oct 18, 2022 @aristath aristath force-pushed the module/sqlite branch from 64256b5 to c841a26 Compare Oct 19, 2022 felixarntz felixarntz reviewed Oct 24, 2022 View changes Copy link Member @felixarntz felixarntz left a comment @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. Sorry, something went wrong. All reactions modules/sqlite/integration/load.php Outdated Show resolved Hide resolved admin/load.php Outdated Show resolved Hide resolved modules/sqlite/integration/load.php Outdated Show resolved Hide resolved modules/sqlite/integration/load.php Outdated Show resolved Hide resolved modules/sqlite/integration/load.php Outdated Show resolved Hide resolved 8 hidden conversations Load more... modules/sqlite/integration/wp-includes/sqlite/class-wp-pdo-engine.php Outdated Show resolved Hide resolved modules/sqlite/integration/wp-includes/sqlite/db.php Outdated Show resolved Hide resolved modules/sqlite/integration/wp-includes/sqlite/db.php Outdated Show resolved Hide resolved modules/sqlite/integration/wp-includes/sqlite/db.php Outdated Show resolved Hide resolved modules/sqlite/integration/load.php Outdated ), file_get_contents( __DIR__ . '/db.copy' ) ); $wp_filesystem->put_contents( $destination, $file_contents ); Copy link Member @felixarntz felixarntz Oct 24, 2022 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. [Choose a reason] Hide comment 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. Sorry, something went wrong. All reactions Copy link Member Author @aristath aristath Oct 25, 2022 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. [Choose a reason] Hide comment 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. Sorry, something went wrong. All reactions Copy link Member @felixarntz felixarntz Oct 25, 2022 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. [Choose a reason] Hide comment @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? Sorry, something went wrong. All reactions Copy link Member Author @aristath aristath Nov 1, 2022 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. [Choose a reason] Hide comment 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. Sorry, something went wrong. All reactions @aristath aristath force-pushed the module/sqlite branch from 0da3745 to 3a2e351 Compare Oct 25, 2022 @aristath Copy link Member Author aristath commented Oct 25, 2022 Thank you for all the comments @felixarntz! I addressed all feedback and pushed lots of tweaks as per your comments 2 saas786 and felixarntz reacted with thumbs up emoji All reactions * 2 reactions Sorry, something went wrong. felixarntz felixarntz reviewed Oct 25, 2022 View changes modules/database/sqlite/load.php Outdated Show resolved Hide resolved modules/database/sqlite/load.php Outdated Show resolved Hide resolved modules/database/sqlite/load.php Outdated Show resolved Hide resolved modules/sqlite/integration/db.copy Outdated activate_plugin( '{PERFLAB_PLUGIN}' ); } } ); Copy link Member @felixarntz felixarntz Oct 25, 2022 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. [Choose a reason] Hide comment @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. Sorry, something went wrong. 1 saas786 reacted with thumbs up emoji All reactions * 1 reaction felixarntz felixarntz reviewed Oct 25, 2022 View changes Copy link Member @felixarntz felixarntz left a comment @aristath Thanks for all the updates. I left a bit of follow-up feedback. Sorry, something went wrong. All reactions @aristath aristath force-pushed the module/sqlite branch from 92d1c8c to e0e5dd6 Compare Nov 1, 2022 @aristath Copy link Member Author aristath commented Nov 1, 2022 Pushed another round of fixes & refactors 1 saas786 reacted with thumbs up emoji 1 joldeski reacted with hooray emoji All reactions * 1 reaction * 1 reaction Sorry, something went wrong. @aristath aristath force-pushed the module/sqlite branch 2 times, most recently from 83cae70 to e4fce10 Compare Nov 8, 2022 @aristath aristath requested a review from OllieJones as a code owner Nov 8, 2022 @aristath aristath force-pushed the module/sqlite branch from e4fce10 to 161daa9 Compare Nov 8, 2022 @bethanylang bethanylang added the Needs Review Anything that requires code review label Nov 8, 2022 @bethanylang bethanylang moved this from In progress to Review in [Focus] Database Nov 8, 2022 @OllieJones Copy link Contributor OllieJones commented Nov 8, 2022 * edited 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... All reactions Sorry, something went wrong. @aristath aristath force-pushed the module/sqlite branch from 1290cfb to d75d91c Compare Nov 9, 2022 @aristath Copy link Member Author aristath commented Nov 9, 2022 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 1 saas786 reacted with thumbs up emoji All reactions * 1 reaction Sorry, something went wrong. 149 hidden items Load more... mukeshpanchal27 mukeshpanchal27 reviewed Dec 7, 2022 View changes Copy link Member @mukeshpanchal27 mukeshpanchal27 left a comment Thanks @aristath It looks good to me. Left some nit-pick and questions. Not a critical or blocker. Sorry, something went wrong. 2 aristath and saas786 reacted with rocket emoji All reactions * 2 reactions modules/database/sqlite/load.php Show resolved Hide resolved modules/database/sqlite/wp-includes/sqlite/db.php Outdated Show resolved Hide resolved modules/database/sqlite/wp-includes/sqlite/db.php Outdated Show resolved Hide resolved modules/database/sqlite/wp-includes/sqlite/db.php Outdated Show resolved Hide resolved modules/database/sqlite/wp-includes/sqlite/db.php Outdated Show resolved Hide resolved aristath and others added 5 commits Dec 7, 2022 @aristath fix admin notice accb7f8 @aristath @mukeshpanchal27 Update modules/database/sqlite/wp-includes/sqlite/db.php ... c5d73a3 Co-authored-by: Mukesh Panchal @aristath @mukeshpanchal27 Update modules/database/sqlite/wp-includes/sqlite/db.php ... 7458780 Co-authored-by: Mukesh Panchal @aristath @mukeshpanchal27 Update modules/database/sqlite/wp-includes/sqlite/db.php ... d019817 Co-authored-by: Mukesh Panchal @aristath address issues with site-health when db.php is missing eb77632 felixarntz felixarntz requested changes Dec 9, 2022 View changes Copy link Member @felixarntz felixarntz left a comment @aristath Left a few nit-pick comments, however your recent commit eb77632 introduces a fatal error that needs to be addressed. Sorry, something went wrong. All reactions admin/load.php Outdated Show resolved Hide resolved modules/database/sqlite/load.php Outdated Show resolved Hide resolved modules/database/sqlite/load.php Outdated Show resolved Hide resolved modules/database/sqlite/db.copy Outdated Show resolved Hide resolved modules/database/sqlite/activate.php Outdated Show resolved Hide resolved modules/database/sqlite/wp-includes/sqlite/db.php Show resolved Hide resolved aristath and others added 9 commits Dec 12, 2022 @aristath @felixarntz Update admin/load.php ... 77f294c Co-authored-by: Felix Arntz @aristath @felixarntz Update modules/database/sqlite/load.php ... ef10d2f Co-authored-by: Felix Arntz @aristath @felixarntz Update modules/database/sqlite/load.php ... 3ae41f9 Co-authored-by: Felix Arntz @aristath rename db.copy file 7a2647e @aristath Split install functions to separate file 8f698b9 @aristath Merge remote-tracking branch 'origin/module/sqlite' into module/ sqlite aafe8ea @aristath move file fd3fb8d @aristath @felixarntz Update modules/database/sqlite/activate.php ... d64c36c Co-authored-by: Felix Arntz @aristath Merge remote-tracking branch 'origin/module/sqlite' into module/ sqlite 2dcf03e @aristath Copy link Member Author aristath commented Dec 12, 2022 * edited 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 2 felixarntz and simos reacted with thumbs up emoji All reactions * 2 reactions Sorry, something went wrong. felixarntz felixarntz approved these changes Dec 12, 2022 View changes Copy link Member @felixarntz felixarntz left a comment 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. Sorry, something went wrong. All reactions modules/database/sqlite/install-functions.php Outdated ); } $GLOBALS['wpdb'] = new Perflab_SQLite_DB(); Copy link Member @felixarntz felixarntz Dec 12, 2022 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. [Choose a reason] Hide comment @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. Sorry, something went wrong. All reactions felixarntz added 2 commits Dec 12, 2022 @felixarntz Merge branch 'trunk' into module/sqlite 14fae02 @felixarntz Move global wpdb adjustment back into db.php file. 2b2fad2 @felixarntz felixarntz changed the title [DEL:SQLite integration module:DEL] [INS:Implement new experimental SQLite integration module :INS] Dec 12, 2022 Hide details View details @felixarntz felixarntz merged commit 1697790 into trunk Dec 12, 2022 8 checks passed @felixarntz felixarntz deleted the module/sqlite branch Dec 12, 2022 @felixarntz Copy link Member felixarntz commented Dec 12, 2022 2 mukeshpanchal27 and eclarke1 reacted with hooray emoji 4 aristath, Soean, mukeshpanchal27, and eclarke1 reacted with rocket emoji All reactions * 2 reactions * 4 reactions Sorry, something went wrong. @felixarntz 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 A new module proposal Needs Review Anything that requires code review labels Dec 12, 2022 @felixarntz felixarntz mentioned this pull request Dec 12, 2022 Make WP filesystem setup more robust to prevent potential errors #595 Merged 3 tasks @bethanylang bethanylang added the Needs Testing Anything that needs testing from a developer perspective label Dec 13, 2022 Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment Reviewers @tillkruss tillkruss @Soean Soean @eugene-manuilov eugene-manuilov @mehulkaklotar mehulkaklotar @OllieJones OllieJones @mukeshpanchal27 mukeshpanchal27 @felixarntz felixarntz @JustinyAhin JustinyAhin Assignees No one assigned Labels [Focus] Database Issues related to the Database focus area Needs Testing Anything that needs testing from a developer perspective [Type] Feature A new feature within an existing module Projects [Focus] Database Review Milestone 1.8.0 Development Successfully merging this pull request may close these issues. None yet 10 participants @aristath @saas786 @OllieJones @felixarntz @tillkruss @Soean @eugene-manuilov @mehulkaklotar @mukeshpanchal27 @bethanylang Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Footer (c) 2022 GitHub, Inc. Footer navigation * Terms * Privacy * Security * Status * Docs * Contact GitHub * Pricing * API * Training * Blog * About You can't perform that action at this time. You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.