Allow modules to expose default flags

I was wondering about this myself, but just ended up doing an INSERT in my .install the way flag.install inserts the default 'bookmark' flag. Not ideal, but works for me for now.

I'm also really interested in this, though I think it'll be post 1.0 version (which hopefully will happen soon).

Hah, the flag "fid" exists mostly for speed of JOINs internally in Flag module. Views module uses the same technique for speeding up retrieval of view information. The fid also makes it possible to rename the flag "name" without breaking all existing flaggings, so it's not going anywhere.

So we'll need to figure out some way of assigning default flag a flag ID, the same way Views does. We'll probably need to compare the flags in the database to those provided by default flags and make sure that any database settings override whatever is provided by the default flag by the same name.

Also, flag_get_flags() *should* be keyed by flag name, not flag ID. It's been long standing on my TODO list to switch this around, since we nearly always use the flag name instead of ID (except for JOINs of course, as I mentioned). flag_get_flag() even defaults to using name, but it requires doing a loop over all the flags to find the right name.

Lastly, I don't think the content types should be locked by default flags. If a module needs to specifically lock-down one type (like project module), it can do a form_alter() and specifically disable that single type.

BTW, you're using FAPI's #disabled. In D6 (and D7?) this is destructive.

+1

Other issue has been fixed, setting active again. What's next?

Sun's patch in #4 is a good start. Basically I think we need a reroll at this point, now that we're keying off of 'name', it should be pretty straight forward.

We also need to assign default flags an FID. Because FIDs are the keys used for joining within the flag tables, you won't be able to flag anything until one is assigned. So we'll need to do some kind of check against the database to see what the flag's FID is. And merge in the database override values with whatever was provided by the "default flag".

I've been looking into merlin's work on Views to see how he's done this. Some pretty interesting stuff. Incidentally, he informed me that he's currently building a framework for this exact task: http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/ctools/incl...

But I'm not keen on forming a dependency for this functionality. But it could be a great reference.

In regards to the Views approach. Here's how things generally work:

- All database views are loaded from the database
- All default views are loaded from code (module_invoke_all())
- Default views come with a "disabled" property, defined in code. There is no "disabled" property for database views.
- If a database view exists with the same name as the default view, the default view is essentially discarded and not used. Though a property "type" is assigned an "overridden" value on the database version of the view, so we know it is derived from a default.
- If a database view does not exist for a default, the view is given a "type" property of "default".
- The combined list of views is returned.

Unfortunately this has made it clear to me that Views doesn't need to assign a vid to default views, because views don't have any arbitrary data attached to them like flags do. In Flag, we'll absolutely need an FID in order to actually flag things (since FID is needed to record the flagging). Views only needed the VID to property retrieve all the configuration of a view, which is entirely provided by code in the case of a default.

So... basically, we won't be able to use the exact same model as Views. More thinking yet to be done.

We also need to assign default flags an FID

It's easy: we sould save all the default flags.

Pseudo code:

function flag_get_flags(...) {

   static $flags = array();

   // Step 1: Load flags from the DB:

   $flags = SELECT * FROM {flags}

   // Step 2: Merge the default flags into $flags, and make sure
   // they also exist in the DB:

   $default_flags = module_invoke_all('flag_default_flags');
   foreach ($default_flags as $flag_name => $flag) {
     if (!isset($flags[$flag_name]) {
        // This flag is not in the DB already, so put it there.
        $flag->save();
        $flags[$flag->name] = $flag;
     }
     else {
        // This flag was already loaded from DB, so there's nothing
        // to do (except perhaps sync the fields in it that are locked
        // in the settings form).
     }
   }
  
}

(BTW, $flag->save() sets $flag->fid if it's missing.)

(Note that default flags sooner or later get to the DB anyway (that's how a user is able to override some settings of them --by submitting the settings form).)

Alternative: Don't allow to use default
flags directly; "install" them by enabling from a "also available" list.
[...]
The site admin chooses which one she wants.

That's an interesting proposal.

1. Should modules be able to have their exposed flags enabled by default? If so, we'll have to do my proposed $flag->save() anyway (for these flags).

2. It also means that programmers won't be able to rely on their exposed flag always being enabled. (It's different than default views, where a disabled one "hurts" nobody.)

3. I think we'll want to split our overview page to two sections anyway ("your flags" and "module flags").

Thanks mooffie, I've been thinking along similar lines. Merlin kept telling me that we'd run into problems when modules update default views, which might be true, but after some analysis, I don't think it's likely that there will be much that the module can provide anyway.

So here's what I'm thinking to handle this entirely:

Default flag declarations:
- Add a "status" column for flags in the DB, all default flags are immediately saved to the DB (but can be off by default by setting $flag->status = FALSE).
- Default flags can lock certain properties, these properties cannot be edited by the end user, and the code version always takes precedence. The module providing the default flag adds the special property $flag->locked, which is an array of locked properties on the flag.

function mymodule_flag_default_flags() {
  $flags = array();
  $flags[] = (object) array(
    'content_type' => 'node',
    'name' => 'starred_issues',
    'title' => 'Starred issues',
    'roles' => array(
      0 => '2',
    ),
    'global' => FALSE,
    'types' => array(
      0 => 'project_issue',
    ),
    // etc.
    'locked' => array(
      'name',
      'global',
      'roles',
      'show_on_form',
    ),
    'status' => TRUE,
  );
}

So in this example, users could override everything but name, global, roles, and the "show on node form" display option.

Workflow for loading flags:
- Load all flags from DB
- Load all flags from hook_flag_default_flags().
- Add any new default flags to the DB.
- Replace any "locked" properties from the default flags in the final list.

Deleting a default flag sets 'status' column to 0 in the database, since you can't actually delete the code (unless you disable the module providing the default). Deleting a database view actually removes the database entry entirely.

Ah sorry sun, I should have specified, any default flag that has $flag->status = FALSE would not need to be loaded into the database at all. However, ones with $flag->status = TRUE will need to be loaded into the DB immediately to give them an FID.

Interestingly, I noticed that Views does not have a "status" column in the database at all. Instead it just maintains a single variable of "disabled views". Note in Views you also can't "disable" a database View, you can only delete it. So that's an interesting solution that doesn't require another database column and keeps disabled flags out of the DB entirely (which I think would be a great thing).

If a user uses/overrides a default flag, I don't think we should allow her to alter the default values provided by the module in any way.

I think there a lot of reasons to allow users to alter a lot of the default values. Especially things like the "marked message" and most definitely the roles that are allowed access. I'm going to go ahead and take a stab at updating the patch and we'll see if any roadblocks come up.

Hm, sorry this patch is on 6.x (it'll be backported of course), but this version is 100% functional.

Implementation details:
- Disabled flags are stored in a single variable "flag_disabled_flags".
- Disabled flags are not stored in the database at all, other than the above variable.
- flag_get_flag() can load a disabled default flag (used for enabling a default flag), but flag_get_flags() never includes disabled default flags.
- A few new API functions, flag_get_default_flags(), $flag->enable(), and $flag->disable().
- Database flags cannot be disabled, only deleted (same as current implementation).

factory() is becoming ugly, isn't it? Here's a possible remedy: #333753: Have factory_by_row() and factory_by_array()

- flag_get_flag() can load a disabled default flag (used for enabling a default flag)

It returns disabled flags. It means that a disabled flag could be used, doesn't it? (weird things would happen, though). Perhaps it would be better to make flag_get_flag() return NULL for disbaled flags, so that code using a disabled flag will fail completely instead of partially. (To get at a disabled flag, maybe we could have an enforcing third argument for this flag_get_flag() ? )

(Oh, all in all, your code is very nice! I think it's a very important feature.)

Here's an updated patch that applies with all the other changes that have been happening recently.

- flag_get_flag() no longer returns default flags, since it was an exception when "enabling" a default flag that we used this anyway. Now the default flag is simply loaded by flag_form() if needed.
- I changed the variable "flag_disabled_flags" to "flag_default_flag_status" to record both manually enabled and manually disabled default flags, since I found it was difficult to determine if a flag that was marked disabled by the code version was enabled by the administrator.
- Various bug fixes where I had missed checking the the $flag->locked variable for some fields.

I'd like to test this better, (applies just fine) but am unsure how to formulate a default flag. Can I get a working example to test?

Also, this brings up the need for an exporter of flags. I'll open up a separate feature request as an offshoot of this one.

Ah yes, here's default flag definition (it's now an array):

function mymodule_flag_default_flags() {
  $flags = array();
  $flags[] = array(
    'content_type' => 'node',
    'name' => 'starred_issues2',
    'title' => 'Starred issues',
    'roles' => array(
      0 => '2',
    ),
    'global' => FALSE,
    'types' => array(
      'album',
      'webform',
    ),
    'flag_short' => 'Star',
    'flag_long' => 'Add this issue to your list of starred issues',
    'flag_message' => 'Added to your starred issues.',
    'unflag_short' => 'Unstar',
    'unflag_long' => 'Remove this issue from your list of starred issues',
    'unflag_message' => 'Removed from your starred issues.',
    'show_on_page' => TRUE,
    'show_on_teaser' => TRUE,
    'show_on_form' => FALSE,
    'status' => FALSE,
    'locked' => array('show_on_teaser', 'name', 'types', 'roles', 'global', 'types'),
  );
  return $flags;
}

Once we have this feature, I think it makes sense to make the starter "bookmarks" type a default flag rather than a purely database one, especially if we go with #333984: Remove Dynamic Default Views.

Thanks Nate.

I tried this out by simply appending the function to the flag module itself (after applying the patch of course) and renaming it accordingly to flag_flag_default_flags().

Looking at the flag listing page (/admin/build/flags) I see the specified flag under disabled flags. I think this kind of suggests that there should be a way to disable currently existing flags. (feature request? part of #335453: Import/Export Support?)

Module column is empty. I see the relevance to listing this, but I wonder if it would be preferable to have a more uniform listing of information that corresponds to the 'Enabled' flags.

I also see that it specifies content types that I do not have. Noted that 'types' is part of the 'locked' property... twice... not sure why that is. Therefor this flag would only be good to an installation that contained these types, and I have no way of using this as a sort of 'template' for similar functionality. (Not sure if that would be feature creep.) I do see the merits for not wanting certain flags, utilized by particular modules to be used in a way other then intended since that other module might be operating on this flag in a particular way. Things could get out of hand quickly. So this is not a bug. (just writing my line of thinking as i go here)

If everything I described so far is the intended functionality, then RTBC for 6.x

Patch does not apply for D5 (where I happen to need it). If no one volunteers I'll try to rework the patch for D5 as well this week.

P.S. Properties become enabled as you remove them from the 'locked' index just fine.

Looking at the flag listing page (/admin/build/flags) I see the specified flag under disabled flags. I think this kind of suggests that there should be a way to disable currently existing flags.

Similar to Views, you can't "disable" database Flags. You can however disable default flags provided by modules, though they will be reset to the module defaults. So it's not really "disabling" the default flag, since all changes are lost. So I kept the operation as "delete" even when you're disabling a default flag.

Module column is empty.

Hm, that should list the module that provided the flag. Rather odd.

I see the relevance to listing this, but I wonder if it would be preferable to have a more uniform listing of information that corresponds to the 'Enabled' flags.

Listing information such as what types it applies to or global doesn't necessarily mean a lot for a disabled flag (since it's not currently doing anything). Given that a user can change these values when they first enable the flag, I didn't think it worth including in the list of flags.

Noted that 'types' is part of the 'locked' property... twice... not sure why that is. Therefor this flag would only be good to an installation that contained these types, and I have no way of using this as a sort of 'template' for similar functionality.

Sorry didn't mean to add "types" twice. The point illustrated in the example is that you can lock/unlock any property you want. In the case of locking the "types" property, it would makes sense in the case that sun is trying to create, where he wants project module (which hard-codes the "project_project" type) to have a flag explicitly for use with the type it provides.

All makes perfect sense :) Thanks Nate! Should we wait for a D5 patch before marking RTBC?

There's was minor conflict due to #335319: PHP notice:Undefined index. This reroll fixes the conflict (one less line to change, since that other issue fixed the problem there). Seems like it applies to Drupal 5 already without any changes.

This also fixes the blank column problem. I was trying to use node_get_types() for some reason. :P There isn't an easy way to get the human readable name of a module, so this just uses the machine name in that column.

Heh, might be good to get rid of the sample implementation. Here we are again.

Patch applied to D5 dev, and although it doesn't look like the patch itself removes this functionality, the flag link types are not showing up now on default or even older flag in the system that had them. I would assume this should be compatible with #160519: Allow admin to select Ajax or non-ajax behaviour per Bookmark type.

Thanks sirkitree, you're right this patch as inadvertently "locking" the link_type unless it was specified as "locked" (essentially it was doing the opposite due to an isset() being in the place of an empty()). Fixed in this version. I also found that checkboxes don't respond well to having #access set to FALSE. If using an associative array for values Drupal sets the return value to 1 instead of whatever the given value was.

I discovered this by creating a default flag, enabling it in the UI, then renaming the default flag in code. The copy saved to the database had corrupted access and type data. So this patch now gets around the problem by manually setting #value for checkboxes. It's unfortunate it's needed, but it solves the problem.

So sexy, works perfectly!

Only question is: how to define the link type in that array structure?

It should be a straight conversion from the flag object:

$flags[] = array(
  'link_type' => 'confirm',
  'locked' => array('link_type'),
);

Of course this makes clear the need for #335453: Import/Export Support once we put this in.

Oops... testing broke again... now I can't see any content types when editing/enabling a default flag.

scratch that! my bad! I had the incorrect machine name for the content type in 'types'

Now committed to Drupal 5 and 6 branches:

http://drupal.org/cvs?commit=155241
http://drupal.org/cvs?commit=155240

Nathan, thanks for this nice feature!