Placement of flag links as pseudofields

Thanks for contributing your work on this.

However, I don't think this is the right approach: firstly, using a weight of -50/+50 may cause problems, as FieldAPI can probably put things outside of that range. More generally though, if we're going to put a flag link at the top level in the content array, then we should expose it as a pseudofield to allow the admin user to apply any ordering in the display fields admin UI.

I actually have been doing a bit of work myself towards that, though it needs to wait for #1871426: Ability to add flag links to any view mode.

Also, the flag link will need some sort of wrapping block-level element (P or DIV), otherwise it'll get formatted weirdly. That will require some replumbing of the flag theme system.

I was thinking maybe refactor the flag placement to use hook_field_extra_fields() + hook_entity_view() but in that case I think the approach here: #1871426: Ability to add flag links to any view mode might be obsolete or redundant... thoughts?

> use hook_field_extra_fields() + hook_entity_view()

Yup, that's what I had in mind here.

However, this approach shouldn't replace what we currently have, because it produces fundamentally different output:

- you output each flag individually
- each flag is a single content element, rather than within the 'element links'.

It also has the downside compared to the entity links system that the field weights are stored in the variables table, and so are a pain to export, and aren't exported as part of the flag even if you do export them.

For entities that don't have a concept of 'entity links' (everything other than node and comment), this approach is maybe what most site builders would want, but for nodes and comments it really depends on the content.

So I think that adding this functionality means that it sits alongside the 'show in links' functionality, as just a new way to output flags.

Good points... so what should we call it? "Show as field"?

Setting version, and postponed on #1871426: Ability to add flag links to any view mode.

It would be great to get this into 3.x, but we do have to think about getting a stable 3.0 out so we can start working on 8.x!

I should really post the work I have for this so far, though the patch will be broken by the other issue that blocks this.

Tagging

Here's my work in progress.

Needs a bit more doing, in particular the flag link needs to be wrapped in something so it doesn't look dreadful. Input from themers would be very welcome!

Will also need a reroll when #1871426: Ability to add flag links to any view mode lands.

Applied #10

+++ b/flag.moduleundefined
@@ -921,6 +931,20 @@ function flag_entity_view($entity, $type, $view_mode, $langcode) {
+  // Get all possible flags for this entity type.
+  $flags = flag_get_flags($type);
+  dsm($entity);
+  foreach ($flags as $flag) {
+    $entity_id = $flag->get_entity_id($entity);
+    if ($flag->show_as_field) {
+      $entity->content['flag-' . $flag->name] = array(
+        // ARGH needs extra wrapping!
+       '#markup' => $flag->theme($flag->is_flagged($entity_id) ? 'unflag' : 'flag', $entity_id),
+      );
+    }
+  }

Wrap the flags in a span or p tag. Module should provide minimum markup as it's first option. If other people want to add more markup; they should be able to overrride it with a template file (at best) or a theme function (at worst); given if that's even possible.

And also, a small nitpick. Would be nice if you commented out the dsm. But obviously this is a WIP patch.

Would mark to needs work. But should stay at postponed right now! :)

> If other people want to add more markup; they should be able to overrride it with a template file (at best) or a theme function (at worst); given if that's even possible.

+      $entity->content['flag-' . $flag->name] = array(
+        // ARGH needs extra wrapping!
+       '#markup' => $flag->theme($flag->is_flagged($entity_id) ? 'unflag' : 'flag', $entity_id),
+      );
+    }

I think there are two options here:

a). add a new theme function or template for this. This would be a mere wrapper around theme_flag(), and therefore a bit pointless and silly
b) add an option to theme_flag() for whether to wrap the whole thing in a P.

Well then b it is.

Added link to meta issue.

Updated patch, rerolled for HEAD now that #1871426: Ability to add flag links to any view mode is in.

Does anyone care to tackle the wrapping part? Would be great to get a themer to look at this.

On IRC:

> chrisjlee: joachim_: right. I'm just curious how you see one would configure the wrapper
> chrisjlee: joachim_: would it be simply a checkbox in the settings?

I don't think we need settings here.

What we have is currently a single point of entry for theming a flag link. We should retain that -- adding a different theme template or a wrapper would be crazy.

So:

- most of the time, the flag link is fine as it is
- when it's output as a pseudofield, it needs some kind of block HTML element around it such as a DIV, otherwise the formatting is dreadful (and possibly non-semantic)

So what I suggest is:

- an extra item in the $variables passed to flag.tpl.php such as 'needs_wrapping_element'
- some conditionals in flag.tpl.php to optionally wrap the output in a DIV
- on the pseudofield, pass in needs_wrapping_element = TRUE

@joachim Passed 'needs_wrapping_element' to variables array

I guess i was wrong. Not sure if wrapping it with a <p> is necessary. Flag seems ok without any wrapper IMHO. Patch attached anyways.

I think i misunderstood you. So this is probably what you were looking for.

- Changed the wrapper to be a block level element

I need to figure out how to remove the span that wraps the p. I think it's in the theme function. If anyone can advise that'd be great!

I don't think your 'after' screenshot is of the right thing -- the presence of the 'readmore' link and it being in an LI suggest that's 'classic Drupal' node links, not a pseudofield.

@joachim Sorry, you're right. I need to figure out what a psuedofield is.

+++ b/theme/flag.tpl.php
@@ -27,6 +27,8 @@
+ * - $needs_wrapping_element: By default is true. Determines if flag displays a
+ *   wrapping element or not.

Making the wrapping TRUE by default isn't going to work -- we need to extend the template for this new special case, while leaving it unchanged for everything else. Hence the default has to be FALSE.

+++ b/theme/flag.tpl.php
@@ -37,7 +39,9 @@
-<span class="<?php print $flag_wrapper_classes; ?>">
+<?php if ($needs_wrapping_element) : ?>
+<p class="<?php print $flag_wrapper_classes; ?>">
+<?php endif; ?>

The current wrapping classes have to be kept as a SPAN, as all the JS expects that, and also they are shown inline in the entity links.

Here's an updated patch with that fixed & some other things fixed too.

+++ b/includes/flag/flag_flag.inc
@@ -1430,12 +1430,28 @@ class flag_flag {
+   * @param $variables = array()
+   *  An array of further variables to pass to theme('flag'). For parameters
+   *  documentation, see flag.tpl.php

As a follow-up issue, I'll remove the $errors and $after_flagging parameters to simplify this, now that we have a $variables.

Patch seems to work fine. Patch modifies and allows for 'flag-outer' class.

+++ b/theme/flag.tpl.phpundefined
@@ -37,6 +39,9 @@
+<?php if ($needs_wrapping_element): ?>
+  <div class="flag-outer">

Shouldn't the wrapper classes be on the outer most div when it's displayed as a field?

I honestly don't know! It seems to be working fine with the JS link though.

Nevermind. Seems to click on and off fine. Thanks Joachim for your work and patience with me.

Thanks for the review! I'm going to take that as an RTBC.

I've tweaked the CSS class on the new wrapper DIV, and added a second class with the flag name. Here's the final patch.

Issue #1892930 by joachim, chrisjlee: Added placement of flag links as pseudofields.