Allow any entity to be the owner of an ElggFile

[coldtrick]

Why must ElggFiles have a ElggUser as owner?

for example:
I want to extend the Blogs to allow upload of an image. And I want the image to be owned by the Blog this is impossible.

Other example:
The Group icon is stored with the group owner (a user) however if I transfer ownership to a different user I also have to move the icon to this new user.
If the icon was owned by the Group this wouldn't be any problem.

Please remember that this is just a question, not a bug (yet ;) I would like to know why this choice was made.

[brettp]

This doesn't exactly fail gracefully, does it...

Pure speculation, but I'd imagine the original decision was made because the file store matrix was created based upon username. Since 1.7 the matrix is based upon time created and entity guid, so that is no longer an issue. I think this is something worth investigating...

[brettp]

At this point only supporting ElggUser is a bug. Will require removing the username checks in ElggDiskFilestore.

[ewinslow]

Why not use container_guid instead? I think owner_guid is useful for tracking which user was responsible for creating the entity in the first place. Overriding owner_guid like this would force us to lose that information. Maybe we need a creator_guid?

[brettp]

container_guid doesn't solve the problem of User creates group w/ icon, different user takes over as group admin, original user deleted, group icon deleted. Using a metadata creator_guid on the ElggFile would help to solve some of this, but we still have to set the owner to something.

I admit the blog example is a bit strange, and container_guid is probably a better solution in that case.

I think allowing any entity to own an ElggFile solves the the most common problems encountered. This makes the most sense in Elgg's data model, too.

[ewinslow]

The "delete everything owned by the user when the user is deleted" behavior seems to be causing problems elsewhere as well. See #1250. Are we sure this is what we want?

[brettp]

Recursive deleting solved more problems that would result from entities being owned by users that don't exist. In the ticket you referenced, the solution is also to set the owner_guid to something other than a user. There will likely be fewer cases when the owner needs to be something other than a user than cases where we want to delete all entities owned by a user when deleting that user.

For this ticket in particular, when the group/blog post is deleted, it will also delete the file it owns. I think this makes sense.

[ewinslow]

Ok, sounds good to me.

[brettp]

Who ever implements this remember to remove the check for $entity->username. See #3582.

[Cash Costello]

Refs #2290 removes check for username

Changeset: 3a58c21904d22c0e2f80891f265ba4d687f838e3

[cash]

Just uploaded a file with a blog as owner and it works. If the subtype is not changed from 'file' it will show up on the all files page. There are display issues because the owner is not a user. That's a separate issue and one we're unlikely to change. If you subclass ElggFile and use a different subtype, everything should work and it won't show up in the list pages.

[ismayil.khayredinov]

Sorry to bring this up so late, but just saw this. Are you suggesting that I can no longer expect an owner to be a user and need to run an additional check to make sure I don't run into a fatal error by trying to run an ElggUser method? I think this creates all kinds of complications and sets a troubling precedent.

[cash]

Elgg has never required that the owner_guid of an entity belong to an ElggUser. There was a unique check in the filestore code for users owning files that was removed here.

Right now, we're running with an implicit contract that owners will be users. If I created a blog that was owned by another object, all sorts of things would start breaking. Most likely this would result in a WSOD on any page where the blog was mentioned.

Options:

• Continue with the implicit contract, but document that developers need to be careful breaking that contract. This ticket is an example. ColdTrick should use a different subtype if they want a file owned by a blog. Otherwise, it will break the page listing all objects of type 'file'.
• Make the contract explicit. Only users can own entities. "Owned by" relationships could be used to tie entities to non-user entities.
• Remove the contract and make sure that all code interacting with owners expects any type of entity. This could include using the __call() magic method to handle unimplemented methods.
• Make the contract explicit/implicit on an object by object basis. For example, the ElggBlog object could enforce that its owner has to be a user.

I'll also note that this issue already exists with containers. There are places in the code where we test whether the contain is a group or a user and take different actions based on that.

Definitions: implicit means there is no code forcing this. Explicit means there is code enforcing it. Things can be documented but still be implicit as I'm using the word here.

[brettp]

My preference is to remove the contract and allow non-users to own files. There are use cases for this (group icons) and I think defensive coding by plugin authors is a good thing. Cash named other systems that require additional checks, so it's a good habit to be in to check what sort of entity you have before calling methods. 1.8's elgg_instanceof() helps with this.

[cash]

Brett - I think Ismayil is talking about any arbitrary ElggObject, not just ElggFile. Right now, we don't stop an ElggObject from owning a a blog post but it would crash the blog plugin. Should we be adding defensive code to every use of the owner for blogs?

[ismayil.khayredinov]

I didn't actually realize that a non-user can own an object. My initial question was about ElggFile, but yes, it has now become much broader and scarier. This suggests that any plugin available in the core and outside of it needs to be rewritten as to avoid crashing the site. What happens if I unknowingly or accidentally assign a non-user as an owner of an object in a plugin? I would basically have to go and manually change the owner in the database (if that's even possible identifying which object has a non-user assigned in tens of thousands of rows), because none of the core plugins actually checks for who the owner is (though I do agree that it's a good practice to use elgg_instanceof()).

I think No. 4 on Cash's list is the way to go, though I would reverse it. By default, enforce that an owner of any given object subtype is a user, and provide a possibility for a specific subtype to allow non-users as owners. After all, I think what ColdTrick is trying to do is a very exceptional case.

[Cash Costello]

Refs #2290 removes check for username

Changeset: 3a58c21904d22c0e2f80891f265ba4d687f838e3