search

lcache / wp-lcache

A WordPress implementation of LCache
https://wordpress.org/plugins/wp-lcache/
GNU General Public License v2.0
88 stars 5 forks source link

Remove lcache_tags table when LCache supports disabling tags #123

Open frankiejarrett opened 7 years ago

frankiejarrett commented 7 years ago

In all my testing, the lcache_tags table appears to be unused.

Should it be removed from the project?

danielbachhuber commented 7 years ago

In all my testing, the lcache_tags table appears to be unused.

It's not used by WP LCache, but it's technically a feature available in the LCache library.

Should it be removed from the project?

I'm on the fence about this.

On one hand, I don't like the idea of creating a bunch of unused database tables in people's WP installs.

On the other hand, I don't like the WP LCache breaking unexpectedly because of the missing database table.

Thoughts?

frankiejarrett commented 7 years ago

@danielbachhuber I'd think there could just be a wp_lcache_version option which we check after an update and run migrations when tables/schema changes occur in a particular version.

danielbachhuber commented 7 years ago

@fjarrett I've pinged inside Pantheon to see if there are broader opinions about this.

What's your timeline for needing a decision? Are you planning a broader rollout?

frankiejarrett commented 7 years ago

@danielbachhuber No timeframe for this one, just looking to trim some fat. From a broader point of view, yes I am investigating whether a broad rollout of WP Lcache would be feasible in it's current state, and helping out on issues I think are relevant to that being a success.

frankiejarrett commented 7 years ago

@danielbachhuber From what I can tell, the object-cache.php drop-in does not provide the $tags param in any of the places it's calling the set() method. Additionally, I did a quick scan of the LCache library and everywhere $tags could be called, it defaults to an empty array.

IMO it seems safe to remove the table until such a time the drop-in decides to start using tags.

I took a stab at this in #124, also providing a database migration pattern for changing it back in the future. Would like your thoughts/opinions here.

danielbachhuber commented 7 years ago

From what I can tell, the object-cache.php drop-in does not provide the $tags param in any of the places it's calling the set() method. Additionally, I did a quick scan of the LCache library and everywhere $tags could be called, it defaults to an empty array.

Correct.

IMO it seems safe to remove the table until such a time the drop-in decides to start using tags.

Here's what @davidstrauss said, which makes sense to me:

dts [5:05 PM] I would want to allow initializing the library in a "no tags" mode first.

So, I think the LCache library should explicitly support this first, then we can conditionally create the table.