Skip to content

Add an indicator in the adminbar to show when using SQLite #604

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

Merged
merged 5 commits into from
Feb 15, 2023

Conversation

aristath
Copy link
Member

@aristath aristath commented Dec 15, 2022

Summary

Adds an indicator in the adminbar to show when using SQLite:

Screenshot 2022-12-15 at 2 54 54 PM

Checklist

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

@aristath aristath added [Type] Enhancement A suggestion for improvement of an existing feature [Focus] Database labels Dec 15, 2022
@aristath aristath added this to the 1.8.0 milestone Dec 15, 2022
@aristath aristath requested a review from felixarntz December 15, 2022 12:58
@aristath aristath requested a review from OllieJones as a code owner December 15, 2022 12:58
@felixarntz
Copy link
Member

@aristath The change looks good to me, but I think we need to evaluate whether this is generally considered useful enough to take a spot in the admin bar. I think it's a bit too late for 1.8.0 since it's an enhancement that was just opened today and needs a bit of thought, so will move this to 1.9.0.

@felixarntz
Copy link
Member

@aristath We still haven't had any conversation about this. For example here's some questions I have which would be good to discuss:

  • Is this something that users of the module would want to see?
  • Is it justified to reserve some of the very limited space in the admin bar for a very basic piece of information like this?
  • Since it sets a precedent on adding PL information to the admin bar, should we rather think about a dedicated PL admin bar menu which could potentially incorporate other important heuristics than just this?

Last but not least, should we do this or should we not do this?

@bethanylang This is probably not a massive conversation to reserve an entire chat for, but still it could be a good topic to cover in one of the next ones.

For now I'll move this to the next release (2.0.0) since we need to create the 1.9.0 release branch today.

@felixarntz felixarntz modified the milestones: 1.9.0, 2.0.0 Jan 11, 2023
@felixarntz
Copy link
Member

@aristath Wondering what your take is on the above? What led you to opening this PR? :)

@aristath
Copy link
Member Author

Initially, when someone was activating the module, they were getting the install screen where they would set their site-name etc. With that workflow, I was just setting the site-name to SQLite test and I could immediately see (by the site-title) which db I'm working with.
In order to make the testing experience smoother, that process was automated and the installation of WP runs automatically, copying over the pre-existing sitename, admin-user etc. As a result, I no longer know which db I'm working with unless I go to site-health to check - or check the filesystem, to see if wp-content/db.php is there.
If we keep the current flow that runs during the module's activation, a visual indication of which DB is currently used would be useful.

@felixarntz
Copy link
Member

Thanks @aristath, makes sense. I was just a bit concerned to use the limited WP Admin bar space, but that use-case is reasonable to me.

However, I would advise we also show that menu item when not using SQLite, just as long as the module is active. We also need to consider the value of the DATABASE_TYPE constant. Maybe like this:

  • If PERFLAB_SQLITE_DB_DROPIN_VERSION is defined and DATABASE_TYPE is set to "sqlite", the menu displays in green something like "Database: SQLite".
  • Otherwise, the menu displays in red something like "Database: MySQL".

Or alternatively, we could just rely on $wpdb->db_server_info() to determine which DB is being used, this way we wouldn't need to deal with the constants at all, and it would look at the real source of truth (arguably the $wpdb class is the source of truth for what is actually being used). If we do that, we can even differentiate between "SQLite", "MySQL", and "MariaDB" if we want (the latter isn't really necessary but nice to have).

Let me know what you think. My main feedback would be to show the menu even when SQLite is not enabled but the module is enabled, as that would be a good indicator that something is wrong.

@felixarntz
Copy link
Member

@aristath Friendly ping ^

@SergeyBiryukov
Copy link
Member

Or alternatively, we could just rely on $wpdb->db_server_info() to determine which DB is being used, this way we wouldn't need to deal with the constants at all, and it would look at the real source of truth (arguably the $wpdb class is the source of truth for what is actually being used). If we do that, we can even differentiate between "SQLite", "MySQL", and "MariaDB" if we want (the latter isn't really necessary but nice to have).

Sounds good to me 👍

@aristath aristath force-pushed the add/sqlite-adminmenu-indicator branch from 5c49aca to 5dfa9c2 Compare February 15, 2023 12:44
@aristath
Copy link
Member Author

  • If PERFLAB_SQLITE_DB_DROPIN_VERSION is defined and DATABASE_TYPE is set to "sqlite", the menu displays in green something like "Database: SQLite".
  • Otherwise, the menu displays in red something like "Database: MySQL".

Done.
I also rebased the PR to bring it up to speed with trunk 👍

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@aristath A bit of additional follow up feedback.

/* translators: %s: SQLite icon. */
'title' => defined( 'PERFLAB_SQLITE_DB_DROPIN_VERSION' ) && defined( 'DATABASE_TYPE' ) && 'sqlite' === DATABASE_TYPE
? '<span style="color:#46B450;">' . __( 'Database: SQLite', 'performance-lab' ) . '</span>'
: '<span style="color:#DC3232;">' . __( 'Database: MySQL', 'performance-lab' ) . '</span>',
Copy link
Member

Choose a reason for hiding this comment

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

See my comment #604 (comment) (also #604 (comment)), we should use $wpdb->db_server_info() to display more accurate information. It's not always MySQL, it can also be MariaDB.

Also noting that inlining this conditional here makes this hard to parse, we can simplify this by e.g. using a $title variable.

@@ -58,3 +58,23 @@ function perflab_sqlite_plugin_admin_notice() {
);
}
add_action( 'admin_notices', 'perflab_sqlite_plugin_admin_notice' ); // Add the admin notices.

/**
* Add a link to the admin bar.
Copy link
Member

Choose a reason for hiding this comment

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

Function descriptions should start with a third-person verb per WP docs standards. Also, we need to add a @since annotation.

Suggested change
* Add a link to the admin bar.
* Adds a link to the admin bar.
*
* @since n.e.x.t

@felixarntz
Copy link
Member

@aristath In the interest of time, I've quickly addressed the outstanding feedback myself in 6bf9328, since we preferably need to merge this today to get it into the 2.0.0 release.

@SergeyBiryukov Can you give this a quick review, since you previously commented (and I rather don't just approve my own additions 😄)?

@felixarntz
Copy link
Member

@aristath Can you add this change to the standalone plugin as well please?

@aristath
Copy link
Member Author

Will do 👍

@felixarntz
Copy link
Member

I'm gonna go ahead and merge this in order to create the new release branch. If there's anything that should be changed, we can always open a quick follow up PR against that branch release/2.0.0.

@felixarntz felixarntz merged commit 134221b into trunk Feb 15, 2023
@felixarntz felixarntz deleted the add/sqlite-adminmenu-indicator branch February 15, 2023 23:30
aristath added a commit to WordPress/sqlite-database-integration that referenced this pull request Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants