No content types checked: allow all

+1

"What nodes this flag may be used on:" should default to all if nothing is selected.

Consider the scenario:

I create "Report abuse" flag and select all content types. Now every time I add a content type, I need to go back to the Flag settings and mark the checkbox for the new content type I just created. There is no other way to communicate my intentions.

Another scenario from a module developer perspective:

I've supplied a flag through hook_flag_default_flags, I cannot possibly know what types to allow the flag for. I have several choices:
* Use Drupal defaults ('page', 'story') and pray that the target install still has them
* List all types
A sensible default of empty (allow all types) would be much better.

Thank you!

I've supplied a flag through hook_flag_default_flags, I cannot possibly know what types to allow the flag for. I have several choices:

There are other choices that you didn't consider:

- You can get a list of the content types by calling node_get_types();
- If a new content type is created, you can respond to the creation of the new content type by implementing hook_node_type() when $op == 'insert'.

Regardless, the ability to uncheck all content types seems reasonable, except that the the "User roles" options right next to the node types options works in reverse, where unchecking all user roles makes it so that the flag is completely disabled (except for user #1). Even though Drupal often uses this "leave empty to allow allow" paradigm, I'm not sure I want to have two sets of checkboxes work in opposite ways right next to each other.

Thank you for the quick reply.

Using node_get_types is what I meant for "List all types" and this is the approach I am using now. =) I want to provide a sensible default of "apply to all", but I don't want to actively enforce it down the line because the user might disagree with my defaults. Using node_get_types mimics this behavior as closely as possible as it is now. All of this is OK for a developer, but an end user of Flag still has no way to create this behavior out of the box.

I think you said it best yourself:

Drupal often uses this "leave empty to allow all" paradigm

As a Drupal user thats exactly the behavior I expected when I left it blank, but I was instead surprised with a "this field is required" message.

What if you change "What nodes this flag may be used on:" to "Restrict which nodes this flag may be use on:" in order to underline that an action here would narrow down the flag and change the hint from "Check any node types that this flag may be used on[...]" to "Leave blank to allow the flag to be used on all nodes." for completeness.

In a word, I think that the proposed behavior matches a typical Drupal user's expectations and thats reason enough for change.

The other obvious solution is to have an extra control in a form of a checkbox or a radio selection to select that you want the flag to apply to all types. But I'm not a fan of adding "more stuff." =P

You could have a radio selection for "What nodes this flag may be used on:"
"All nodes"
"Restrict to several types"

Default to the first option and hide the list of types, which makes the form cleaner, then if the user selects the second use jQuery to show the list of types.

This could actually be a win win.

Anybody has more ideas on this?

> I'm not sure I want to have two sets of checkboxes work in opposite ways right next to each other.

> the proposed behavior matches a typical Drupal user's expectations and thats reason enough for change.

*sigh*

I kind of agree with both...

The two sets of checkboxes look different, so I think that lowers the weight of the objection about that. And things like entityref module have this behaviour too now, so it's more and more prominent.

And I for one am finding it a real pain when setting up test flags... ;)

As much as it pains me, I think we have to postpone this to 3.x.

The reason is that it's not just a question of allowing the UI to accept no checkboxes selected.

Because then either:

A) we save all known bundles for the entity into {flag_types}. But then when an entity is added, the user gets a WTF, because the new entity isn't covered.

B) We save nothing into {flag_types}, and a LOT of code has to be changed to take that into account.

The good news though is that I plan on branching for 7.x-3.x pretty soon :)

See also #1699660: drop {flag_types} table; move bundle data to $flag->options.

Huh. This was a lot easier than I thought!

Testbot likes it and it's a simple change. Committing this & we'll get human testing done when there's an alpha release I guess!

I added a line of docs to the class variable:

  // The sub-types, e.g. node types, this flag applies to.
  // This may be an empty array to indicate all types apply.
  var $types = array();

Issue #471212 by joachim: Changed flag types property to allow an empty array to mean all object subtypes/bundles apply.

Attaching a backport for 7.x-2.x

#13 missed a required update to type_access_multiple which prevents the check from happening if flag types are not checked.

This reroll includes that change as well.

> #13 missed a required update to type_access_multiple which prevents the check from happening if flag types are not checked.

Did I miss that out on the 3.x patch too? If so, we need to fix it on that branch first, then come back to the backport.

@joachim: from looking at the diff it looks like you did. However, the check is still present in 3.x. It looks like it went in during the followup -- #1743240: type_access_multiple() doesn't take empty $types array into account

Ah good. Thanks for clearing that up!

I'd really like some users of 7x2x to try out this patch before I commit it!

Been using this patch with Flag 7.x-2.x in Open Atrium 2 successfully for many months now.

The fix in #14 was fixed on the 3.x branch in #1743240: type_access_multiple() doesn't take empty $types array into account. So I'm rerolling the patch here so that the fix is the same.

(Not that we're doing much backporting, but if we can keep the code the same in both branches with minor effort, we might as well.)

I tend to prefer to write 'bail-out' blocks where possible, as it means you get one less level of nesting, and you also have a place to comment what the bail condition is, so:

if ($dontcare) {
  // Bail because of foobar.
  return;
}

// Do actual stuff.

BTW @mpotter: I didn't know Atrium was still on Flag 2. I'd really like the large users of Flag to upgrade to 3, as I am thinking it would be good to be able to drop support for it! I'll maybe open an issue to discuss the matter.

Issue #471212 by joachim, wbobeirne, beeradb: Changed flag types property to allow an empty array to mean all object subtypes/bundles apply.