My advice is to save module settings in {variable} table, not in custom {tvi_settings}, because

  1. It is much quicker to run varialbe_get() thant run db_query() - this is very usefull for highload sites
  2. Storing settings in {variable} makes module compatible with Feature module for easy deploy

i attach patch that modify tvi.query.inc file

Notice to module author: If you will use this modification in future release don't forget to

  1. Remove schema from tvi.install file because it is unused
  2. Add hook_update() to tvi.install which will copy current settings from old table to {variable}
CommentFileSizeAuthor
#15 tvi-use-variable-table-1585350-15.patch6.97 KBduaelfr
#15 tvi-uuid-variable-merge-4.patch7.45 KBduaelfr
#13 tvi-use-variable-table-1585350-13.patch3.41 KBduaelfr
#13 tvi-uuid-variable-merge-3.patch4.86 KBduaelfr
#12 tvi-use-variable-table-1585350-12.patch3.42 KBduaelfr
#12 tvi-uuid-variable-merge-2.patch4.86 KBduaelfr
#11 tvi-uuid-variable-merge.patch6.45 KBduaelfr
#10 tvi-use-variable-table-1585350-10.patch6.05 KBduaelfr
#9 tvi-use-variable-table-1585350-9.patch6.07 KBduaelfr
#7 tvi-remove-notice-display-1107972-11.patch481 bytesduaelfr
#8 tvi-use-variable-table-1585350-7.patch4.04 KBduaelfr
tvi.query_.patch2.74 KBEugene Fidelin

Comments

Eugene Fidelin’s picture

Eugene Fidelin commented
Status: Active » Needs review
kingandy’s picture

kingandy
he/him
commented

+1 for this, these seem like the kind of persistent settings that would be appropriate for a variable.

The patch looks OK by eye but should really include a hook_uninstall in the .install file which removes all of the variables...

kingandy’s picture

kingandy
he/him
commented

... Actually, as soon as I posted that I started to rethink. If there's potentially one setting per term and vocabulary, that could be a lot of settings ... my instinct is that if you're storing data about an entity - even if it's metadata - it should go in a database table. I'm honestly not sure which way I'd call this.

Eugene Fidelin’s picture

Eugene Fidelin commented

I don't think that there will be a lot of tvi settings.

For most terms will be used the same view as for vocabulary, only for some special terms different view will be used.

To minify number of variables you need to delete variable if checkbox "Use view override. " is not checked while term or vocabulary editing.

In this case you will have variable only for vocabulary and terms that have different view.

charlie-s’s picture

charlie-s commented

Just stumbled upon this but it's my opinion that settings related to what this module provides are not needed "site wide" and thus should not be in the variable table. You mention that a reason for this is that variable_get is cheaper than db_query, but the entire variable table is queried and loaded on bootstrap, affecting (however slightly) the overall page load. The variable table is not meant to be a generic storage system.

Eugene Fidelin’s picture

Eugene Fidelin commented

Variable table is specially designed to allow modules save their setting in common way and not reinvent the wheel :)

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
StatusFileSize
new tvi-remove-notice-display-1107972-11.patch481 bytes

Eugene you are a king ! Thank you for this work :)
I also loved to see you transformed vocabulary ids in machine names which is often forgotten but really important.
However, your patch contains some coding standards issues that I fixed here [See #8 ;)].

I think the next step is to provide an integration with UUID module (I will open another issue for that)

The (very close) future of Drupal resides in Features-like integration. Storing configuration in a custom table without providing a common import/export API is really painful for people looking for an industrial way to develop, stage and deploy websites.

For those who care about performances issues :

  1. small websites with common use cases won't (or wouldn't) use the views by term functionnality
  2. huge websites use cache solutions like memcache for example so this is not an problem for them
  3. you may provide a patch in which term's specific configuration is stored into vocabulary's one (I would like to do this by myself but I have no time for now)

See #8 comment for the good patch file...

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
StatusFileSize
new tvi-use-variable-table-1585350-7.patch4.04 KB

Sorry, wrong patch file ^^

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
StatusFileSize
new tvi-use-variable-table-1585350-9.patch6.07 KB

New version !
I removed a mistake from the previous patch and added an update path !

Please review !

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
StatusFileSize
new tvi-use-variable-table-1585350-10.patch6.05 KB

Sometimes I might think twice before doing something ;)
New patch...

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
StatusFileSize
new tvi-uuid-variable-merge.patch6.45 KB

++ Clever merge with #1684928: UUID integration

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
StatusFileSize
new tvi-uuid-variable-merge-2.patch4.86 KB
new tvi-use-variable-table-1585350-12.patch3.42 KB

New version with upgrade path to replace vids by machine names
+ Clever merge patch rerolled

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
StatusFileSize
new tvi-uuid-variable-merge-3.patch4.86 KB
new tvi-use-variable-table-1585350-13.patch3.41 KB

13th is the good one ;)
Fixed upgrade path for vid -> machine names and schema -> variables

I won't change anything more ^^

Eugene Fidelin’s picture

Eugene Fidelin commented

Nice patches DuaelFr.

I think module owner must include them in dev release.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
StatusFileSize
new tvi-uuid-variable-merge-4.patch7.45 KB
new tvi-use-variable-table-1585350-15.patch6.97 KB

I lied :D
In this patch I added more consistency with other variables entries, using arrays instead of stdClasses. No upgrade path needed as old objects will continue to work and will be replaced by arrays at first save.
Again, it includes the merge diff to avoid collision with my other issue.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Status: Needs review » Closed (fixed)

This patch has been rerolled and push to the 7.x branch.
It will be available in the next dev release.