Special-case the display of field_issue_assigned to handle unassigned issues

I'll work on this next.

Ugh. This is turning into a rat hole. :(

Here's a work-in-progress hack, but it's not really working (and it's ugly).

In practice, we've hidden 'Anonymous' as an option in the assigned choices, instead opting for the "- none -" value for the field. However, once you do that, the field has no value, and doesn't render at all (no label, no value, nothing). The only reason we're seeing the Anonymous stuff from the OP is due to d.o issue migration -- we set the value to uid 0 instead of unsetting it entirely. And helpfully, devel_generate (what I see on my local site) sometimes uses uid 0 for this field and we see 'Anonymous'. But when you edit those issues, the Assigned widget defaults to "- none -" and the auto-generated comment shows:

Assigned: Anonymous >>

I'm really not sure the right thing to do here, but I lean toward just leaving it alone, having the field completely disappear from display if the issue isn't assigned, and fix the migration to not set it to uid 0 when it's unassigned. That'd be the least amount of custom magic and be closest to the D7 Field API Way(tm).

When I proposed this option before submitting this issue, tvn didn't like that since then users wouldn't necessarily know that "Assigned" is an option until they try to edit the issue. But, now we have to weigh that concern against sinking a bunch more time into this and fighting against field API. Generally, I'd rather just do things how Field API expects -- the more we fight it and try to add special-case magic, the less benefit we get from this whole re-write.

Thoughts?
-Derek

In Skype, tvn asked if hook_field_attach_view_alter() might help. Nope. :( Tried that, but if there's no field value, the field doesn't show up at all at view_alter() time. I mean, we could potentially have some magic that tries to re-inject a value or something, but ugh. :(

Another approach would be making field_issue_assigned required (so there's no "- none -" value), change project_issue_assigned_choices() to put 0 back as a choice, have the field default to unassigned on new issues (so people don't have to click to choose), and continue down the path from patch #2. Then, there'd always be a value, the migration stuff wouldn't need to be changed, and I believe we could get this working as desired without *too* much more trouble. The main problem is that entityreference is going to think 0 == 'Anonymous' so we'd also have to somehow alter the widget, too. I believe that could be as simple as hook_field_widget_form_alter().

Yeah, that all works quiet well. This is basically happy now, with a few exceptions:

A) We also need to teach nodechanges about this special case. :(

B) I'm not thrilled about the name for this new field formatter for entity reference fields. Also, it honors a display setting for 'link' but it's not actually exposing that in the UI -- I guess we need to implement hook_field_formatter_settings_form().

So technically this should stay 'needs work', but I wouldn't mind a review before I continue down this path.

Thanks,
-Derek

whoops, the patch would help. ;)

I pointed out in IRC that an alternative to special-casing this in nodechanges is to just go ahead and fix #1979066: Consider using field formatters to display changes since then we can just reuse the special-case field formatter and it should Just Work(tm). There are a growing number of things that would solve for us, e.g. issue tags as links, etc. I'm not positive how hard that is, but drumm said it's worth spending ~30 minutes trying to figure out how it would work, since if it is as easy as I hope/think it will be (maybe 1/2 day), it's a win. So, I'm going to do that next...

Re: nodechanges: there's now a working patch at #1979066-3: Consider using field formatters to display changes that solves that part of this problem.

So, all that remains here is 7.B. Anyone want to sanity check the rest of #8?

Thanks,
-Derek

Rerolled now that #1979066: Consider using field formatters to display changes is in (since I pushed a commit to configure the new 'nodechanges' view mode and the previous patch no longer cleanly applied). The settings form still doesn't exist/work. Still interested in a review on the basic approach.

Actually working patch now. ;)

This is uglier and more evil than I thought. I'm not sure this is really the right approach. However, it seems there's no hook_field_formatter_view_alter() we can use, so I don't think there's a way to actually just extend and alter EntityReference's formatter, instead of trying to define our own like this...

Some tasty quotes from IRC about this patch:

[5:55pm] dww: what's the most sane way to alter the output provided by another module's field formatter? I'm trying to special-case the handling of uid 0 for an entityreference field...
[5:56pm] dww: I've tried defining my own formatter, but that's evil and gross for all sorts of reasons.
[5:56pm] timplunkett: dww: there isn't a hook_field_formatter_view_alter, i always end up writing my own formatter and wrapping the other
[5:56pm] dww: ugh
[5:56pm] linclark: yep, same
[5:56pm] dww: wow, crazy.
[5:56pm] dww: I was sure hook_field_formatter_view_alter() should exist.
[5:57pm] timplunkett: dww: i asked for it in D7, but it was considered terrible for performance
[5:57pm] timplunkett: dww: "e symmetry between widgets and formatters raises the question of a similar hook_field_formatter_view_alter() hook on the formatter side - which we cautiously avoided so far for performance reasons (a listing page can call 100s of formatters, and field rendering is already quite slow)."
[5:58pm] dww: timplunkett: yeah.
[5:58pm] dww: hrmph. So, https://drupal.org/node/1983294#comment-7396496 is really the best I can hope for? :/
[5:59pm] • dww hangs head in shame for https://drupal.org/files/1983294-12.assigned-user-unassigned-is-special....
[6:00pm] timplunkett: dww: http://drupal.org/node/1204230#comment-4728526, oooold issue. no chance for D7
[6:00pm] Druplicon: http://drupal.org/node/1204230 => Missing hook_field_widget_form_alter() => Drupal core, field system, major, closed (fixed), 48 comments, 42 IRC mentions
[6:00pm] timplunkett: dww: in D8 you can just extend the Formatter class you want
[6:00pm] dww: timplunkett: right. That would be easier.
[6:02pm] dww: timplunkett, linclark : I don't suppose either of you wants to look at this horrible patch and tell me how it could be better, do you? ;)
[6:03pm] dww: (D7 drupal.org would thank you)
[6:03pm] timplunkett: dww: i glanced, looked like stuff i've done before
[6:03pm] dww: timplunkett: hah
[6:03pm] • dww sighs
[6:04pm] dww: it's issues like this that make me wonder if re-writing all of project_issue to be all Field API all the time is actually a win for reducing technical debt. :/
[6:04pm] timplunkett: dww: pseudo subclassing field_collection formatters is gnarly
…
[6:06pm] davereid: dww: Another alternative would be to use http://drupal.org/project/token_formatters and have project_issue make a custom token of [user:name-or-custom:Unassigned] as a dynamic token
[6:07pm] davereid: it might be way less debt
[6:07pm] dww: davereid: but that wouldn't allow the username to be links to the user, right?
[6:07pm] davereid: yes it could still link to the user
[6:07pm] dww: davereid: hrm, interesting!
[6:07pm] timplunkett: davereid++
[6:07pm] timplunkett: ++++
[6:08pm] dww: davereid: okay, maybe I'll take a look... although there's always the risk that someone gets their undies in a bunch about deploying another module on d.o. :/
[6:08pm] davereid: or just a token like [user:linked-name-or-unassigned] that does the link or not
[6:08pm] • davereid realizes he has a typo in his project screenshot
[6:08pm] dww: davereid: yeah, that does sound a lot better than this.
[6:08pm] dww: davereid: thanks!
[6:09pm] dww: davereid++
[6:09pm] davereid: dww: I can help with writing the tokens that you'd need too

[6:09pm] dww: davereid: if you want to post another patch to that issue, I'd be thrilled. Sadly, I need to bail pretty soon for the night... :/
[6:10pm] davereid: but yeah it is unfortunate that there's no hook_field_formatter_view_alter()
[6:10pm] davereid: :/

@drumm: What do you think about using token_formatters for this instead of my horrible patch? That seems like a useful and slick module, which might potentially solve other problems for us down the line. Please assign back to me once you decide if we should continue with #12 or if we should go this new direction.

Thanks,
-Derek

Sweet, thanks!

@davereid: any interest and availability to roll a patch for this?

Sweet, thanks! I'm merging that in with the relevant chunks from the previous patch, and then reconfiguring this field to use the new display formatter. I'll post a new patch with everything once it's working locally. My current plan is to make token_formatters optional, not a new dependency. If that becomes a PITA, I'll just make it a dependency. Anyway, stay tuned... ;)

Almost working... but whoops. ;) #1997430: Entity reference support can't handle anonymous users due to !empty() check

davereid is a champ! He already fixed that issue and created 7.x-1.2 of the module for us. ;)

This is now working beautifully on my local site. There were a few minor bugs in #18 (we need 'name' not 'title' and this check was backwards: $langcode = !isset($options['language']) ? $options['language']->language : NULL;) but those were easy.

So, I pushed #18 as Dave:

http://drupalcode.org/project/project_issue.git/commit/d965f22

Then pushed all my changes to take advantage of it:

http://drupalcode.org/project/project_issue.git/commit/73d9ecf

And finally a fix to drupalorg not to undo part of these fixes during rebuild. ;)

http://drupalcode.org/project/drupalorg.git/commit/a82e178

It ended up being a bit of a PITA to try to make this module optional, since the wasn't an obvious and good place to enable it during the upgrade such that it would be enabled when we're trying to create the default issue node type. So, I just made it a required dependency. Oh well...

Calling this fixed. Thanks again, Dave!

p.s. Also pushed a fix to the projectinstall distro to add the new dependency:
http://drupalcode.org/project/projectinstall.git/commit/a535c0d