-
Notifications
You must be signed in to change notification settings - Fork 5
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
Remove lcache_tags table when LCache supports disabling tags #123
Comments
It's not used by WP LCache, but it's technically a feature available in the LCache library.
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? |
@danielbachhuber I'd think there could just be a |
@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? |
@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. |
@danielbachhuber From what I can tell, the 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. |
Correct.
Here's what @davidstrauss said, which makes sense to me:
So, I think the LCache library should explicitly support this first, then we can conditionally create the table. |
In all my testing, the
lcache_tags
table appears to be unused.Should it be removed from the project?
The text was updated successfully, but these errors were encountered: