Problem/Motivation

The procedural function flag() in flag.module is basically just a wrapper for flag_flag::flag() in flag.inc (implemented via $flag->flag()). However, the latter OO function has $skip_permission_check as a parameter whilst the former lacks this. For the sake of API consistency, the $skip_permission_check should be added to flag().

Proposed resolution

Provide $skip_permission_check as an optional boolean argument for flag(). Alternatively, deprecate the flag() function in favor of $flag->flag().

Remaining tasks

Patch rolling shortly, reviews needed. Also, arguments whether to deprecate flag()?

User interface changes

None

API changes

The procedural function flag() now accepts $skip_permission_check as an optional argument, defaults to FALSE.

Comments

joachim’s picture

joachim CreditAttribution: joachim commented

> Alternatively, deprecate the flag() function in favor of $flag->flag().

I'm tempted.

Or, keep the procedural flag() but don't make it as fully-featured as the method.

#871064: Making flaggings fieldable adds yet another parameter to the method :/

socketwench’s picture

socketwench CreditAttribution: socketwench commented

Or, keep the procedural flag() but don't make it as fully-featured as the method.

I'm much more in favor of object APIs myself. The downside is that this is already an established API call. If we're going to break something, let's break it once to allow for a better API.

joachim’s picture

joachim CreditAttribution: joachim commented
Version: 7.x-2.x-dev » 7.x-3.x-dev

Hmmm this one fell off my radar.

If we're going to break APIs we need to decide on something soon, before 3.x reaches beta release.

socketwench’s picture

socketwench CreditAttribution: socketwench commented

Let's not change everything at once. We're already moving everything to entities, and that's a big enough change already. Unless we want more headaches, let's let that simmer and focus on an object oriented API in 4.x?