Seems to be installing the table without ->install()

[rmorse]

Hi Alex + co!

Checking the sample plugin I see code like:

// Instantiate the Books Table class.
$table = new Books_Table;

// Uninstall the database. Uncomment this code to force the database to rebuild.
//if($table->exists()){
//	$table->uninstall();
//}

// If the table does not exist, then create the table.
if ( ! $table->exists() ) {
	$table->install();
}

However, running just:

// Instantiate the Books Table class.
$table = new Books_Table;

Seems to create the table anyway...

I had planned to put the install logic on plugin activation (assuming that's the best place for something like this).

Also, I did notice, when I delete the wp_options option, with the table version number (that is also auto created), this behaviour did not re-occur. I guess maybe there is check for an existing version number which triggers this behaviour.

Thanks!

[alexstandiford]

Hey @rmorse!

Okay, so I did a little digging to figure out what's going on here. I knew that BerlinDB does install tables somewhere automatically but couldn't for the life of me remember if that was something that was actually in core, or if that's something that I had just built into Underpin. Turns out, it is in core:

core/src/Database/Table.php

Line 912 in 0a9d65e

add_action( 'admin_init', array( $this, 'maybe_upgrade' ) );

So As you can see above, the table will automatically when someone visits an admin page, and it will only install if the tables haven't already been set-up. Generally, this happens automatically as soon as the plugin is activated.

So, technically, this bit of code isn't necessary:

// If the table does not exist, then create the table.
if ( ! $table->exists() ) {
	$table->install();
}

However, I do think it helps to rebuild the table when the uninstall snippet is added above.

Perhaps the comment above the table->exists check should be more specific, and explain this?

[rmorse]

Hey @alexstandiford !

Thanks for the reply - yeah I had started digging though the code to see what it does but didn't find the trigger.

As long I know how it works I think I'm happy with this implemenation - I will probably try to ensure that the tables can only be created in admin, and I think the solution for that is to simply only include this:

$table = new Books_Table;

in some admin facing part of the plugin.

** edit, thats totally not necessary because you're doing that already via admin_init

Re the ->install()

From what you've told me and what I can see my understanding is:

• The auto install of a table (when instantiating the class only) occured when there was also no corresponding version number in the options table
• Instatiating the class builds the table via maybe_upgrade (and creates a corresponding version number in wp_options)
• maybe_upgrade returns early if it's not upgradable / doesn't need an update - having the version number in the wp_options table existing seems like the only scenario the table is not rebuilt automatically - by falling into one of these early returns.
• install() can be used to force install the tables even if a version number is already present

I'll do some more testing, but if my assumptions are correct, if you use ->uninstall() (from what I can tell it removes both the version number from wp_options and the table itself) then the table would probably be auto created again via maybe_create (by just instantiating the class) - because it won't return early...

I'll be working on DB stuff again probably tomorrow and will run these scenarios though (and share my findings).

Thanks

[rmorse]

After thought - if I don't instantiate the class

$table = new Books_Table;

until after admin_init, then maybe_upgrade won't get fired at all and the behaviour would be a bit different (requiring us to use ->install() at that point instead

[rmorse]

Hey @alexstandiford

So I did a bit more digging and I think my assumptions above are correct:

1. When the class is insantiated before admin_init, an upgrade check is performed, and if upgradable, or no version number present, the table is installed/upgraded (which will happen on first use).
2. When instatiating the class after admin_init, I believe, the auto install/upgrade will never complete because the build of the table (from maybe_upgrade) is done in admin_init hook
3. ->uninstall() clears out both the table and the version number - meaning this would send you into a kind of loop (of tables being created + destroyed) if you did this before admin_init:

$table = new Books_Table; // This will create the table
$table->uninstall(); // Removes the table (and version number) again

So I think the takeaway is, if you are instantiating the class before admin_init you will get some automation occuring, but if for some reason you do it somewhere after, you will need to explicity call ->install() and ->upgrade() (or just ->maybe_upgrade())

Is that how you see it all working?

Also: Would be happy to help where I can (just point me in the right direction) and now I've realised this ticket should have been in core 🙄

An extra thought - would it be worth implementing an opt in / out of this behaviour when instantiating the class?

[alexstandiford]

now I've realised this ticket should have been in core roll_eyes

No worries - I went ahead and transferred this issue to core! 😎

Yep, what you're saying matches my expectations. I usually use $table->uninstall() inside a plugin's uninstall.php file, but in the context of the demo plugin it's just being used to force the site to re-install itself.

I'm kinda iffy on creating some kind-of opt-in/out behavior, but if anything I could see a private property being added to the Table class that can toggle versioning for a table.

[JJJ]

My original intention for the database tables being auto-managed was only to make installing and upgrading them as easy as possible. I did not care much about the idea of deleting them (in the same way we don't know if Apple Notes or Calendar delete their SQLite tables in our iPhones and iPads.)

That is to say: I can imagine how uninstalling tables could be less smooth, because I hadn't designed it to be elegant 😅

My gut tells me this might be an implementation detail best suited for the application using Berlin to address (by overriding the public methods in the Table class) rather than Berlin handling it internally, but let's talk it out anyways.

---

If I was going to design a solution for a persistent uninstall, the Table class would need a way to know not to automatically reinstall the database tables on the subsequent include, and that way probably needs to be identical to how it knows what upgrades are pending – an option in the database.

Perhaps Berlin could use a unique value in the db_version_key like uninstalled – that way it doesn't need to reinvent or abstract the logic used for generating the db_version_key format? Then it just looks for that value in maybe_upgrade() and bail early if so?

Once that option is in the database, how will a Table class ever know that it is reinstallable? There isn't a way with 100% certainty to execute code when a plugin is deleted/purged from the file system; all it's got is when someone uninstalls or deletes a plugin using the admin-area UI.

Is that enough?

When would Berlin ever outright delete the options (indicating reinstalling is OK) vs. setting the option values to uninstalled (indicating reinstalling is not OK)?

---

Or... would it be enough to simply not immediately trigger a reinstall on the subsequent request?

The uninstall() method could do:

$this->set_db_version( 'uninstalling' );

Add an is_uninstalling() method:

	/**
	 * Return whether this table is being uninstalled.
	 *
	 * @since 2.1.0
	 *
	 * @return bool True if table is being uninstalled. False if not.
	 */
	public function is_uninstalling() {

		// Get the current database version
		$this->get_db_version();

		// Is the database table being uninstalled?
		$is_uninstalling = ( 'uninstalling' === $this->db_version );

		// Return true if uninstalling, false if not
		return (bool) $is_uninstalling;
	}

And the maybe_upgrade() method could do:

		// Bail if uninstalling
		if ( $this->is_uninstalling() ) {
			$this->delete_db_version();
			return;
		}

It's not much, but at least that gives plugin authors some window?

[alexstandiford]

I kind of think you could create a protected property auto_install that can be set to true/false, which determines if the table should use the auto install process provided. Default as true. Let developers figure out how to set that from there.

[rmorse]

Hey @JJJ + @alexstandiford , thanks so much for the feedback and details.

My ticket really didn't start as a request to change anything, rather, understand in absolute terms what is occuring and why...

I think it all stems from the - not knowing what is being automated - and therefor a desire to have absolute control over the inner workings... but I think overriding the default class would be a fine way to do it.

I think even making this clear in docs somehow would have prevented me from opening this ticket altogether.,

That all being said, in the name of rounding off all areas of the library, having a look at the uninstall + *automation scenarios couldn't hurt, but in terms of coming up with implementation ideas, I think they are best left in both your capable hands! :)

I'll mull this over anyway as I'm using this on a plugin I'm working on and no doubt will come up with something (...but perhaps nothing) over the next couple of weeks.

Thanks for talking this out!