Allow using standard permissions table for flags

Input formats switched completely over to using permission table, so it seems reasonable for flag to do the same.

Not sure, but flag currently has the issue that it stores it as role id's so doesn't transfer well to other sites with different roles (and can cause a security issue on the other site due to mismatch?). This would address that issue as well (since features role, perm support doesn't use rids).

Means about 4 permissions per flag: flag [flag name] content, unflag [flag name] content, flag [flag name] content, flag [flag name] own content, unflag [flag name] content.

could likely do something where keep the same ui, but save to permissions table for role settings, else just do like formats did (which I'm not totally sure XD).

Would probably mean some sort of handling for depracted flag definations, etc..

This looks good!

> Means about 4 permissions per flag: flag [flag name] content, unflag [flag name] content, flag [flag name] content, flag [flag name] own content, unflag [flag name] content.

Probably only two for now:

- 'flag FLAGNAME content'
- 'unflag FLAGNAME content'

To have 'own' permissions, we need to have a notion of which flag types support it: see #879988: Flag permission for 'own entities', which I've marked as a follow-on to this.

Assigning this to myself.

Adding the Needs change notification tag. This should be created as a stub before this patch is committed, so that the hook_update_n() that sets up permissions based on flags can give users a link to it.

The reason for this is that this change will introduct a significant change in access to flag properties: users who can admin permissions but can't admin flags will be able to change access to a flag, whereas before they could not. This is something site admins need to be made aware of, as it could have implications for their site roles and access.

This is a work in progress:

- the update path needs testing and possibly tweaking, as I'm not sure why on my copy flags are still showing they have a roles array.
- some things need changing in flag export.
- I suspect we may need to change the flag API version to 3 to reflect that we no longer have a roles array stored in the flag object.
- possibly need a form validation on the main permissions form to warn of incompatible permissions, such as:
-- can't have unflag permission without flag permission
-- can't have only flag permission without an 'unflag denied' message

(These special rules are a good example of why flag handling its access in-house has advantages!)

Testing or review of what there is so far would be very welcome!

Fixed the problem the testbot was dying on.

I've changed the issue summary, as it was out of sync with what I'm doing in the patch.

Previously: suggested adding an option to each flag 'use core permissions'
Changed to: all flags provide core permissions.

This is considerably simpler to implement, as it's replacing the flag access system, rather than implementing a new system to live in parallel with the current one!

+  // Saving the flag now will remove the roles array from the serialized options.
+  $flag->save();

Shouldn't that be inside the foreach loop?

Oh heck yes! Good catch! :D

Updated patch with that fix. See #8 for list of things still to do.

Update function works fine now :)

Tagging.

I've figured out what's going wrong in the update; will work on it soon.

In the meantime do please test this as a fresh install and report any findings.

Here's the updated patch.

It would be great if this could see some testing -- it's a pretty big change!

Needs work.

I realized that loading roles into the flag object with user_roles() is adding a ton of queries to flag loading, and flag loading happens on most page requests.
Currently reworking this so we use user_access() when needed instead.

Updated tests.

I think we're going to have to give validation of user permissions a miss, because of #222380: No error highlighting on form checkbox or radio input types. Showing validation errors on such a large form, and with no easy way of finding the offending element is just going to be a UX horror.

Test was failing because permissions cache needed clearing in the test.

But this also let me discover that changing the flag name caused permissions pollution. Fixed that too.

Fixed the test again.

Finally! :D

Updated patch which restores the default values for the role access checkboxes when creating a new flag.

Any more reviews before I commit this?

Another thing to do: somehow handle the change from $flag->roles to permissions in the flag update system.

What I'm thinking is to change the key to $flag->import_roles and then take those as incoming data only when saving a new flag (ie with no fid).

Added updating of roles on API 2 flags in code.

And another thing. Disabling $flag->roles no longer makes sense. If you want to lock that, use something that locks permissions generally.

Yikes. Flag_bookmark tosses a nasty error when enabling:

[tess@nitori drupal7]$ drush en -y flag_bookmark
PHP Warning:  Module 'xdebug' already loaded in Unknown on line 0
The following extensions will be enabled: flag_bookmark
Do you really want to continue? (y/n): y
WD php: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'module' cannot be null: INSERT INTO {role_permission} (rid,    [error]
permission, module) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array
(
    [:db_insert_placeholder_0] => 2
    [:db_insert_placeholder_1] => flag bookmarks
    [:db_insert_placeholder_2] => 
)
 in user_role_grant_permissions() (line 3034 of /home/tess/sites/drupal7/modules/user/user.module).
Cannot modify header information - headers already sent by (output started at /usr/lib/drush/includes/drush.inc:596) bootstrap.inc:1239            [warning]
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'module' cannot be null: INSERT INTO {role_permission} (rid, permission, module) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array
(
    [:db_insert_placeholder_0] => 2
    [:db_insert_placeholder_1] => flag bookmarks
    [:db_insert_placeholder_2] => 
)
 in user_role_grant_permissions() (line 3034 of /home/tess/sites/drupal7/modules/user/user.module).
Drush command terminated abnormally due to an unrecoverable error.                                                                                 [error]
Error: Module 'xdebug' already loaded in Unknown, line 0

Interestingly, the module does enable.

Beyond that, everything tests cleans and functions correctly.

Thanks for trying it out! I didn't think of enabling the bookmark module... for that matter, not 100% I've tested creating a new flag, which may have the same sort of problem...

> Column 'module' cannot be null: INSERT INTO {role_permission} (rid, [error]
permission, module)

Quite a showstopper! This is because the flag doesn't exist at this point, so hook_permission() doesn't define permissions for it, which means that the attempt to give users those permissions is trying to grant permissions that system doesn't know about.

Urgh. No idea how to resolve that.

Forgot to mark as 'needs work'.

Ah, just a cache clear thing. Phew. Will fix later.

Yet another patch :)

There we go! Enables clean, tests clean, confirmed with manual testing. Marking RTBC!

Woohoo!!! let's get this in!

Thanks for the reviews.

Oh yes I need to write the change notification too.

Done.