clean up flag() and its low-level helpers

This clean-up will help fix #2196055: entity hooks don't get passed a proper Flagging entity and #2196085: hook_entity_delete() should be invoked before field data is deleted.

The more I think about this, the more I think the right approach is indeed this:

> My hunch is to move all the 'work' from flag() into the low-level helpers, probably _insert_flagging() / _update_flagging() / _delete_flagging(), and kill off _flag() and _unflag().

This would mean we separate the logic and validation and the prep work from that actual work of the DB changes, API calls, and hook invocations.

So we'd have:

function flag() {
  // Get the user.
  // Check permissions.
  // Validate the flag.
  // Get session ID if required.
  // Are you flagging or unflagging?
  if (flagging) {
    // Prep the flagging object.
    // Low-level:
    _flag();
  }
  else {
    _unflag();
  }
}

function _flag() {
  // Change count
  // Invoke Rules
  // Invoke hooks
  // Call FieldAPI stuff.
  // Create the flagging entity.
  // Clear caches
}

// Same for _unflag().

Here's a rough stab. Needs further work. I should also have made this a sequential patch so it's clearer to see where the code is being moved around...

Here's a sequential patch that refactors the code and removes both sets of low-level methods in favour of a new single set.

With this, all the code for hook invocations, database operations, and cache clearing is together (apart from count changes).

It's now MUCH clearer to see what's going on and in what order. We can now clearly see problems like #2121053: field_attach_presave() is called too late.

So lots of bugfixes to do now that we have a clearer picture of the code flow here.

As for the count change code, I'm torn about folding that in. It does make sense, but I think it would tip flagging_insert() over into being too long on the page to comfortably take in. However, there are two things I can see that we could do to help with that:

- move the chunk of drupal_static_reset() calls to a helper method. I think that's useful code to wrap up, because it's perceived as a single block of 'clear lots of static caches' anyway. It's also a block that's repeated in two places, so that's a good thing too.
- add a helper to do rules_invoke_event_by_args() and the prep work. That would help readability: at the moment my eye just skims over the whole of that 'if (module_exists('rules'))' bit as just 'the Rules bit'.

So I think the roadmap now is take care of those two smaller refactorings in a follow-up, then consider moving the count change code in.

This isn't meta any more, though it is the linchpin to a lot of other fixes and clean-ups.

Thanks for reviewing!

Committed with a non-fast-forward merge, so we get the step-by-step changes in the history.

> I wish we could do more to refactor the top of that function

Any bits in particular? The things that stand out for me:

- The getting / building of a $flagging looks a bit messy to me.
- if (!$skip_permission_check) { -- I generally think double negatives are bad, and when there is also an else{} block there is just no excuse for it!!

> I prefer to keep a method no longer than a single screen

Agreed!

I definitely want to trim down flagging_insert() in that way: #2254171: move Rules event invocation to a helper method in flag_flag. I'll have a look at flag() and see if anything can be done there.

Hmmm the only thing I can find is #2254399: remove pointless $account check. Other than that:

- the stuff with $skip_permission_check could be moved to a helper, but then that's a lot of nesting with access() which is bad too
- the stuff with getting a sid could be moved to a helper, but likewise there'd be more nesting

Also in both cases, we'd have to have the helper set $this->errors, and then flag() would have to go inspecting $this->errors to see whether it should stop proceedings if an error's been set. So to my mind we wouldn't gain much.

Fixing #2196055: entity hooks don't get passed a proper Flagging entity should involve a fair bit of refactoring of code around the creating/populating of the $flagging, which hopefully will reduce the code.