Skip to content
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

plugin API: Improve plugins' ability to indicate custom DB tables #317

Closed
garvinhicking opened this Issue Mar 9, 2015 · 16 comments

Comments

Projects
None yet
4 participants
@garvinhicking
Copy link
Member

garvinhicking commented Mar 9, 2015

Create a new array structure (or something like that) in the introspect-method to let plugins declare which tables they create with which structure. Allow to use specific version numbers to later upgrades to the schema (staticpages is a good example).

Create a new plugin API method like "init_tables()" which replicate the current custom functionality in plugins that use their "db_version" strings (use db_version for each plugin that supports custom table structure?).

Add a version check to only use those new methods when s9y 2.1+ is used, so that those plugins can be upgraded but will stil work on older environments for the time being.

@onli

This comment has been minimized.

Copy link
Member

onli commented Mar 9, 2015

I added a feature branch containing a first proposal for that feature: https://github.com/s9y/Serendipity/tree/feature_dbpluginapi

@onli onli added this to the 2.x.0 milestone Mar 14, 2015

@garvinhicking

This comment has been minimized.

Copy link
Member Author

garvinhicking commented Mar 18, 2015

@onli I committed some changes that also reference Issue #318 - can you check it out and tell me what you think? @ophian please also feel free to comment.

If it's ok I'd like to merge that to master.

@ddeimeke - If you want/can, please also check out the new maintenance section :)

garvinhicking added a commit that referenced this issue Mar 18, 2015

* Add unused plugin detection
* Change wy to query tables to list all possibly non-matching tables

Please discuss in issue #317
@ddeimeke

This comment has been minimized.

Copy link

ddeimeke commented Mar 18, 2015

No new items found in "Wartung". Do I need to configure something?

@garvinhicking

This comment has been minimized.

Copy link
Member Author

garvinhicking commented Mar 18, 2015

@ddeimeke Sorry. You need to check out the "feature_dbpluginapi" branch... however, this does not contain the other master changes. So you shouldn't use that on your live blog, but install a fresh one with that branch.

Or wait until @onli approves so that we can merge it with master for you to test with.

Or, you could manually copy over those files:

to your current blog...

@ddeimeke

This comment has been minimized.

Copy link

ddeimeke commented Mar 18, 2015

@garvinhicking No issues with that.

I wait for @onli and the merge to master (easier for me).

@onli

This comment has been minimized.

Copy link
Member

onli commented Mar 19, 2015

If it's ok I'd like to merge that to master.

I think that is too soon. The api is not ready - we need a (basic) mechanism to upgrade a database. And I don't think I checked, but we need to make sure the install-function is executed afterwards, to make it possible to add manual indexes. Also, what is missing is to support db_schema, to make that work properly with different databases.

I'll add another comment to your commit itself.

@garvinhicking

This comment has been minimized.

Copy link
Member Author

garvinhicking commented Mar 19, 2015

Difficult to write that the right way. I am just not too happy with that :-/

No offense taken, nor intented by my response. I appreciate your input. I'd like to hear @ddeimeke chime in, who was leading the charge of wanting this feature. :-)

That is a solution that would work now, and it seems like it will work nicely. But what I imagined we >would have is a small api-field to register a table, and then a small piece of code to show when such >a database table is still there but not used anymore (and of course in the core code to create and >update the table, but that is a different story)
This is doing more, and it is not small anymore. I realize you proposed that in 5632341 but got no >answer, sorry about that!
You remember probably that I was not too keen on the idea of the db cleanup, and maybe that is the >same thing. But to show all tables that don't belong to s9y, that does not seem like it should be our >work, and part of the core. There is already the prefix for that (I might miss something here).

I think otherwise; if we have a feature that deals with stale tables, we really need to make it work properly, and take the real-world scenario into account. If we only supported the API way and not look at existing plugins, that wouldn't benefit the users (for which we want such a feature).

$legacy_plugintables looks like a maintenance nightmare to me.

It would not be; for all future plugins, the "tables" api mechanism would meant to be used. I wouldn't plan on updating that array ever.

Currently I think we have two issues mixed up with each other: One is the maintenance option, and one is the DB plugin API. While I think that the DB plugin API needs more improvement, I do like the maintenance task at this point, and that it takes care of all tables.

I can surely hold off on merging the code if you want to improve the DB plugin API at first.

I would also be fine with dropping $legacy_plugintables, if we update ALL spartacus plugins to support the 'tables' mechanism. But that seems like a whole lot of work, because very many plugins use different ways of initializing the database. I'd rather like to make a "fresh" start for new plugins that should use this "tables" feature, and not backport it to all old plugins because it could really break; and we'd have to redundantly always have a fall-back way for serendipity < 2.1 so that plugins would work.

This is why I currently honestly think the $legacy_plugintables approach is the pragmatic, workable way...

@ddeimeke

This comment has been minimized.

Copy link

ddeimeke commented Mar 19, 2015

@garvinhicking Currently it is difficult to follow, because you quote a post, which I am not able to see.

Coming from the maintenance perspective:

My very pragmatic approach would be filling a "transition table" which includes a mapping between plugins and tables. This would be a one time effort.

Future plugins or future plugin versions could register their needs in that table on their own.

A second table would be needed to classify the plugins as part of the core.

Last step would be a check. Show all plugins which are disabled and not part of the core. Check with the database which tables are affected (and exclusively used by disabled plugins). Offer to delete the plugin from disk and the table from the database.

I don't know enough about internal structures to know if that is possible.

@garvinhicking

This comment has been minimized.

Copy link
Member Author

garvinhicking commented Mar 19, 2015

@ddeimeke Sorry. Was quoting this: 90f8020#commitcomment-10282829

(Your idea goes a bit into the currently implemented direction; of course with the "nasty" sideeffects that onli mentions, because it's basically redundancy right now)

Maybe it would be great if you could copy over the files I mentioned in the post earlier to your current blog (make a backup of the files you currently use) to see the current implementation of it. It only affects the maintenance section, so that shouldn't interfer. (If you can create a clone of your blog, that would be best though, if you have means to easily duplicate your blog with files + database)

@onli

This comment has been minimized.

Copy link
Member

onli commented Mar 20, 2015

If we only supported the API way and not look at existing plugins, that wouldn't benefit the users (for which we want such a feature).

I think part of my doubts is that this is not totally the feature I wanted ;) I understood we wanted to list plugin-tables, not potentially all of them. See also Dirks last description :)
I fear that - even though the code is filtering for the serendipity-database - that goes in the direction of a database-management-tool like phpmyadmin, and that it is confronting users with knowledge they should never need.

Currently I think we have two issues mixed up with each other: One is the maintenance option, and one is the DB plugin API.

Yes, right. That is unfortunate :/

Your idea goes a bit into the currently implemented direction;

That is right, Dirk basically described the $legacy_plugins-array. So, if that is to stay, maybe we can hide that at least? It could go include/tpl/ or include/db/ or something like that and only be included here?

And then we could maybe break that out of the db-plugin-api and have that feature already, while having time to improve the API separately? Maybe that is not really needed, but I'm not sure how difficult that will be to do right.

@ddeimeke

This comment has been minimized.

Copy link

ddeimeke commented Mar 25, 2015

@garvinhicking Contentwise it is exactly what I wanted. The only thing I can not see is, if the plugins are part of the core.

Designwise there could be some makeover. The boxes are really large with a lot of whitespace.

@onli

This comment has been minimized.

Copy link
Member

onli commented Mar 26, 2015

I made a new issue for the table list in the maintenance section, #332. We should discuss there the details.

@onli

This comment has been minimized.

Copy link
Member

onli commented Jan 15, 2016

@garvinhicking Shall we still add the API, minus the upgrade functionality, or are you happy with the current solution in 90f8020 ?

@garvinhicking

This comment has been minimized.

Copy link
Member Author

garvinhicking commented Jan 15, 2016

Not sure of the state right now. Maybe we could discuss this in our s9y meetup?

@onli

This comment has been minimized.

Copy link
Member

onli commented Jan 15, 2016

Then let's ignore it for 2.1

@yellowled yellowled removed the 2.0 label Jan 15, 2016

@garvinhicking garvinhicking modified the milestones: x.0.0, 2.x.0 Apr 8, 2017

@onli

This comment has been minimized.

Copy link
Member

onli commented Jun 4, 2017

I don't think I'll revive this effort. I'll close it, we might pick this up if we ever rework the database api.

@onli onli closed this Jun 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.