I was exporting some changes to one of my features after updating to Flag 2.0-beta3, and I noticed in the code diff that the line that defines the flag name in a view relationship had been deleted in the new export like so:

@@ -2840,7 +2839,6 @@ if ($node->status && $node->nid) {
   $handler->display->display_options['relationships']['flag_content_rel']['field'] = 'flag_content_rel';
   $handler->display->display_options['relationships']['flag_content_rel']['relationship'] = 'content_profile_rel';
   $handler->display->display_options['relationships']['flag_content_rel']['label'] = 'Featured Profile';
-  $handler->display->display_options['relationships']['flag_content_rel']['flag'] = 'featured';
   $handler->display->display_options['relationships']['flag_content_rel']['user_scope'] = 'any';
   /* Relationship: Flags: following counter */
   $handler->display->display_options['relationships']['flag_count_rel']['id'] = 'flag_count_rel';

The view itself had not changed in any way. I did, however, also update to the latest development version of Views 3. After pouring over the Flag module code to see why that line had been removed and what impact it might have, I think I have discovered what is happening. If I'm correct, what's happening is that Views is not exporting items in a relationship that are default. That would be items that match what the relationship's option_definition() method returns. Since the flag_handler_relationship_content->option_defition() method sets $options['flag'] to be the first flag in the content_types list by default, that's what gets returned by the option_definition(). Views dutifully sees that the relationship definition for the flag matches what's been defined as the default and does not export it.

Setting defaults for user_scope and required make sense since those are static options. Since the flags and content_type mappings a user has can and will change from installation to installation, actually setting a default flag here causes problems. If I export that view to a feature and distribute that, other systems are not guaranteed to have consistent results if the flag was the default flag on my system but isn't on theirs.

I'm not sure what the right answer is here, but my thought is that the default flag should *not* be set in the option_definition. That causes problems. I would think that setting the #default_value in flag_views_flag_config_form() from flag_views_flag_default() if a $current_flag was not given would be the right answer. I have not yet created a patch to this effect because I wasn't sure of any other possible repercussions of doing this and wanted to float this out there before doing it.

CommentFileSizeAuthor
#13 no-default.diff2.16 KBmooffie
#8 views-def-flag.diff3.99 KBmooffie
#1 flag_views_relationship_no_default_flag.patch1.07 KBmgriego

Comments

mgriego’s picture

mgriego commented
Status: Active » Needs review
StatusFileSize
new flag_views_relationship_no_default_flag.patch1.07 KB

OK, I was able to come up with a patch for this. Of course, you must declare an option in the option_definition(), so, in this case, I set the default to NULL. In flag_views_flag_config_form(), I change it to check and see if the $current_flag is empty and, if so, set it to flag_views_flag_default(), otherwise use the $current_flag value. This seems to work fine in my testing, and it ensures that the flag name will always be output for exported Views relationships.

mooffie’s picture

mooffie commented
Status: Needs review » Needs work

Yes, I too have noticed this don't-export-defaults behavior of Views. Indeed, we must not declare a "default" flag.

As for your patch:
- flag_views_flag_default() is no longer necessary, I believe. You can initialize #default_value to the first of $options.
- There are two more places to get rid of flag_views_flag_default().

mooffie’s picture

mooffie commented

There's a subtle problem:

I think it's possible to skip filling out the options form: because one doesn't have to click the 'Update' button (I don't have Drupal in front of me, so I can't validate this). This is most probably the reason we'd set up a default for the flag-name.

So, with this new patch, when a user skips this form, the flag-name setting will be NULL. This will break our handler. We can use #901116: Views validation: guard against non-existent flags to guard against this, but still if the user looks at the form he will see nothing wrong...

=======

Things to check: can we force Views to export an option even if it's a default? (I think I once saw something about an export callback. Maybe nowadays there's something simpler.)

mgriego’s picture

mgriego commented

Ah, yes, I see what you're saying. I went with the NULL approach after looking at how some other relationship handlers were written.

I think we have a couple of options here. In all cases, I think the validation patch seems like a good one to include. Beyond that, we could do this:

1. Leave the default value at NULL and change ui_name() to this:

  function ui_name() {
    // We put the bookmark name in the UI string to save space.
    return t('!group: !title', array('!group' => $this->definition['group'], '!title' => (empty($this->options['flag']) ? '(Flag not specified)' : $this->options['flag'])));
  }

Doing that lets the user know if they forgot to specify the Flag. That along with the validation patch seems like the best route to me.

or

2. We could specify the export handler in option_definition() ($options['flag']['export']) which would output the flag definition regardless of whether it was the "default" value or not. This doesn't look too difficult.

As long as we do one of those two things along with the validation handler, we should be safe. With the default value being NULL, the user won't be able to save the View since it won't validate. And this ensures that the saved value will *never* be the default value.

mgriego’s picture

mgriego commented
Issue tags: +export, +views, +features
mooffie’s picture

mooffie commented

I've marked #901116: Views validation: guard against non-existent flags a duplicate of this one.

mooffie’s picture

mooffie commented

I think we have a couple of options here.

AFAICS, There's only one solution: none of the radios will be selected initially. If the user doesn't select a flag, our validate() will shout at him. (There's also the cosmetic "(Flag not specified)" label you suggested in #4).

We could specify the export handler in option_definition() ($options['flag']['export']) which would output the flag definition regardless of whether it was the "default" value or not.

(No, it turns out this won't help: this addresses only the export problem. But, as I explain (in the following comment), there's yet another problem to consider.)

mooffie’s picture

mooffie commented
Status: Needs work » Needs review
StatusFileSize
new views-def-flag.diff3.99 KB

I'm attaching the patch.

I want to focus this discussion by explaining the problem again:

We can't use a "default" for the flag. Why?

  1. Because Views (newer versions) doesn't export default values. However, an even nastier problem is this:
  2. Views doesn't force a user to click the "Update" button. When the user skips the "Update" button this means that the $options aren't saved. They will be initialized anew (from the defaults) every time the view runs. The resulting view will be inconsistent: it we add/remove flags, the "default" one will change and the view will fail mysteriously. Bugs that are very hard to detect will ensue.

The solution

  1. We don't pick a default flag.
  2. The radios for picking a flag are, as usual, #required. None of the radios is initially selected. If the user doesn't select a flag (that is, if he skips this form), then our validate() will shout at him.

The implications

  1. Again: None of the radios will be selected initially. If anybody knows of any alternative, let's hear it. (I tried initializing $options['flag'] in the init() method, thinking this will get stored in the view even if the "Update" button is skipped, but it isn't).
  2. Naturally, you'll have to import a flag before importing a view that uses it (because you won't be able to save a view if it refers to a non-existing flag: validate() will complain).
mooffie’s picture

mooffie commented
Priority: Normal » Critical

This is certainly a critical bug.

(We should probably commit this to Flag 1.x as well. I have an "abstract" keyword in that patch, which only works on PHP 5, but I'll remove it before committing to Flag 1.x (Flag 2.x is about to become PHP 5's: we've settled on this at an issue mentioning the "static" keyword).)

mooffie’s picture

mooffie commented

[...] and change ui_name() to this:

  function ui_name() {
    // We put the bookmark name in the UI string to save space.
    return t('!group: !title', array('!group' => $this->definition['group'], '!title' => (empty($this->options['flag']) ? '(Flag not specified)' : $this->options['flag'])));
  }

Well, I'll add this extra notification if the patch gets approved (by Nate).

But people might interpret this "(Flag not specified)" message as some feature ("Ahh, nice! If I don't pick a flag it grabs all of them!"). I don't know if it's a great idea.

mgriego’s picture

mgriego commented

I pretty much agree on all counts. Having no defaults and complaining if nothing is chosen is best. That forces the user to do something and exports to work properly (assuming the flag exists, of course). I do think that the change to ui_name is important for usability reasons, though we can certainly pick different wording.... "(Please choose a Flag)"? Also, I haven't fully looked over the patched code yet, but it might be worthwhile to distinguish, both in validate() and ui_name() between "no flag chosen" and "flag chosen does not exist", at least from an ongoing support standpoint.

mooffie’s picture

mooffie commented
Issue tags: -export, -views, -features

But there's also backward-compatibility to worry about.

I'm going to add an init() method to fix this. See the "Backward compatibility" comment:

  function init(&$view, $options) {
    parent::init($view, $options);

    if (!isset($this->options['flag'])) {
    
      // Backward compatibility: There may exist old views on the system whose
      // 'flag' option isn't set. (This happens if the admin had skipped
      // clicking the 'Update' button.) When run, these views should behave as
      // if the first flag was selected.
      $content_type = isset($this->definition['flag type']) ? $this->definition['flag type'] : NULL;
      $this->options['flag'] = flag_views_flag_default($content_type);
      
      // If we're on the admin page, we also write this selection back to the 
      // view. Otherwise, we give the user the impression the first flag is 
      // selected but if he skips the 'Update' button this options isn't 
      // recorded.
      //
      // (For the technique, see #313377 and #380560.)
      if (function_exists('views_ui_cache_set')) {
        $view->set_item_option($view->current_display, 'relationship', $this->options['id'], 'flag', $this->options['flag']);
        $temp_view = $view->clone_view();
        views_ui_cache_set($temp_view);
      }
    }
  }

I also use here a trick to write the flag selection back to the view. So now everything behaves as it used to be: the first radio button is automatically selected.

However, there's a potential bug here: $view->current_display. Is it possible that an instantiated handler belongs to a display that isn't current_display? I think so. I'll have to investigate this.

Alas, our Views support shows its age. 'Flag Vista' has none of these problems.

mooffie’s picture

mooffie commented
StatusFileSize
new no-default.diff2.16 KB

Please completely ignore my previous comment.

The patch for #901116: Views validation: guard against non-existent flags has been committed, and it makes solving this issue easier.

Here's a revised patch.

mgriego’s picture

mgriego commented

I like the way it looks. I may not get a chance to test it myself, though, until after the holidays, but it looks like it pretty much covers everything here when combines with the other patch.

mooffie’s picture

mooffie commented
Title: Exported views may not specify the Flag in a relationship if it is the default flag on the originating system » Don't specify a default flag

Shortening the title.

mooffie’s picture

mooffie commented
Status: Needs review » Fixed

Committed.
http://drupal.org/cvs?commit=470148
http://drupal.org/cvs?commit=470152

Nate:
The only user-visible change is that none of the radios (for choosing a flag) is selected initially. (I investigated a hackish "workaround", in comment #12, but eventually decided against it: we should use the tools Views gives us, not invent our own.)

quicksketch’s picture

quicksketch
he/him
commented

Thanks mooffie, I was worried seeing #12. Flag is already a more unwieldy module that you'd expect. The less complexity the better. :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

ckidow’s picture

ckidow
he/his
Primary language German
commented

Drupal: 6.22
Flag: 6.x-1.3
Views: 6.x-2.12

Any support for flag 6.x-1 versions on this issue?

Because default value for exposed filter is always 1 (i think because of the relationship settings) and views can't remember my settings for this filter.