Failed update persists entity attributes

[hypeJunction]

$ia = elgg_set_ignore_access(false);
$user->language = 'de';
$user->save(); // false

echo $user->language; // de
echo get_entity($user->guid)->language; // en

The problem is that subsequent calls to other methods that do not require permissions checks could propagate these attributes to caches.

[mrclay]

So we either revert attributes on a failed save, or provide a revert() method and beg plugins to use it. Any other ideas?

[mrclay]

We might want to add $entity->hasUnsavedChanges() and have entity caches check it. I'd prefer that instead of resetting attributes in a way that might surprise devs.

[hypeJunction]

We should start throwing exceptions, IMO. Ideally we would have a set of HTTP exceptions, which would be caught in router. Failing silently is not an option anymore.

[mrclay]

You mean if a plugin uses permissions in a valid way to block a save(), we should throw? Of course, plugins can try/catch a FailedSaveException, but it's a big BC break.

[hypeJunction]

Plugins should only be saving in actions, that would be caught by the router. So, we are just making their lives easier.

[mrclay]

Legacy written actions would lose custom error messages and, more importantly, any work that came after, or partial Ajax response data that was going to be returned. These aren't reasons not to change, but considerations we can't ignore.

[hypeJunction]

Let's stick with untestable code that has a brain of its own. Maybe in another 20 years we will finally learn

[mrclay]

save() returning false doesn't make it untestable. It's a trade off like everything else. The greater crime toward untestability is all the invisible caching. I gave a solution above that I could try.

[mrclay]

Another: the moment an attribute is changed, purge the entity cache of it and disable caching for it. Leave it to be cached on a later request.

This is functionally the same as above though. We want to mark the entity dirty for the rest of the request.

[jdalsem]

will this still be a problem when the attributes have been moved to metadata?

[hypeJunction]

will this still be a problem when the attributes have been moved to metadata?

It would for user entities

[jdalsem]

There is no clear direction for a solution for now. If using save() do not assume it is successful