More consistent RDFa logic for username

+++ modules/rdf/rdf.module	2009-10-26 03:32:57 +0000
@@ -434,46 +438,37 @@ function rdf_preprocess_user_profile(&$v
 function rdf_preprocess_username(&$variables) {
...
+  if (!empty(user_load($account->uid)->rdf_mapping)) {
+    $rdf_mapping = user_load($account->uid)->rdf_mapping;

I guess the condition should be empty(), not !empty().

Also, you cannot use functions in empty(). What you want is probably:

if (empty($account->rdf_mapping)) {
  $rdf_mapping = user_load($account->uid)->rdf_mapping;

+++ modules/rdf/rdf.module	2009-10-26 03:32:57 +0000
@@ -434,46 +438,37 @@ function rdf_preprocess_user_profile(&$v
+    if ($account->uid != 0) {

Please test > 0 here.

+++ modules/rdf/rdf.module	2009-10-26 03:32:57 +0000
@@ -434,46 +438,37 @@ function rdf_preprocess_user_profile(&$v
+    // Annotate the user name in RDFa. The attribute 'property' is used here
+    // because the user name is a literal.
+		$variables['attributes_array']['property'] = $rdf_mapping['name']['predicates'];

Strange indentation.

+++ modules/rdf/rdf.module	2009-10-26 03:32:57 +0000
@@ -434,46 +438,37 @@ function rdf_preprocess_user_profile(&$v
+      // The rel attribute added by template_preprocess_username() is a string.
+      // @todo Fix template_preprocess_username() to use an array instead.
+      $variables['link_attributes']['rel'] = array_merge($rdf_mapping['homepage']['predicates'], array($variables['link_attributes']['rel']));

huh? Can the 'rel' attribute really take multiple values?

+++ modules/user/user.module	2009-10-26 03:41:56 +0000
@@ -3269,6 +3269,10 @@ function user_rdf_mapping() {
+        'homepage' => array(
+          'predicates' => array('foaf:page'),
+          'type' => 'rel',
+        ),

The "homepage" property actually belongs to anonymous users only and to comment module only.

Is it a good idea to add it to the RDF mapping for users?

I'm on crack. Are you, too?

+++ modules/rdf/rdf.module	2009-10-26 03:32:57 +0000
@@ -434,46 +438,37 @@ function rdf_preprocess_user_profile(&$v
+  // Check whether RDF mappings are available for the user bundle. Since the
+  // full user object is not available in $variables, it needs to be loaded to
+  // get the proper RDF mapping.
+  if (!empty(user_load($account->uid)->rdf_mapping)) {
+    $rdf_mapping = user_load($account->uid)->rdf_mapping;

Even with the comments which are nice, it's still confusing (maybe, it's just me; I still can't seem to wrap my head around how RDF actually works). I wonder if #585838: Consider a generic $entity->user property will end up helping make this less weird.

+++ modules/rdf/rdf.module	2009-10-26 03:32:57 +0000
@@ -434,46 +438,37 @@ function rdf_preprocess_user_profile(&$v
-      // The first thing we are describing is the relation between the user and
-      // the parent resource (e.g. a node). Because the set of predicate link
-      // the parent to the user, we must use the 'rev' RDFa attribute to specify
-      // that the relationship is reverse.
-      if (!empty($account->rdf_mapping['uid']['predicates'])) {
-        $variables['attributes_array']['rev'] = $account->rdf_mapping['uid']['predicates'];
-        // We indicate the parent identifier in the 'resource' attribute,
-        // typically this is the entity URI. This is the object in RDF.
-        $parent_uri = '';
-        if (!empty($account->path['source'])) {
-          $parent_uri = url($account->path['source']);
-        }
-        elseif (!empty($account->cid)) {
-          $parent_uri = url('comment/' . $account->cid, array('fragment' => 'comment-' . $account->cid));
-        }
-        $variables['attributes_array']['resource'] = $parent_uri;
-      }

For the benefit of other reviewers, as I understand it, this is the main benefit of this patch. This current code in HEAD is some weird RDF trickery and it would be nice to see this go away. However, the cost of having this go away is the extra code added to rdf_preprocess_node() and rdf_preprocess_comment(). In my opinion, the code added in those functions make more sense than the code being removed, so I'm in favor of this. However, from a markup standpoint, this results in an extra span tag around the comment and node author. It's a span wrapper that only gets rendered if the rdf module is enabled, and a themer has access to it via the 'rdf_template_variable_wrapper' theme hook, so it's about as harmless as it can be, but the question needs to be asked: does anyone consider an extra span wrapper to be worse than the RDF trickery needed to avoid it? I don't, but I'm not sure I'm the best judge of that.

+++ modules/rdf/rdf.module	2009-10-26 03:32:57 +0000
@@ -434,46 +438,37 @@ function rdf_preprocess_user_profile(&$v
+      // The rel attribute added by template_preprocess_username() is a string.
+      // @todo Fix template_preprocess_username() to use an array instead.
+      $variables['link_attributes']['rel'] = array_merge($rdf_mapping['homepage']['predicates'], array($variables['link_attributes']['rel']));

Given how template_preprocess_username() currently is, this code makes sense. But yes, we might want to make changes to template_preprocess_username() to take into account that "rel" can be an array and can have semantic meaning in addition to a hint for search spiders.

I'm on crack. Are you, too?

Thanks! It's getting cleaner and clearer.

+++ modules/rdf/rdf.module	2009-10-29 06:21:17 +0000
@@ -434,46 +438,40 @@ function rdf_preprocess_user_profile(&$v
+  $rdf_mapping = user_load($variables['account']->uid)->rdf_mapping;
+
+  if (!empty($rdf_mapping)) {
+    $rdf_mapping = user_load($account->uid)->rdf_mapping;

The line above this is $account = $variables['account'], so I'm not clear why you need to user_load() both.

This review is powered by Dreditor.

As mentioned in the @todo of this patch, the work here uncovered a shortcoming in theme_username() preprocessing. See #623592: Fix theme_username() preprocessing to work with attributes that are arrays.

+++ modules/rdf/rdf.module	2009-11-04 22:09:55 +0000
@@ -434,46 +438,38 @@ function rdf_preprocess_user_profile(&$v
   $account = $variables['account'];
-  if (!empty($account->rdf_mapping['name'])) {
-    if ($account->uid != 0) {
-      // The following RDFa construct allows to fit all the needed information
-      // into the a tag and avoids having to wrap it with an extra span.
-
-      // An RDF resource for the user is created with the 'about' attribute and
-      // the profile URI is used to identify this resource. Even if the user
-      // profile is not accessible, we generate its URI regardless in order to
-      // be able to identify the user in RDF.
+
+  // Get the RDF mapping of the user. Since $variables['account'] contains the
+  // mapping of the node or comment being rendered, we need to load the user.
+  // We do not directly use rdf_mapping_load() here because some modules might
+  // want to alter the RDF mapping of a given user via hook_user_load().
+  $rdf_mapping = user_load($variables['account']->uid)->rdf_mapping;

Ok, now I understand. So, we don't actually need anything from $variables['account'] because it's a fake account object. Instead, we want the real account object for the user, so:

$account = user_load($variables['uid']);
if ($account) {
$rdf_mapping = $account->rdf_mapping;
}

+++ modules/rdf/rdf.module	2009-11-04 22:09:55 +0000
@@ -434,46 +438,38 @@ function rdf_preprocess_user_profile(&$v
+    if (!empty($account->homepage) && !empty($rdf_mapping['homepage'])) {

s/$account->homepage/$variables['homepage']/

This review is powered by Dreditor.

As a measure of precaution, we keep as much information as possible from the original $variables...

Yes, I wasn't suggesting changing $variables['account'], but setting the local $account variable to the object needed. We don't want to assume that user_load() will succeed; it might return FALSE, in which case, accessing a property will trigger a warning. I hope this patch helps clarify.

This patch also includes minor cleanup and the necessary change to template_preprocess_username() and template_process_username().

This patch includes a small polish that supports the possibility that earlier hook_preprocess_username() implementations added to the attributes that can be arrays, and that rdf should add to these rather than clobber them. The use-case for this would be esoteric, because these are RDFa specific attributes, but who knows what other doctypes people might want to work with, and perhaps some of those doctypes re-purpose these attributes the way that RDFa re-purposes the "rel" attribute.

@sun: since you're the only subscriber to this issue other than scor and me, can you RTBC this, or provide feedback, or suggest who else should review it? Thanks.

+++ includes/theme.inc
@@ -2023,7 +2026,14 @@ function template_process_username(&$variables) {
+    // $variables['attributes_array'] contains attributes that should be applied
+    // whether a link is being rendered or not. $variables['link_attributes']

I think we're missing a "regardless of" before "whether" here.

+++ includes/theme.inc
@@ -2023,7 +2026,14 @@ function template_process_username(&$variables) {
+    // rendered. Preprocess functions are encouraged to use the former unless
+    // they are adding attributes that would be senseless to place on a
+    // non-link. If a link will be rendered, these need to be merged. Some

"senseless"? Example?

+++ modules/comment/comment.test
@@ -881,36 +881,101 @@ class CommentRdfaTestCase extends CommentHelperCase {
+    // Enabled anonymous user comments.

s/Enabled/Enable/

+++ modules/comment/comment.test
@@ -881,36 +881,101 @@ class CommentRdfaTestCase extends CommentHelperCase {
+    // Allow anonymous to leave their contact information.
+    $this->setCommentAnonymous('1');
+    $this->setCommentPreview(DRUPAL_OPTIONAL);

1? Isn't this a constant as well?

+++ modules/comment/comment.test
@@ -881,36 +881,101 @@ class CommentRdfaTestCase extends CommentHelperCase {
+  function _testBasicCommentRdfaMarkup($comment, $account = array()) {
...
+    $this->assertEqual((string)$comment_author[0], $name);
+    }
 }

Wrong indentation here.

+++ modules/comment/comment.test
@@ -881,36 +881,101 @@ class CommentRdfaTestCase extends CommentHelperCase {
+    // The author tag can be either a or span

Let's write HTML tags always uppercase.

+++ modules/rdf/rdf.module
@@ -402,6 +402,10 @@ function rdf_preprocess_node(&$variables) {
     $date_attributes_array = rdf_rdfa_attributes($variables['rdf_mapping']['created'], $variables['created']);
     $variables['rdf_template_variable_attributes_array']['date'] = $date_attributes_array;
...
+    $variables['rdf_template_variable_attributes_array']['name']['rel'] = $variables['rdf_mapping']['uid']['predicates'];

This really screams for a clean-up. Why do we have a template variable with the term "template_variable" in it? Shouldn't we also consider to build a single $variables['rdf'] array that contains the keys 'attributes' + 'attributes_array' keyed by other variables - or similar?

Doesn't belong into this issue, so let's create a follow-up for that and link to it here.

+++ modules/rdf/rdf.module
@@ -440,48 +444,46 @@ function rdf_preprocess_user_profile(&$variables) {
+  // the real account object for the user is needed. The real account object is
+  // needed for this function only; it should not replace $variables['account'].
+  if ($account = user_load($variables['uid'])) {

Not sure why we wouldn't want to overload the existing variable when we have to load the full user account anyway. Of course, the user should always be fully loaded in the first place, but that's a different issue... ;)

So, yeah, not sure why we don't pass on the "proper" data if we have to load it anyway.

+++ modules/rdf/rdf.module
@@ -440,48 +444,46 @@ function rdf_preprocess_user_profile(&$variables) {
+    // Annotate the user name in RDFa. The attribute 'property' is used here
+    // because the user name is a literal.
+    if (!empty($account->rdf_mapping['name'])) {

Docs-only issue: Why is the user name considered literal, if we have a mapping for the user name?

Or what exactly is meant by "literal"? ;)

I'm on crack. Are you, too?

+++ modules/rdf/rdf.module
@@ -440,48 +444,46 @@ function rdf_preprocess_user_profile(&$variables) {
 function rdf_preprocess_username(&$variables) {
...
+  // $variables['account'] is a pseudo account object, and as such, does not
+  // contain the rdf mappings for the user; in the case of nodes and comments,
+  // it contains the mappings for the node or comment object instead. Therefore,
+  // the real account object for the user is needed. The real account object is
+  // needed for this function only; it should not replace $variables['account'].
+  if ($account = user_load($variables['uid'])) {

Hrm, no, I actually meant

$variables['account'] = user_load($variables['uid']);

i.e. load the full user account, use it onwards, but also provide it to others.

Additionally, we don't have to user_load() again, if $variables['account'] is a full user object already.

However, I'm trying to figure out whether we have any indicator for that...

This review is powered by Dreditor.

This looks good to me (I didn't review the tests in detail, but the code changes look good). I agree that we need two follow-up issues outside the scope of this patch: 1) The whole $account parameter passed to theme('username') being a pseudo-account object, and where, if anywhere it's appropriate to replace it with the result of a user_load() -- wherever is the right place to do that, I'm pretty confident that it's not in the rdf module. 2) Cleaning up the naming convention for $variables['rdf_template_variable_attributes_array'] (for reference, this name was introduced in http://drupal.org/node/493030#comment-2168566).

I'd still like @sun to give his approval before we mark it RTBC. What do you think, sun? Do you have any concerns that haven't been adequately addressed?

I'm fine with this patch. user_load() is cached now, and since we have this issue since like forever, there's no reason to hold off this patch. ;)

Please link to the two (unrelated) follow-up issues in here.

Committed to CVS HEAD. Thanks!

As requested, linking to #633642: Rename $variables['rdf_template_variable_attributes_array'] to something sane. Will add link to other follow-up issue regarding $variables['account'] when it's posted.

And the other one: #633652: What should $variables['account'] within theme_username() be?