Ability to add flag links to any view mode

Seems like a reasonable feature to add, and your suggested implementation sounds like the way to do it. If you were able to work on this that would be great!

There are a few complications from the fact that for nodes, the view modes are currently hardcoded as separate properties:

    $form['display']['show_on_teaser'] = array(
      '#type' => 'checkbox',
      '#title' => t('Display link on node teaser'),
      '#default_value' => $this->show_on_teaser,
    );

    $form['display']['show_on_page'] = array(
      '#type' => 'checkbox',
      '#title' => t('Display link on node page'),
      '#default_value' => $this->show_on_page,
    );

It should be simple to change that UI to a single checkbox and then a dependent set of checkboxes for the view modes. This should be done in the parent class where there's already 'show_on_entity'. The node-specific checkboxes in flag_node can then be removed.

However, we have the problem of converting people's existing flags to their settings are preserved. That's going mean adding to FlagUpdate_2 to handle this. It'll also mean that anyone running on 3.0 alphas will lose their current show_on_teaser/show_on_node setting, which is a shame, but I guess that's what alpha releases entail...

> the options for the non-standard view modes could be kept in their own options (maybe a single serialized one).

All the flag configuration options are serialized anyway. Once you define them in options(), saving them to the flag config is taken care of for you, as is unpacking them when the flag object is loaded from the DB.

> those two options could be kept as is to preserve backward compatibility

That's not really the Drupal way... :)
Better to break compatibility for a clean system that keep lots of legacy junk.

We currently have 300-odd users of the 3.x branch, which is not that many in the grand scheme of things.

> I'll try to submit a patch in the next few weeks.

Looking forward to it!
Feel free to ask any questions if you get stuck on anything :)

Tagging and upping priority. Let's make this a release blocker, as we need it in before release if it's getting in this time round!

Folding #1793756: rename uses_hook_link() into this.

Patch attached that does what joachim proposed in #1:

• Removed specific node and comment settings for view modes
• Removed uses_hook_link() implementation from comment and node flag classes (see below, generic implementation in entity class will suffice)
• Re-implemented 'show_on_entity' for both nodes and comments (checking this triggers the per-view mode settings)
• Added dynamic form options in entity flag class for view modes
• Reimplemented uses_hook_link() in entity flag class to use new settings
• Refactored flag_link() to accept $view_mode as a parameter instead of $teaser
• Refactored flag_bookmark module to use new settings
• Refactored tests to use new settings

Caveat: Tests fail because of an unrelated error:

Fatal error: Call to a member function fetch_roles() on a non-object in flag.test on line 274

Update: Apparently the tests pass without this patch, will look into it.

I've tested this quickly with nodes and profile2 profiles and it works as expected. Either way I think its a solid start! :)

Updated patch that fixes one of the 2 failing tests (still throws a notice) but the other still crashes and burns.

I'm going to need some help on this... I believe that the error has to do with how we're checking for properties in the saved flag object. Using debug() I found that the properties are actually set properly in the flag but checking $flag->$property doesn't work. This is likely due to how the flag view mode settings are stored (as an array) but then again I could be wrong.

Can someone advise or take it from here?

Thanks! :)

The $edit array that gets posted to the admin form is then massaged into data that can be compared with the saved flag:

    $saved = $edit;
    $saved['roles'] = array('flag' => array(2), 'unflag' => array(2));
    $saved['types'] = array('page');
    unset($saved['roles[flag][2]'], $saved['roles[unflag][2]'], $saved['types[article]'], $saved['types[page]']);

Hence these bits:

      'roles[flag][2]' => TRUE,
      'roles[unflag][2]' => TRUE,

become a proper array:

    $saved['roles'] = array('flag' => array(2), 'unflag' => array(2));

You'll need to do the same to 'show_on_view_modes'.

We'll also need code adding to the FlagUpdate_3 class, to convert the various old properties to the new ones.

Also, I am now wondering if we need the 'show_on_entity' property as well as 'show_on_view_modes'? It seems redundant to me -- what happens if you select 'show_on_entity' and no view modes?

@joachim, you are right regarding the last part about show on entity but not on view modes, maybe we can just have it as a dummy checkbox in the UI to show/hide the view mode options or better still we could just get rid of it entirely. please advise!

Thanks for the help with the tests, now I see it :)

Did the rest look ok?

Hopefully I'll be able to roll a new patch in a couple hours.

Taking the liberty since I'm knee-deep in this. if its not ok just unassign. New patch should be ready in a couple hours max.

Ok, as per #11:

• removed show_on_entity setting (view modes setting should suffice)
• fixed tests
• left show_on_profile intact because it changes the behavior specifically for user profiles; however users work just the same as other entities and can have extra settings for each additional view mode
• Added code in FlagUpdate_3 to convert 2.x flags to 3.x whilst preserving display settings for page and teaser view modes for nodes and show_on_comment for comments (users need no upgrade as show_on_profile was kept)

Note: I think its kinda confusing that we have a separate "show_on_profile" setting and behavior for user flags because the user can now chose between that one and the new per view mode "Show on User account view mode".

Regardless the patch works and tests should pass :)

Hmm I've been pondering about whether we need the 'show on entity' or not, as it occurred to me that on single-view mode entities (eg user) it'll look silly otherwise.

However, I think this is solved by making the view modes options a group of checkboxes like this:

    $options = array();
    foreach ($entity_info['view modes'] as $name => $view_mode) {
      $options[$name] = t('Display on @name view mode', array('@name' => $view_mode['label']));
    }
  
    $form['display']['show_on_view_modes'] = array(
      '#type' => 'checkboxes',
      '#title' => t('Display in entity links'),
      '#description' => t('Show the flag link with the other links on the entity.'),
      '#options' => $options,
      '#default_value' => $this->show_on_view_modes,
    );

Another thing I think should be changed is the property name 'show_on_view_modes'. I'd rather this was 'show_in_links'.

Other than that, I think this looks good.

Done! :)

I've just realized this needs a hook_update_N() to convert flags stored in the database. Sorry, should have realized sooner!

hmm good catch, I assumed that's what the FlagUpdate_3 bit was for, ok ill try to do this asap!

@joachim,

Ok, I finally got around to this! :)

• The patch adds an extra hook_update_N() to update flags from 2.x to 3.x
• A minor tweak was needed elsewhere: in the includes/flag/flag.entity.inc; I had to refactor the way we fill in default values for flag view modes because despite working everywhere and passing tests, etc with boolean settings, the #checkboxes element requires the value to be the same as the key name if its TRUE and to be 0 if its FALSE, so I gave it what it wanted. This is completely harmless.

Apologies for taking a couple days to get around to this and fingers crossed for the testbot! :)

+++ b/flag.install
@@ -652,3 +652,15 @@ function flag_update_7305() {
+    FlagUpdate_3::update($flag);

Sorry, that's not going to work... parts of FlagUpdate_3 have already been run on database flags at this point.

You do raise an interesting way of doing this in future, though! Although what's been happening on 3.x development is that each change to flag object structure has required a new hook_update_N() and a tweak to the FlagUpdate class, so they've not been in sync at all. It's not a very elegant system, but there's not much point in improving it, as I presume D8's config system will take care of all that.

+++ b/includes/flag.export.inc
@@ -340,5 +340,37 @@ class FlagUpdate_3 {
+    // Update show_on_entity property to use new view mode settings.
+    // This is actually trickier as it can be any entity type.
+    if (isset($flag->show_on_entity)) {
+      // Profile2 is an exception and a very common use-case.
+      if ($flag->entity_type === 'profile2') {
+        $flag->show_in_links['account'] = TRUE;
+      }
+      else {
+        // "Full" is the standard view mode for entities in general.
+        $flag->show_in_links['full'] = TRUE;

The current logic is that the link is shown on all view modes. So the most correct update procedure would be to enable it for all view modes.

There's no need for special handling, as entity_get_into() gives us all view modes for any entity.

> Sorry, that's not going to work... parts of FlagUpdate_3 have already been run on database flags at this point.

Hmm, I see, so do I just duplicate the logic there in the update hook?

> You do raise an interesting way of doing this in future, though!

Hah, that's what I thought that was class was there for...

@joachim, can you please elaborate so I can get this done! :) Thanks!

> Hmm, I see, so do I just duplicate the logic there in the update hook?

Yup, pretty much.

Load all flags, go through them all, tweak the properties structure, save each flag again.

Done deal, thanks! Will get to this in a few hours hopefully!

Ok, new patch does 2 things:

• Updates FlagUpdate_3::update() to address comments from #21
• Updated flag_update_7306() to address comments from #24

Hopefully this should be good to go! :)

Hmm I had a flag I was pretty sure was set to show on nodes for full and teaser, but after running the update 'Display on Full content view mode' wasn't enabled.

I'll wipe by database and have another go at it tomorrow.

sounds good, I'm pretty sure it works here :)

+++ b/flag.install
@@ -652,3 +652,42 @@ function flag_update_7305() {
+    if (isset($flag->show_on_page)) {
+      $flag->show_in_links['page'] = TRUE;
+      unset($flag->show_on_page);
+    }

The 'page' view mode for nodes is actually called 'full', same as other entities. This is what wasn't working in my update.

+++ b/flag.install
@@ -652,3 +652,42 @@ function flag_update_7305() {
+    if (isset($flag->show_on_comment)) {
+      $flag->show_in_links['full'] = TRUE;
+      unset($flag->show_on_comment);
+    }

This is actually causing extra view modes to lose the flag. In my case, the view mode provided by Diff module, which is possibly undesirable anyway, but other modules might provide view modes we don't know about - DS, for instance.

Could you check these are right in the update class as well please? (It's a pain to have to have it in two places, I know :( -- should all go away for D8!)

> This is actually causing extra view modes to lose the flag. In my case, the view mode provided by Diff module, which is possibly undesirable anyway, but other modules might provide view modes we don't know about - DS, for instance.

I'm not sure what you mean here? What did you want me to do with the show_on_comment block? Get rid of it?

It needs the same processing as show_on_entity, so probably lump it in:

if (show_on_entity || show_on_comment) {
  ...
}

Done, fixed in .install and copied changes over the the update class.

Still doesn't work.

Try chucking this at the end of flag_form():

  dsm($flag);
  
  // Update show_on_teaser property to use new view mode settings.
  if (isset($flag->show_on_teaser)) {
    $flag->show_in_links['teaser'] = TRUE;
    unset($flag->show_on_teaser);
  }

  // Update show_on_page property to use new view mode settings.
  if (isset($flag->show_on_page)) {
    $flag->show_in_links['full'] = TRUE;
    unset($flag->show_on_page);
  }

  // Update show_on_comment and show_on_entity properties to use new view
  // mode settings. Since the old logic was to show on all view modes, do that.
  if (isset($flag->show_on_entity) || isset($flag->show_on_comment)) {
    if ($entity_info = entity_get_info($flag->entity_type)) {
      foreach ($entity_info['view modes'] as $view_mode => $value) {
        $flag->show_in_links[$view_mode] = TRUE;
      }
    }

    unset($flag->show_on_entity, $flag->show_on_comment);
  }
  dsm($flag);
  

The problem the use of isset(). These should be !empty, as the flag object's options() sets the property to FALSE if it's not set when loading the flag.

Hi alexweber!
Do you still have time to work on this to get it ready, or should I unassign you? I think it's nearly there! :)

Hi joachim, sorry I've been kinda busy lately. I'll work on this the next few days, thanks!

@joachim, I'm sorry after like a billion patches it doesn't look like I'm going to be able to drive this one home due to a severe lack of time. Apologies for the hold up...

No problem.

You've done a ton of work on this one and lots of other issues on flag too -- thank you very much for all your help!

My pleasure! :)

So what *does* need to be done? My manual testing has revealed no issues...

See #33.

I think you've misunderstood that comment!

#33 is debug code that shows the problem. The actual problem is explained at the end of that comment:

> The problem the use of isset(). These should be !empty, as the flag object's options() sets the property to FALSE if it's not set when loading the flag.

Those need changing in 2 places: the hook_update_N(), and the flag export converter.

@joachim:
Which is currently the working patch? #32?

Yup. Ignore #41. #32 needs just a little bit of extra work.

Going to do babysteps as per feedback suggested in #33.

Hoping for feedback please. Would this be the right direction ?

That looks right. The same change needs to be made in the update class, FlagUpdate_3.

Thanks for tackling this! :)

@Joachim: No problem. Thanks for the quick and clear feedback. I think this should take care of the other changes as you requested.

Let's see if testbot likes it.

Also, that should be it as a grep shows no results:

$ grep -nr --exclude='interdiff.txt' 'isset(\$flag->show_on' .

Patch seems to work on the settings form. But i must be missing something I can't figure out how to add flag markers fields on the different view modes.

You should have a set of checkboxes for the view modes on your flag settings form, IIRC.

I thought i did check that box and enabled those settings you speak of.

I spawned a simplytest.me instance with this patch. You can see the screenshots and details of my findings:

I created a flag added a field to the flag (a boolean) and checked and enabled all the view modes (because one is not enough :) )

This allowed me to enable flags on the field page

However, there is flag on the view mode :(

This patch is still keeping all the flags output in the entity links.

If you want flag output as pseudofields so you can order them on the field admin UI, there's a follow-up issue for that!

@joachim: Ah I see. Thanks. Well then the patch works perfectly.

Found the followup issue: #1892930: Placement of flag links as pseudofields

Updated issue summary.

I've tested the upgrade path with flags on:

- node full page only
- node teaser only
- node both full page and teaser
- comment

All settings upgrade as expected.

Committed and pushed. Thanks to everyone who's helped with this, and especially huge huge thanks to alexweber for the tremendous amount of work he did on this patch!

git commit -m "Issue #1871426 by alexweber, chrisjlee: Added ability to set display of flag links on specific view modes." --author="alexweber "

Changed record filed: http://drupal.org/node/1990790

@joachim I only just saw this, awesome! Thanks for the props and glad the patch made it in!!

Updated issue summary.