This is a followup request to #1965040: change handling of return values from hook_flag_access() in order to support integration with the OG module (via the og_flag module).

One of the barriers to fully implementing Organic Groups support is the need to apply group specific flag overrides early enough in the processing cycle so that the flag module itself and other contrib modules can react to the group specific overrides rather than the default system wide flag definition.

The attached patch adds a new hook to flag.module: hook_flag_get_flags().

This hook allows other modules to modify the list of available flags before they are filtered and returned to the calling functions. By making modifications to the flags at this point, I can avoid having to duplicate and potentially override other logic already existing in the flag module and other contrib modules. The OG Integration only allows group administrators to change existing settings such as the display text or permissions for a flag. It doesn't provide any new functionality.

The attached patch works on both the 2.x and 3.x branches of the flag module. Ideally this update would be incorporated into both branches.

Files: 
CommentFileSizeAuthor
#6 1973406-og_flag.patch482 bytesshawn_smiley
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]
#6 1973406-init_callback.patch746 bytesshawn_smiley
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]
hook_flag_get_flags.patch1.32 KBshawn_smiley
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]

Comments

shawn_smiley’s picture

Status:Active» Needs review
joachim’s picture

Status:Needs review» Postponed (maintainer needs more info)

Can you not use hook_flag_alter()?

shawn_smiley’s picture

Status:Postponed (maintainer needs more info)» Needs review

My original implementation used hook_flag_alter() and it worked well as long as all entities processed during the request belong to the same group.

Here is a use case where using hook_flag_alter() doesn't work.

Take the case of where you're either using the default Drupal home page or you have a view that renders nodes and displays content from multiple groups. Now say we have 2 groups defined. Group 1 overrides the text of the bookmark flag to say "Save Me" rather than the default value. Group 2 sticks with the default implementation.

Since the result of hook_flag_alter() is statically cached by flag_get_flags(), the hook is only executed once per page request. So the first rendering a node from Group 1 correctly applies the overrides. However the processing for the 2nd node never has a chance to modify the flag object. So the nodes for Group 2 also get the overrides from Group 1.

Implementing a hook that fires after the static caching of hook_flag_alter() in flag_get_flags() seemed like the best solution. Though I'm open for other suggestions. The main issue I have is that I need to potentially modify the flag object on an entity by entity basis before any of the other logic in the Flag module processes it.

Another path I considered, but couldn't find a way to make work, was have OGs that override the flag actually create a new flag object that applies only to that group. So instead of Group 1 loading flag "bookmark", when they override that flag we would create a new flag named og1_bookmark that got loaded instead of the default "bookmark" flag.

joachim’s picture

You could force the static cache in flag_get_flags() to clear.

I do think though that wanting the same flag handler to behave in different ways depending on context is a bit weird and bending Flag beyond what it's meant to do...

> The OG Integration only allows group administrators to change existing settings such as the display text or permissions for a flag.

Is there nowhere this can be altered later on?

shawn_smiley’s picture

Status:Needs review» Active

Resetting the static cache is an interesting idea. Let me give that a try.

I don't think this could be done any later without trying to override and rewrite all of the access check and rendering routines of the Flag module and any other contrib modules. For example, within flag_get_flags() the modification needs to occur before the $flag->user_access() call at the end of the function. If resetting the static cache works, then I should be able to use the existing hook_flag_alter() implementation.

Thanks for the alternate suggestion. :-)

shawn_smiley’s picture

Status:Needs review» Active
StatusFileSize
new746 bytes
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]
new482 bytes
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]

I've found another solution which works much better than trying to clear the static variables.

I was able to inherit from and override the base flag classes from the og_flag module and implement all of the customizations in those overridden classes. The only thing I need to make this fully functional is to have the flag_get_flags() method invoke a method on each Flag object during each request.

The attached patch (1973406-og_flag.patch) implements this direct og_flag support.

A more generic variation of the above would be to add an extra parameter to the flag_type_info() array that defines an additional initialization callback method on the flag. Then the flag_get_flags() method could call that callback on each flag object if it is defined. However, that would add some extra processing for everyone not just sites that implement the og_flag module. I've also attached a patch showing this alternative (1973406-init_callback.patch).

The important thing for the og_flag module is that there is some trigger call after the flag has been loaded from the database/static cache but before any filtering/processing has occurred. I can't use the hook_flag_alter() because that alters the flag in the static cache.

shawn_smiley’s picture

Status:Active» Needs review
joachim’s picture

Status:Active» Needs work

If a custom flag class works for you, then I think you can do this entirely from OG.

Every time a flag handler is instantiated, its construct() method is called -- so you can do whatever you need there. (The chain of functions is a bit confusing, but documented at the top of the flag_flag class.)

shawn_smiley’s picture

Thanks. Providing my own class implementations for each of the flag types (entity, node, user, comment) and tieing into the contruct() method gets me 98% of the way to a solution.

However, the above only gets called once per page request. The results of the construct() method and object instantiation get statically cached (as they should) so I can't use that to perform any entity specific context switching. I still need a trigger/notification every time flag_get_flags() is called not just when the flags are loaded into memory.

Currently I'm distributing the og_flag module with the patch 1973406-og_flag.patch in #6 above. (though I've moved that execution point to just before the return statement)

joachim’s picture

If you're now using your own Flag handler class, can't you do whatever you want inside it when it comes to access, labels, etc etc?

shawn_smiley’s picture

Status:Needs work» Closed (won't fix)

Thanks joachim,

It took some tracing through the object structure, but I was able to find a way to implement the og_flag module without patching the flag module. :-)

Marking this issue as closed.

joachim’s picture

Glad to hear you got it to work.

If there are any improvements you'd like to make to the documentation of the various functions/methods/classes you trawled through to make them easier to understand, do please file a patch :)

joachim’s picture

Issue summary:View changes

Fixed typo in related issue number.