I am new to Social Engine, but have already found a number of quite surprising issues with it. As such my analysis relies upon a certain amount of logic and assumption. Ironic really, as assumptions are what cause this particular issue...
Symptoms:
A user trying to delete their profile from http://{domain.com}/members/settings/general gets an 500 Internal Server Error.
Administrators attempting to delete profiles from admin get delete confirmation modal popup. On clicking delete, the popup turns white and user is not deleted. Inspecting traffic in the Firebug Net panel, the ajax request is returning a 500 Internal Server Error.
Checking {domain}/statistics/logs/error_log reveals the following errors generated each time:
PHP Fatal error: Call to a member function remove() on a non-object in /path/to/domain/application/modules/Album/Model/Photo.php on line 150
PHP Fatal error: Undefined class constant 'PRIMARY_TYPE_NUM' in /path/to/domain/application/modules/Core/Model/DbTable/Session.php on line 538
Cause:
I believe the cause is a user with missing images that are listed in the database. This in itself should not be an issue, but it seems the code makes fatal (!) assumptions regarding the existance of images that are listed in the database, a rookie mistake if ever there was one. When dealing with files on the file system, one must always confirm their existance, and if possible validate the file content before attempting to operate on it, else you are open to software errors and worse, security risks.
In this case the files are dealt with through objects, representing the file and the possible methods, or operations, upon it. Where a file does not exist, the object is not created, so it is the object, rather than the file whose existance needs confirmation before operating upon it.
As the source code itself notes:
// This is dangerous, what if something throws an exception in postDelete
// after the files are deleted?
While the author of this note is considering a different edge case, this would seem to be a worrying indicator of the standard of coding .
The 'Undefined class constant' error is merely a consequence of the prior fatal error.
The offending lines are
application/modules/Album/Model/Photo.php
l.149 - 150
$file = $this->api()->getApi('storage', 'storage')->get($this->file_id, null);
$file->remove();
You could say that it is 149 that is the cause, rather than 150. 150 tries to do a perfectly valid operation upon the presumed object, but it is 149 that has failed to return an object as expected.
By that token however the cause lies in the storage API, in that it fails to return an object in the first place.
Note that exactly the same careless approach is taken immediately after this operation for thumbnails, and then again (although commented out) for cropped versions.
Solution:
Hack:
Add an if(is_object($object)) check before the operation:
Unix Diff:
application/modules/Album/Model/Photo.php
150c150,152
< $file->remove();
---
> if(is_object($file)){
> $file->remove();
> } 152c154,156
< $file->remove();
---
> if(is_object($file)){
> $file->remove();
> }
For clarity, we are replacing:
l.149-152:
$file = $this->api()->getApi('storage', 'storage')->get($this->file_id, null);
$file->remove();
$file = $this->api()->getApi('storage', 'storage')->get($this->file_id, 'thumb.normal');
$file->remove();
with
l.149-156:
$file = $this->api()->getApi('storage', 'storage')->get($this->file_id, null);
if(is_object($file)){
$file->remove();
}
$file = $this->api()->getApi('storage', 'storage')->get($this->file_id, 'thumb.normal');
if(is_object($file)){
$file->remove();
}
I am unsure as to just what the failed operation on 149 had returned - if it was anything equating to FALSE, then an alternative and more succinct solution might be:
if($file = $this->api()->getApi('storage', 'storage')->get($this->file_id, null)){
$file->remove();
}
As this is little more than a dirty hack, I haven't taken the time to look into this further. I don't want to be creating users and deleting them just to test out a hunch.
Proposed system solution:
The storage API should still return an object when a file is not found. This object should contain a 'not found' flag, and should not contain a remove() method, or the remove() method of said object could simply log the non existance of the file when the 'not found' flag is set.
The object could also reference a 'not found' image for use in the front end of the site in the case of images.
The advantage of this approach is that only the object definitions would need to be changed.
Alternatively, every instance of a file operation needs to have a check coded into it as per the hack above.
Further remedial action:
Having corrected this error, the system still failed to delete the user, this time throwing the following error
PHP Fatal error: Call to a member function getNextCollectible() on a non-object in /path/to/domain/application/modules/Core/Model/Item/Collectible.php on line 54
PHP Fatal error: Undefined class constant 'PRIMARY_TYPE_NUM' in /path/to/domain/application/modules/Core/Model/DbTable/Session.php on line 538
While I am not familiar enough with Social Engine to know exactly what collectible items are (Humbold figurines? Pokémon? I dunno...), it is the same error and can be fixed with a similar approach:
application/modules/Core/Model/Item/Collectible.php
54c54,56
< return $this->getCollection()->getNextCollectible($this);
---
> if(is_object($this->getCollection())){
> return $this->getCollection()->getNextCollectible($this);
> }
At this point the user could finally be deleted.
