Passing NULL to entity hooks

That's not the right way to fix this, I'm afraid.

We're always going to be saving a flagging, so we should always invoke that hook.

It's just that it's being invoked too early.

I'm confirming this bug. I'm using flag on user with ajax. When I click the flag I get 500 error. And the error is

EntityMalformedException: in entity_extract_ids() (line 7721 of includes/common.inc).

I traced the error and found that $flagging variable is NULL when is passed to entity presave hook implementations. In my particular case it crashes in title_entity_presave().

I see here the same error caused by the same NULL $flagged variable: #2189461: EntityMalformedException: Missing bundle property on entity of type flagging. in entity_extract_ids() (line 7721 of XXX\includes\common.inc)..

I don't know if this patch is the right approach. It seems to me that it only hides the problem. We have to find out why the entity comes as NULL.

And this is critical.

#2189461: EntityMalformedException: Missing bundle property on entity of type flagging. in entity_extract_ids() (line 7721 of XXX\includes\common.inc). has been marked as duplicate.

The entity passed to the hook is NULL because the hook is invoked at the wrong point.

It should be invoked later on in the process, when there is definitely a flagging object.

This is fixing the issue.

@claudiu.cristea, THANK YOU. I've been going nuts on this for past 3 hours.\

Patch 2162925-5 resolves 500 error.

"flag-null_entity.patch" worked for me. Thank you very very much. You saved me a lot of time!

oops - won't let me edit my comment. It is Patch 2162925-5 that worked for me. Thanks again!

Tests clean for me too.

Patch looks good, but will need some careful reading of the function it changes to check all cases are covered: we need to be invoking this hook iff there is a Flagging that is about to be saved.

It seems to me that there is an existing, separate bug where the hook is not invoked if the entity is already flagged -- that should be filed as a new issue once we've taken care of this.

I checked all possible cases in this function. The only point where we need to 'hook presave" is when $action == 'flag' AND !$flagged. Anonymous is saving in cookies and when $flagged = FALSE we are not saving. So here's the place for presave to live.

The proposed patch will prevent entity_presave to be called in the case when $this->_update_flagging($flagging); is reached, i.e. when $action == 'flag' && $flagged == TRUE && $flagging

> It seems to me that there is an existing, separate bug where the hook is not invoked if the entity is already flagged

I was wrong when I said this -- @georgir is correct. This is introduced by the patch, not a separate existing bug.

However, when $this->_update_flagging() is called, no actual database saving is performed. So invoking this hook here is actually incorrect.

@joachim, yes, you're right

patch

The patch in the #15 is solve my problem.

15: flag-2162925-15.patch queued for re-testing.

I committed tests this morning for hook invocation.

Let's check these pass with the patch, and then also consider expanding them to cover this case.

#15 works for me as well :)

#15 works for me too! thanks!

Patch looks good, though I'll add a few more comments to it and reroll when I have more time.

Particularly for this bit, which while it makes perfect sense, needs more explanation for future maintainer sanity:

        // Except in the case a $flagging object is passed in: in this case
        // we're, for example, arriving from an editing form and need to update
        // the entity.
        if (!empty($flagging->flagging_id)) {
          $this->_update_flagging($flagging);
        }

We're doing something in the case that we got a $flagging passed in, but we have to account for the fact that we possibly just made one a few lines up... it makes sense and I don't think it can be done better, but all the same... urgh.

I think part of the problem here is that with the advent of flagging entities in 3.x, the flag() method has gained a 3rd purpose, which is pretty much undocumented:

- you want to flag a thing
- you want to unflag a thing
- you want to update the flagging on a thing

The thing about the 3rd purpose, the update of the flagging, is that the flagging object itself is never updated! This is for the simple reason that there are no properties in the {flagging} table that you can update: there is no 'changed' timestamp, and if you want to change anything else (such as the uid, the entity id, the flag id) then you are actually wanting to unflag and then make a completely new flagging.

Therefore, when you use flag() to update a flagging, while the Field API fields on the flagging can get updated, the flagging table is not, and the flagging object itself is not saved!

This raises an interesting question about whether hook_entity_presave() should even be called in this case! I'm going to say that it should, since the fields are updated, and everywhere in Entity and Field API you have to save an entity to save fields. Therefore, we are in principle updating the Flagging entity, even if we're not bothering to update the {flagging} database table -- this can be viewed as a mere performance enhancement.

Hence I'll add some comments about that too :)

Patch from #15 did it for me too.

Patch #15 did the trick here as well.

I said in #21 I intend to work on it a bit more before committing it.

Updated patch, with more comments, and expanded tests to check for the parameters that hooks get.

We still need tests for the re-flagging case, but I've run out of time -- I'll file a follow-up.

Tests all pass for me locally.

The patch does not apply to current git:

patch < 2162925.26.flag_.entity-hooks-null.patch
patching file flag_flag.inc
Hunk #1 FAILED at 676.
Hunk #2 FAILED at 788.
Hunk #3 FAILED at 830.
3 out of 3 hunks FAILED -- saving rejects to file flag_flag.inc.rej
patching file flag.test
Hunk #1 FAILED at 1179.
Hunk #2 FAILED at 1189.
Hunk #3 FAILED at 1198.
Hunk #4 FAILED at 1207.
4 out of 4 hunks FAILED -- saving rejects to file flag.test.rej
patching file flag_hook_test.module
Hunk #1 FAILED at 6.
Hunk #2 FAILED at 18.
Hunk #3 FAILED at 33.
Hunk #4 FAILED at 42.
Hunk #5 FAILED at 51.
Hunk #6 FAILED at 60.
Hunk #7 FAILED at 118.
7 out of 7 hunks FAILED -- saving rejects to file flag_hook_test.module.rej

Am I doing something wrong?

You might not be on HEAD. Applies for me.

I already made sure I was in HEAD. And I've tried again:

# git fetch origin
# git pull origin 7.x-3.x
From http://git.drupal.org/project/flag
 * branch            7.x-3.x    -> FETCH_HEAD
Already up-to-date.
# git reset --hard HEAD
HEAD is now at e971d38 Issue #2193421 by joachim: Add tests for hook invocation and Rules event triggering during flag/unflag.
# patch < 2162925.26.flag_.entity-hooks-null.patch
patching file flag_flag.inc
Hunk #1 FAILED at 676.
Hunk #2 FAILED at 788.
Hunk #3 FAILED at 830.
3 out of 3 hunks FAILED -- saving rejects to file flag_flag.inc.rej
patching file flag.test
Hunk #1 FAILED at 1179.
Hunk #2 FAILED at 1189.
Hunk #3 FAILED at 1198.
Hunk #4 FAILED at 1207.
4 out of 4 hunks FAILED -- saving rejects to file flag.test.rej
patching file flag_hook_test.module
Hunk #1 FAILED at 6.
Hunk #2 FAILED at 18.
Hunk #3 FAILED at 33.
Hunk #4 FAILED at 42.
Hunk #5 FAILED at 51.
Hunk #6 FAILED at 60.
Hunk #7 FAILED at 118.
7 out of 7 hunks FAILED -- saving rejects to file flag_hook_test.module.rej

Oh, I needed to use git apply, sorry for that.

The patch seems to work for me so far, by the way.

Thanks everyone -- claudiu.cristea for working on the patch & everyone who tested and reporting back.

git commit -m "Issue #2162925 by claudiu.cristea, joachim: Fixed hook_entity_presave() getting invoked with a NULL for the Flagging entity." --author="claudiucristea "

Follow-ups:

- expand our tests: #2196097: add testing of re-flagging case to FlagHookInvocationsTestCase
- while writing the tests, I discovered these bugs! -- #2196085: hook_entity_delete() should be invoked before field data is deleted, #2196055: entity hooks don't get passed a proper Flagging entity.

You're welcome :)

(Resetting to fixed; the testbot doesn't know what it's doing!)

Great. Thanks!

is this part of the dev version now?
Can't apply the patch to the latest dev version.

The patch was committed in comment #32.

I still get this error: EntityMalformedException: Missing bundle property on entity of type flagging. in entity_extract_ids() (line 7727 of xx/includes/common.inc).

Is this not associated with this issue?

Nope, that's something else. If the flagging were NULL, you'd get a PHP crash because you wouldn't even have an object at all.