Consider a generic $entity->user property [#585838]

Entity loader broke the comment publishing status.

The query joins on {user} to fetch user data and the user's status.

Both comments and users have a status. The comment status is always overridden by the user's status.

I've encountered this major flaw a couple of times in the meantime. With the entity loader, the underlying API design flaw becomes even more apparent. The real solution would be to have:

$comment (object)
$comment->cid
$comment->subject
$comment->comment
$comment->status
$comment->name [anonymous comments]

$comment->user (object)
$comment->user->uid
$comment->user->status
$comment->user->name

And that we need also for nodes and all the other stuff. Historically, we only have these ambiguous object properties to make simple list queries faster - which has always been a pain for developers and themers, because you never really know whether you get a half-baked object, which is a different object in reality, or whether it is a proper, fully loaded object of the assumed entity type.

For example, that is the reason for why you can't theme usernames differently depending on their status: theme_username() gets an arbitrary $object. And if you'd try to theme users differently based on $object->status, then you are theming a user based on the status of some arbitrary other object in reality.

This patch fixes comment publishing, which is completely broken currently. Which also means that there are no tests for comment moderation - or - those tests rely on comment_load() instead of the real database values, or whatever...

Comment	File	Size	Author
#5	drupal.comment-status.5.patch	1.81 KB	sun
#5
#2	drupal.comment-status.2.patch	2.08 KB	sun
#2
	drupal.comment-status.patch	2.19 KB	sun

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

catch

he/him

English

CreditAttribution: catch commented 23 September 2009 at 19:07

We moved node user fields into user_node_load() and removed the join - should consider doing same for comments. We should also consider whether to change these from direct queries to real user_load_multiple() calls as well - this assumes modules don't do crazy things like load entire nodes into the $user object though.

I'm pretty sure there's some tests for comment publishing - so must be incomplete rather than completely missing.

Comment #2

sun

German

Karlsruhe

CreditAttribution: sun commented 24 September 2009 at 17:04

File	Size
drupal.comment-status.2.patch	2.08 KB

change these from direct queries to real user_load_multiple() calls

That is what I think is badly needed, as mentioned in the OP. The new *_load_multiple() entity loaders should allow to do this quite fast. However, we'd have to move the $user object into a single property of the parent $object, i.e. $object->user->..., as also mentioned above.

Should we get the existing patch committed first? Removed the 'status' completely, like in user_node_load().

Comment #3

sun

German

Karlsruhe

CreditAttribution: sun commented 9 October 2009 at 13:05

Issue tags:

+D7 API clean-up

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

Comment #4

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 9 October 2009 at 13:25

This chunk:
if (!$user->uid && variable_get('comment_anonymous_' . $node->type, COMMENT_ANONYMOUS_MAYNOT_CONTACT) != COMMENT_ANONYMOUS_MAYNOT_CONTACT) {
- $form_state['#attached']['js'][] = drupal_get_path('module', 'comment') . '/comment.js';
+ $form['#attached']['js'][] = drupal_get_path('module', 'comment') . '/comment.js';
}

Is also being fixed in #440876: Reuse comment.module's anonymous cookie information fyi.

Comment #5

sun

German

Karlsruhe

CreditAttribution: sun commented 9 October 2009 at 16:06

File	Size
drupal.comment-status.5.patch	1.81 KB

Re-rolled against HEAD.

Comment #6

catch

he/him

English

CreditAttribution: catch commented 9 October 2009 at 16:26

Status:

Needs review

» Reviewed & tested by the community

Fine patch. RTBC.

Comment #7

Dries CreditAttribution: Dries commented 9 October 2009 at 17:06

Status:	Reviewed & tested by the community	» Needs work
Issue tags:		+Needs tests

Committed the quick fix in #5, but (i) we'll want tests and (ii) I agree that we'll want a bigger architectural fix (and API clean-up).

Comment #8

sun

German

Karlsruhe

CreditAttribution: sun commented 9 October 2009 at 17:17

Title:	{comment}.status broken	» Mixing of object properties breaks entity loader
Component:	comment.module	» base system

Thanks! Trying to find a better title, corrections welcome.

Comment #9

sun

German

Karlsruhe

CreditAttribution: sun commented 27 October 2009 at 18:18

To move forward here, we need to agree on a possible workaround and/or solution, and a target and time-line.

I'd guess that all we could do is to move those user properties into $object->user, and I actually think that it wouldn't be _that_ much that needs to be changed. Perhaps it can even be done with some regex magic.

For now, I'm not arguing for loading entire/proper objects. We can still load/attach that fake user data, but if we'd move it into a dedicated property, then at least a contrib module would be able to lazy-load a full user object, replacing that $object->user, without hi-jacking the node's or comment's or whatever's properties.

Thoughts?

Comment #10

effulgentsia CreditAttribution: effulgentsia commented 27 October 2009 at 23:17

Comment #11

catch

he/him

English

CreditAttribution: catch commented 28 October 2009 at 02:25

We already have a user_node_load(). Ideally I'd like to see us do user_comment_load() an stick it into a $comment->user property. That also might possibly be faster with very, very large datasets.