drop {flag_types} table; move bundle data to $flag->options [#1699660]

The {flag_types} table stores applicability of each flag to bundle types: eg, if flag foo applies to page and article nodes, {flag_types} represents this with two rows.

I'm not sure what having this as a table actually gives us:

1. extra database operations for creating an updating flags

2. causes multiple rows when loading flags that have to be dealt with:

    $query = db_select('flags', 'f');
    $query->leftJoin('flag_types', 'fn', 'fn.fid = f.fid');
    $result = $query
      ->fields('f', array('fid', 'content_type', 'name', 'title', 'global', 'options'))
      ->fields('fn', array('type'))
      ->execute();
    foreach ($result as $row) {
      if (!isset($flags[$row->name])) {
        $flags[$row->name] = flag_flag::factory_by_row($row);
      }
      else {
        $flags[$row->name]->types[] = $row->type;
      }
    }

The alternative would be to store the list of bundles in the $flag->options serialized array.

The only downside to this that I can see is that when a bundle is deleted, we either have to:

A. load all flags for the entity type in question and re-save them in order to take out the deleted bundle name. Downside is this makes the operation take longer, but I don't think (famous last words) that users will have so many flags on their system to make this unworkable.

B. Just ignore it and leave the obsolete bundle name in flags. It will be deleted anyway when the flag is next edited and saved in the UI. This is crufty, but not horrendously so. The only downside is that if the bundle is subsequently re-created, the flag will then once more apply to it. But maybe that's expected behaviour?

Any thoughts on this?

Comments

Comment #1

socketwench CreditAttribution: socketwench commented 8 August 2012 at 20:58

Write a patch that removes flag_types and then put both the patched an unpatched versions through performance testing?

Comment #2

joachim CreditAttribution: joachim commented 8 August 2012 at 21:04

I doubt it'll affect performance much, but I would expect one less join on flag load to speed things up very very slightly.

This is more about DX -- where data is stored, how it's loaded, how it's exported. If we eventually use CTools exportables for flags it will make it much easier to have all of a flag's definition in one table.

Comment #3

ezra-g CreditAttribution: ezra-g commented 10 August 2012 at 13:26

This decision should be made based on the measured performance implications of the current architecture and the proposed changes.

Comment #4

greggles

he/him

English

Denver, Colorado, USA

CreditAttribution: greggles commented 10 August 2012 at 14:18

The alternative would be to store the list of bundles in the $flag->options serialized array.

I can think of situations where this data storage technique has been a problem that got removed (e.g. #721436: Remove magical fairy saving of cruft from user_save()) but I don't know of any where we've moved *to* that technique. My biggest complaint: It makes it harder to write queries that look for data in the flag_types table in addition to joining something else. That may be an unlikely scenario, but I'd want to see some evidence of code complexity or performance benefit before losing a known benefit in terms of querying.

Comment #5

joachim CreditAttribution: joachim commented 10 August 2012 at 15:09

> My biggest complaint: It makes it harder to write queries that look for data in the flag_types table in addition to joining something else.

Perhaps I should have clarified before calling for comments: {flag_types} stores applicability of each flag to entity bundles, not entity types.

There's only one access of the {flag_types} table: when loading a flag object.

flag_link() only acts at the entity type level and leaves bundle applicability to the internals of the flag object -- the only check you need is $flag->access($entity_id).

Does d.org require the ability to bypass the flag API to discover which flags apply to which entity bundles?