Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate comment annotations to ElggComment objects (Trac #2146) #2146

Closed
elgg-gitbot opened this issue Feb 16, 2013 · 28 comments
Closed

Migrate comment annotations to ElggComment objects (Trac #2146) #2146

elgg-gitbot opened this issue Feb 16, 2013 · 28 comments

Comments

@elgg-gitbot
Copy link

Original ticket http://trac.elgg.org/ticket/2146 on 40330674-12-21 by cash, assigned to trac user juho.jaakkola.

Elgg version: 1.7

Comments and forum comments are annotations. Annotations do not have a container guid. If you want to do anything with just the comments and forum posts, you need to grab all the entities of a group and then grab all the comments on those entities. This is really slow and ugly.

Two solutions to this:

  • add container guid to the annotations table
  • promote comments and forum comments to ElggObjects - Maybe an ElggComment class. This would also support rating comments and everything else that comes with being an entity.
@elgg-gitbot
Copy link
Author

brettp wrote on 40342538-04-14

I'd like to see comments as ElggObjects. The benefits greatly outweigh the possible overhead of the light annotation (which is negated by the problems you mention above).

Another thing to consider is an enhanced ElggAnnotation class that is closer to--or is--an ElggEntity. I'm not thrilled with the current implementation of annotations because they are exactly the same as metadata with a few helper functions to do some math.

@elgg-gitbot
Copy link
Author

ewinslow wrote on 40473773-02-19

+1 for comments as ElggObjects.

I don't think making annotations into entities is necessary. I've thought the same before, but later realized that we're probably getting this feeling because annotations have been used for things that should be entities (like comments and forum posts). The solution, then, is to use Entities to represent those things, and leave the rest as annotations.

If anything, we should think about uniting metadata and annotations since they really are exactly the same in the database (all fields are the same). The thing that's different is the API for interacting with them.

To me it seems annotations are used more subjectively whereas metadata is more objective. I.e., annotations typically describe about the relationship between the annotater and the entity (E.g., rating a blog, creating a revision), whereas metadata is typically used for dynamically adding properties to an entity (E.g., the publish_date of the blog).

For what it's worth, I think this distinction b/w subjective annotations and objective metadata is useful.

@elgg-gitbot
Copy link
Author

Title changed from Support easy method to get comments and forum posts for a group to Migrate comment annotations to ElggComment objects by brettp on 40766304-08-04

@elgg-gitbot
Copy link
Author

cash wrote on 41009457-03-23

This is not going to happen for 1.8. I haven't had the time to take my design and write any code.

@elgg-gitbot
Copy link
Author

Milestone changed to Elgg 1.9 by cash on 41009457-03-23

@elgg-gitbot
Copy link
Author

ewinslow wrote on 41097216-04-20

Messageboard posts also warrant this change. Everything that has the potential to generate unique content that might be "likable" should be an entity.

@elgg-gitbot
Copy link
Author

Milestone changed to Elgg 1.9.0 by cash on 41784044-08-23

@elgg-gitbot
Copy link
Author

trac user juho.jaakkola wrote on 42438298-08-11

It hasn't been confirmed yet, but I may be starting a project in August that requires replacing the comment annotations with objects.

I'll be doing the project with Elgg 1.8, but maybe it could be possible to make a plugin for 1.8 and use the same code in core of Elgg 1.9. (I'm pretty sure 1.9 won't come out early enough so that I could use it instead of 1.8.)

Are there any specifications for the ElggComment yet? I would at least want to make my own implementation as close to the upcoming core functionality as possible so that migration to 1.9 would be easier.

@elgg-gitbot
Copy link
Author

trac user mrclay wrote on 42472941-01-20

Happy to help here. Is there a spec in progress?

@elgg-gitbot
Copy link
Author

cash wrote on 42473759-05-21

Three possible approaches:

  1. Simple upgrade to ElggObject - this would add support for liking, rating, etc. but not have explicit support for threaded comments.
  2. Upgrade to ElggObject and provide an API/UI for threaded comments - I have several ideas on how to represent the thread using containers, relationships, and metadata. I'll get a link out to that.
  3. Upgrade to a new comment type so it gets its own table. Two advantages here: 1) we can model the threading directly in the table rather than using relationships/metadata for better performance. 2) we can subtype the comment type and so support pulling back just generic comments or just forum posts. The above two options will make it difficult to distinguish between a comment on a blog and a reply to a forum post. If I wanted to search over forum posts (if they were implemented as ElggComments), I could not keep blog comments from being returned in any reasonable way.

Downside to adding a new type is 1) adding a new table 2) making it easier to justify adding additional tables :)

I suppose a third advantage of a table just for comments is that it divides the heavy data between the objects table and the comments table. I thought perhaps not inheriting from ElggObject would be a bad thing but that class has almost no methods.

@elgg-gitbot
Copy link
Author

trac user juho.jaakkola wrote on 42473908-03-02

So far approach #3 definitely sounds like the best option. But I'll be happy to see the threading ideas for #2 also.

(Besides I didn't get the project, so there's no need to implement anything that could be used also in 1.8 as a plugin.)

Btw, would there be any advantages if comments were a plugin in 1.9 instead of core functionality? (Although I suppose there would be only rare cases when anyone would want to actually disable commenting completely. Especially if there are other plugins that use subtyped comments.)

@elgg-gitbot
Copy link
Author

cash wrote on 42473942-08-12

Re: comments as plugin - Evan has talked about that. I think his primary motivation behind that is giving site owners control over the migration from annotations to entities. (Evan, correct me if I'm wrong here.)

I'm hesitant to do the upgrade as a plugin because

  1. it means either a duplicate set of views/actions for comments or scripts to migrate back and forth from annotations to entities (or it is a one way upgrade and if you turn off the plugin, you're screwed)
  2. plugins that involve comments may have to exclusively support either annotations or entities (more likely that everyone moves to entity-based comments). It's easier for plugin authors if they know whether comments will be an entity or annotation.

@elgg-gitbot
Copy link
Author

cash wrote on 42474012-12-04

Everyone on this thread should have access to the Google doc on Elgg Comments (it may have been shared a while ago with you).

I was wrong about Evan's reasoning re: comments plugin - it's not about the upgrade but about being able to disable commenting.

@elgg-gitbot
Copy link
Author

trac user juho.jaakkola wrote on 42478647-02-25

Should I have received a link to the Google doc to my email?

@elgg-gitbot
Copy link
Author

trac user juho.jaakkola wrote on 42599594-07-17

Couple of questions:

  • What would be a good name for the relationship between a comment and the container of the object that was commented? (This relationship was in the spec)
  • In elgg_river table should the subject_guid be the comment instead of the user who commented? The user could then be retrieved in the river view with $comment->getOwnerEntity().

@elgg-gitbot
Copy link
Author

trac user juho.jaakkola wrote on 42599833-05-08

No wait... probably more logical in elgg_river would be to have user as subject and comment as object and then get the commented entity with $comment->getContainerEntity()?

@elgg-gitbot
Copy link
Author

ewinslow wrote on 42604261-04-01

inReplyTo should be the relationship for threading purposes.

@elgg-gitbot
Copy link
Author

trac user juho.jaakkola wrote on 42604463-06-03

I don't think that the relationship I mentioned is meant for threading.

My guess is that it would be used for being able to search for comments for example inside a group without having to loop through all the contents. Is this correct?

@elgg-gitbot
Copy link
Author

trac user juho.jaakkola wrote on 42621332-05-20

Here's the current state of my comments branch compared to master:
juho-jaakkola/Elgg@master...comments

I would like to get some feedback about the code.

@elgg-gitbot
Copy link
Author

trac user juho.jaakkola wrote on 42621344-05-12

Oh, and here's a demo site that uses the comments branch: https://demo.mmg.fi/elgg19/

Feel free to register and try it out.

@elgg-gitbot
Copy link
Author

trac user mrclay wrote on 42621993-05-30

This is looking really good. Go ahead and send a PR. I forgot we can't get line-level comments from the compare view on forked branches, has to be a PR.

@ewinslow
Copy link
Contributor

@juho-jaakkola what's the latest on this?

@cash
Copy link
Contributor

cash commented Feb 24, 2013

Waiting on river API/table changes to be pulled in.

@juho-jaakkola
Copy link
Member

@mrclay You mentioned you could test the upgrade script with community site data. Have you tried this?

If there's too much data for the default upgrade feature we should decide what is the best way to handle the migration.

@mrclay
Copy link
Member

mrclay commented Mar 21, 2013

Haven't tried, but took a LONG time on a low-traffic site with a few dozen users.

I think the clear need is to have the installer only convert the latest day or so of comments then provide an admin page that makes successive XHR requests to replace the rest from latest to oldest.

@ewinslow
Copy link
Contributor

Data migrations are my favorite...

We need some way to sanity check whether things are happening correctly. There's no visibility into the upgrade process at present and that definitely caused problems when we upgraded the community to 1.8.

@juho-jaakkola
Copy link
Member

Would something like #4975 require too much work to make it into 1.9?

@ewinslow
Copy link
Contributor

!_! just like that... woot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants