Implement hook_flag_default_flags

boy, wish we had a better way to flag this issue ;) +1

Here's a re-roll against dev and from within the flag module's directory.

I'm not sure why this code is part of this patch:

+  // Give other modules a chance to change defaults using 
+  // hook_default_flags_alter()
+  drupal_alter('default_flags_alter', $default_flags);

In any case, it would be drupal_alter('flag_default_flags', $default_flags);, otherwise the hook would be called hook_default_flags_alter_alter(), when we want it to be hook_flag_default_flags_alter(). This change should be broken out into a separate patch either way, not bundled in this one.

Wouldn't the need for the hook be fixed by #435822: Implement hook_flag_alter? I think that it affects default flags as well as database driven ones (not positive, it's been a while since I reviewed it).

The hook is still named incorrect in #4. It should be drupal_alter('flag_default_flags', $default_flags); (leave off the trailing '_alter', Drupal adds that for you ).

I'm a bit confused by this, according to this documentation page, there's already a hook_flag_default_flags ? or is that doc page wrong there?

There is a hook_flag_default_flags(), which allows the creation of new flags. This issue is sort of a muddle between actually making flag.module implement it's own hook and also adding hook_flag_default_flags_alter(). The _alter() part should probably be dropped since you will be able to accomplish that with #435822: Implement hook_flag_alter. I'd prefer not to add two hooks when one will do just fine.

ow right ok, i see... I'm very glad that #435822: Implement hook_flag_alter is geting worked on, I was last night hammering my head against the wall trying to store some other configuration options for the flag in a module... can't wait for that to happen- flag ftw!

ps. thanks for the explanation =)

# Provides no way of knowing if you're altering a default or normal flag.

Flag works a bit differently in that the distinguishment between "default" and "database" is very blurry. Unlike Views that clearly has code-based views and database-based views, even "default" flags are loaded into the database to give them a FID. So a code-based Flag really is nothing but a database flag with a few items that have been "locked" by code. So therefor, the ability to distinguish whether flag is a "default" is not actually that important, but you can easily tell which flags have locked properties by checking !empty($flag->locked). However if we want to make code-based flags easy to identify, it'd be pretty easy to make $flag->default = TRUE.

# Makes it harder for site builders to understand why flags aren't behaving as described in the UI.

I expect that any altering that is done would probably want to set $flag->locked['property_name'] so that it would become locked in the UI. This can affect database and code-based flags equally.

# Forces module developers to resort to weighting modules to ensure they're behavior isn't overridden.

If 2 hooks were added (both default and normal flag altering), the general flag_alter would be run second anyway, so providing a default_flag_alter() doesn't guarantee anything would not be overridden by the second hook.

So I'm still largely opposed to making 2 hooks when one will do. While Flag uses Views as a model for a lot of things, our implementation of "default" flags is not one that's entirely similar. Though I could be entirely wrong, I can't see a reason why it's needed.

The appeal of a default only alter is that it gives a developer access to make existing flags compatible or to disable them.

It would be handy to be able to "disable" a flag entirely if needed, but because flags are loaded into the database, removing a flag through hook_default_flags_alter() would not actually disable it in any way, since it will also be in the database (though it would "unlock" all it's properties and the user could delete it if desired).

I realized that we already have a $flag->default property: You can just check $flag->module, if it's set then the flag is being provided by that module.

function mymodule_flag_alter(&$flag) {
  if (isset($flag->module)) {
    // Do something to the flag.
  }
}

I've stated above several reasons why we don't need another hook. Unless a situation can be raised (and a practical usage) for something that cannot be done with the current hooks then another hook will not be created.

My understanding is that this issue was "won't fix"-ed because there is no need for an extra _alter hook. However, the title and the OP are still valid concerns.

I came across this issue while trying to understand why flag_get_default_flags() wasn't returning 'bookmarks'. I considered opening a separate issue, but I didn't want to have two issues with identical titles.

The attached patch is similar to the others, but has been updated to 6.x-2.x-dev. The only real changes were removing the now unnecessary flag_enable(), and adding 'api_version' => 2, to prevent flag_update_page() from being called.

The unrelated _alter hook has been removed.

The "bookmarks" flag is intended to be a temporary flag, not a permanent one. I think it would be common that a user would want to export the bookmarks flag (after modification) into their own module or feature. If Flag is already providing the bookmarks flag, then you'd have 2 modules competing for the same namespace. Flag intentionally provides this a "starter" flag that you can easily delete or modify in any way you want. It's intended to be an example, rather than an integral part of the module or something you can count on being there.

Well, that makes sense.