RDF #1: core RDF module

I'm don't feel qualified for extensive review and feedback but
- taking the <h1> out of the hands of templaters sounds a pity (same goes for the node RDF patch)
- since rdf attributes are attached to 'bundles', the work here might be interested in and affected by #470242: Namespacing for bundle names

Detail: back in the FiC sprint, we picked the word 'bundle' as 'bundle (or group) of field instances' - abstracts over 'node type' in a world were not only nodes are fieldable. I don't know if the word 'bundle' is still relevant if other concepts start building upon it.
(but I sure hope so, because moving the whole Field API to a different word would sure be tedious :-)

I have no time right now to write a detailed response, but here are a few thoughts I had while reading the patch and previously the groups wiki page.

1) I like the general concept of the patch. Putting rdf mappings into the objects and having hooks for the actual mappings looks good.
2) What I don't like, though, is the way RDFa is passed into the theming layer currently. IMO it puts far too much work into preprocess functions and templates. What I would propose instead is integrating it into drupal_render() or at least doing it in a similar style. In node_build(), we would add $node->rdf_mapping as a property $node->content['#rdf_mapping']. drupal_render() could iterate over #rdf_mapping if it is set on an element and if the element has a child by the name as a key of #rdf_mapping, its #rdf property would be set accordingly (this means that a node's title would have to be a child of $node->content, this is not a problem at all though).
Then, we'd add a function render_rdfa($element) that would be used directly inside templates to get the rdfa property of an element.

So in drupal_render that'd be something like this:

if (isset($element['#rdf_mapping'])) {
  foreach ($element['#rdf_mapping'] as $key => $data)) {
    if (isset($element[$key])) {
       $element[$key]['#rdf'] = $data;
    }
  }
}

and in node.tpl.php

<h2 <?php render_rdfa($content['title']); ¿>><?php render($content['title']); ></h2>
// or, to get rdfa properties that don't relate to an element in content:
<div id="node-..." <?php render_rdfa($node, 'rdftype'); ¿> >

This means render_rdfa() would either just use the element's #rdf property (if it was set by drupal_render) or otherwise use the element's rdf_mapping property and return the rdfa string for the rdf property passed in as second argument.

This is very rough, I'll try to think about the details some more. But this would mean much much less burden on theme developers to integrate rdfa.

Subscribing.

'bundle' != 'entity type' :
'entity type ' is 'node', 'user', 'taxo term' ...
'bundle' is 'article', 'page' (for nodes), 'vocab_1', 'vocab_2' (for taxo terms)...

I think an 'entity type'-level granularity is not enough for RDF mappings, right ?

>'entity type ' is 'node', 'user', 'taxo term' ...
>'bundle' is 'article', 'page' (for nodes), 'vocab_1', 'vocab_2' (for taxo terms)...

Hm, also a node type can be seen as entity type, it's just even more specialized - but let's stop nitpicking about that ;) However the term 'bundle' currently relates only to field-API bundles, but the rdf module isn't restricted to that. Thus another term might fit better.

One question:
dc:creator is assigned to $node->uid, however does it generate the right URI for the actually referenced user?

In your static html page example there is it like that:

<span class="submitted">Submitted by <span rel="sioc:has_creator dc:creator"><span property="foaf:name" typeof="sioc:User" about="/d701/user/1#this">admin</span></span> on <span property="dc:created" datatype="xsd:dateTime" content="2009-05-11T10:32:40+00:00">Mon, 05/11/2009 - 10:32</span></span>
  

Do the current patches already work that way? How is the uid transformed to the url? I wasn't able to find that part.

I think you didn't understand me right - I'm talking about the (rdf) reference to a user, when he authored a node. Does it actually reference this URI?

docs for hook_rdf_mapping_alter aren't correct

+
+// $Id: $

no newline before and it's $Id$ not $Id: $

function rdf_get_mapping

Don't you want to static cache mappings? and call the internal variable mappings not mapping?

+function rdf_preprocess_page(&$variables) {
+	// dsm(array("rdf_preprocess_page" => drupal_get_rdf_page_mapping()));
+}

ummm

+  $variables['rdfa_page_about_typeof'] = '';
+  $variables['rdfa_page_title'] = '';

strange variable names

The TODOs in theme.inc seem pretty necessary.

Minor, but there's now a http://api.drupal.org/api/function/user_node_load/7 which replaces the join in node_load_multiple() - if user.module is providing RDF mappings in user_load() might make sense to put the node stuff there.

I started working on #551406: Centralize entity properties meta data which this patch could build upon once it's there. Thus hook_rdf_mapping() could be changed to be fold into hook_entity_info() once it's there. Anyway as for token, this should be easy to change afterwards too and need not happen in the first run.

rerolling patch.

We could consider adding database caching for node types? How was the mapping being added to node types before changing the schema?

Just glanced over the patch, but the them('username') changes look a bit heavy - no other way to get it in there?

Responding to catch's comment #27 and his comment on another issue: http://drupal.org/node/553038#comment-1991256.

For anyone here who doesn't already know, #400292: Allow preprocess functions for theme hooks implemented as functions landed in HEAD 2 weeks ago. With respect to minimizing the change needed to theme_username(), this means:

• The customization wanted to $name can be achieved with a rdf_preprocess_username() function that clones $variables['object'] and alters the 'name' property of the cloned object (I'm assuming cloning is needed to prevent altering the global $user object with some invocations).
• The customization needed on the link attributes can be solved by adding $link_attributes as a optional 2nd argument of the username theme hook within drupal_common_theme() and then refactoring theme_username() to add the ones it wants as defaults ('title', 'rel') either within the function, or with a template_preprocess_username() function. With this in place, rdf_preprocess_username() can add its part ('about', 'html').
• That would only leave the desire to surround the output with another span tag (the one with rel=implode(' ', (array)$object->rdf_mapping['uid'])). One solution would be to lobby for #553038: Add postprocess phase for theme(), which needs a convincing use-case to justify it. Another solution would be to have rdf_theme() route the 'username' theme hook to its own function (e.g., rdf_username()) which could call theme_username() but then surround it with the extra span tag. However, this would mean that any theme that wants to have a THEMENAME_username() function would need to build in a check for the rdf module and output that span or not. A final solution would be to make sure all uses of outputting a themed username go through the render() function, so that the rdf module can add a #theme_wrappers function as a way of getting that span in. However, I doubt that's feasible to do before code-freeze. My own bias is to lobby for #553038: Add postprocess phase for theme() but I need help with that, so please add your thoughts to that issue.

delta patch to add other attributes to each block

try again with added isset/empty to avoid exceptions

ignore that one - here's the right one.

if rdf module wants to be able to add to #atributes on pre-process, the drupal_attributes call ahs to be later I think, something like this.

FYI: I added #565792: Refactor theme_username so that RDF patch can be accepted in case it's desired to split off theme_username refactoring from the rest of the patch. Note the implementation in that issue is different (and in my opinion improved) than the one in the patch on comment #38.

Per discussions yesterday, etc - I really think it' a bad pattern to add wrapper div elements - as proposed for the block module we should add generic variables to the outer elements in the templates that can contain any needed attributes that any module would want to add.

subscribe

recieved some PHP notices on the current implementation.

    *  Notice: Array to string conversion in rdf_preprocess_username() (line 237 of /home/ber/Documenten/DPL_RDF_in_core/unfuddle/drupal/modules/rdf/rdf.module).
    * Notice: Array to string conversion in rdf_theme_username() (line 270 of /home/ber/Documenten/DPL_RDF_in_core/unfuddle/drupal/modules/rdf/rdf.module).
    * Notice: Array to string conversion in rdf_preprocess_node() (line 169 of /home/ber/Documenten/DPL_RDF_in_core/unfuddle/drupal/modules/rdf/rdf.module).

A dvm() learned me that rdf_preprocess_username() expects $object->rdf_mapping['name'] to be an array with strings, insted of the current nested array (['name']['property'] = 'foo:bar'

It's a little weird not having enabled the rdf.module to see all RDF related namespaces in HTML node/1

- should this be only when node/1/rdf is requested?
- _and_ only when rdf.module is enabled?

Added rdf_preprocess_field() and a simpletest for rdf properties being rendered.

Added correct title property (Array is now dc:title and implode is deleted).

Changed return value of drupal_rdfa_attributes to array.

Patch in #49 is reverting ie bootstrap.inc, form.inc, session.inc

Rerolled patch.

FYI: I opened a separate issue related to theming: #569362: Document $attributes, $title_attributes, and $content_attributes template variables. This is a generalization of what pwolanin did in #40.

And with the correct patch.

Patches do not apply because our bazaar repo is out of sync with CVS HEAD. Do not submit patches before this is fixed. Please help fixing it!

scor, I think you posted the wrong patch. Do you have already re-rolled this patch?

@previous patch:
Watch out for inline comments to be capitalized sencentence with punctuation, e.g. in drupal_get_rdf_namespaces(). Also the leading space is missing there.

as said, I took this patch and ported it to make use of the centralized entity properties #551406: Centralize entity properties meta data. That way we have a unified backend for rdf and token metadata, and possible lots others like rules2.

Changes:
• The RDF mapping is added to the property-info of hook_entity_info(), so the separate hook is gone.
• As RDF mappings can be added to the specified properties there, there is no need for per-entity storage of mappings any more -> simplifying things. Just each module providing properties can specify a mapping, or of course any module could do by using hook_entity_info_alter().
• For specifying a RDF mapping for the current page a page entity got introduced, which defines the default mapping. For altering that rdf_alter_page_mapping() is used. Also the uri of a page can be altered by altering $page['#uri'].
• Fixed namespacing of all functions in rdf.module.
• Made use of the new attributes_array variables. That way it's easy to add in RDFa attributes without having $rdfa_title_attributes variables or similar, which are odd as they might not be set when rdf is disabled...
• For now I documented the attributes added to hook_entity_info for describing the rdf mapping by using the function rdf_entity_info_attributes() - as it adds stuff to hook_entity_info(). Not sure what's the best way to document this.
• We can use hook_property_info() to define default rdf mappings per property type, as I did for dates.
• As previously, there is a function to get the rdfa properties: rdf_attributes(); which makes use of the property wrapper. Thus it automatically makes use of the specified mappings in hook_entity_info().
• The patch implements node and user attributes, as well as field support. All that is gone into rdf.module, which I think makes sense. So rdf.module comes with mappings for core, but contribs should provide their rdf mappings on their own.
• Fixed the node teaser output to have "typeof" and "about" attributes set.

Questions:
• I just converted the user integration as it was before, however theme_username() now just puts out the foaf:name. But in the node context that would end up as property of the node, which doesn't seem correct to me.

That's it. Opinions?

Well, I think having a unified backend makes a lot of sense and would help us to easily get more RDF, as we would just need to attach mappings to provided properties/tokens. Also that way I think it's simple to reuse this for the generation of usual RDF (not RDFa) (in contrib), as the property wrapper also provides a way to get the actual data that's not tied to the theme system.

So I really hope the property wrappers can still go in, whether as API exception or as part of the RDFa feature exception.

Patch attached. Note it's dependent on the latest patch from #551406: Centralize entity properties meta data, so I let the test-bot ignore it. But for me, the tests are working.

Well, I forgot to mention one thing:
in rdf_preprocess_node()

  $variables['title'] = '<span ' . drupal_attributes(rdf_attributes($wrapper, 'title')). '>' . $variables['title'] . '</span>';
  $variables['date'] = '<span ' . drupal_attributes(rdf_attributes($wrapper, 'created')). '>' . $variables['date'] . '</span>';

Doing so the themer has no way to remove those attributes, which imo is odd. I think we should add a general solution to that problem, so that we can easily add in attributes, but in a way the themer can remove them when he likes to. Perhaps we could convert those simple variables to rendered $elements just having a #value, thus the rdf module may add #attributes and a theme-wrapper that really adds in the attributes?

@fago: #569362: Document $attributes, $title_attributes, and $content_attributes template variables was committed recently. Is this sufficient, or are you finding a different need?

I added a new issue: #574954: Add theme_timestamp for RDFa? for the timestamp problem. Please share your thoughts over there.

@#63: My patch already builds upon that, but it doesn't deal with further specific variables like the node-title (when it's no page title) or creation date.

I think $title_attributes takes care of node title (or am I wrong on that?) But yes, creation date is an issue. What do you think about #64, or do you prefer using a renderable element with a wrapper instead?

ah indeed, $title_attributes should work for the node title too. Anyway I think a general solution like renderable elements would make sense, as there are probably a lot of other variables we cannot attach attributes to yet?

>I think we should keep going in the same direction and be consistent.

So you'd create a $VAR_attributes and $VAR_attributes_array for each variable? Hm, if we haven't too many, that might make sense.

I re-rolled the patch from #61, so it also makes use of $title_attributes of the node template.

@#70: The patch already deals with that, it just passes the rdf mapping up to $page once we are on a full page view. However we still need a solution for other variables like the date above.

"Doing so the themer has no way to remove those attributes, which imo is odd. I think we should add a general solution to that problem, "

Our current solution for this sort of thing is to add the data in preprocess and then do the explode in process (a new phase recently added in d7). so, preprocess functions after template_preprocess_node can make changes. hope that makes sense and is useful.

That's the way it's already done for $title_attributes - so the themer has to add in those attributes on it's own. But this becomes cumbersome, if we end up having more of those variables or if there is no wrapping tag yet... Anyway, I'd suggest to move this discussion over to #574954: Add theme_timestamp for RDFa? - which is about solving this problem for dates.

I had a look at how to solve the problem with outputing the username. I followed the guide from http://groups.drupal.org/node/22231.

-> Thus I improved the patch from #61 further, so rdf_attributes() now has options to add in 'about' (and 'typeof') attributes as well as the 'resource' attribute, if necessary. Also now it automatically detects whether it should use the 'rel' or 'property' attribute. Example output for a node teaser is now:

<div id="node-1" class="node node-article node-promoted node-teaser" about="/drupal-7/node/1#this" typeof="sioc:Item foaf:Document">
      <h2 property="dc:title"><a href="/drupal-7/node/1">asdaf</a></h2>
      <span class="submitted"><span property="dc:created" content="1144-09-12T12:53:00+0100" datatype="xsd:dateTime">Thu, 09/10/2009 - 19:45</span> — <span rel="sioc:has_creator"><a href="/drupal-7/user/1" title="View user profile." class="username" about="/drupal-7/user/1#this" typeof="sioc:User" property="foaf:name">fago</a></span></span>
  
  <div class="content clearfix">
      <div class="field field-name-body field-type-text-with-summary field-label-hidden clearfix" property="content:encoded">
        <div class="field-items">
              <div class="field-item even"><p>s</p>
        </div>
      </div>
 </div>
</div>

According to my ubiquity RDFa output, that's fine now.

Furthermore as I needed another wrapping span for the username, I just implemented the possible solution outlined in my comment http://drupal.org/node/574954#comment-2033772 and converted the node $date and $name to renderable arrays (see patch). Would that a way to go for themers?

Some comments to #74:
* date_iso8601(): Yep, it should move out of rdf.module in that case.
* Namespacing: There also some other functions starting with drupal_ and not rdf_
* You introduce an RDFException but it doesn't seem to be used anywhere.

To prevent confusion I moved discussion about basing this on centralized meta data to a separate issue: #575508: building RDFa support on centralized metadata Let's discuss the pros/cons over there!

This is a reply to #75. Looks good. The only thing is that spans are not allowed to have rel attributes in XHTML 1.0.

I took a first pass over this issue so most of my comments are high-level still. No major red flags for me, but I'll want to take a second pass at this.

+++ modules/rdf/rdf.api.php	2009-09-12 09:37:16 +0000
@@ -0,0 +1,69 @@
+/**
+ * Allow modules to override existing mappings.
+ *  
+ * @param &$mappings
+ *   An associative array of mappings
+ */
+function hook_rdf_mapping_alter(&$mapping) {

Is the content of hook_rdf_mapping_alter() in rdf.api.php a copy-paste error? The function seems to return a new mapping rather than modify the existing one?

+++ modules/rdf/rdf.info	2009-09-11 11:23:02 +0000
@@ -0,0 +1,7 @@
+description = Supports RDF mappings.

I'm worried this is a bit too cryptic for most end-users. Would be nice if we could find something a bit more accessible. The word 'support' is somewhat ambiguous.

+++ modules/rdf/rdf.module	2009-09-15 17:33:03 +0000
@@ -0,0 +1,256 @@
+class RdfException extends Exception {}

Why would we care about an empty base class? If it is important to have/keep, please document why in the PHPdoc.

+++ modules/rdf/rdf.module	2009-09-15 17:33:03 +0000
@@ -0,0 +1,256 @@
+/**
+ * Stores the page level RDF mapping in the dictionary.
+ *
+ * @param $mapping
+ * @return array
+ *   The stored mapping
+ */
+function drupal_set_rdf_page_mapping($mapping = NULL) {

It is the first time I'm reading this, and this didn't make instant sense to me. What is the 'dictionary' and why does it matter? What is 'page level mapping'? It feels like you have to bring us up to speed on those terms and their need in the PHPdoc. Maybe that should happen at the top of the module file through some brief architectural overview.

+++ modules/rdf/rdf.module	2009-09-15 17:33:03 +0000
@@ -0,0 +1,256 @@
+function rdf_preprocess_node(&$variables) {

When looking into this function I wondered if there was an overview of all fields I could set (e.g. about, typeof, property, datatype, content, etc). While I understand what the function does, I had to reverse engineer it a bit. An overview of the data structure would be helpful when reviewing this patch.

Wow! A lot to digest here, but, in response to Dries' tweet earlier today, I'm subscribing. If I can help, I'll pitch in after I get up to speed.

Subscribe.

Subscribe. I'm specifically looking at this issue with an eye towards file/stream metadata RDF integration. Has there been any discussion about that, that would be useful to read to catch up on?

Thanks,
Aaron

On first read through the new patch this looks great - much more self-contained.

This seems like the ideal use case for hook_entity_info_alter(). As far as possible I think core modules should try to behave as if they're in contrib (i.e. have nothing connected to them in /includes, and stay out of each others business where possible).

On the hook_node_load() - we could look at providing a hook_entity_load('type', $entities); inside DrupalDefaultEntityLoader::AttachLoad(). I'd have no specific objection to that although between hook_field_attach_load() hook_node_load() and hook_entity_load('type') things get a bit crowded. This is a good use case though, and I can see others needing to do a similar thing.

oh, nice!

>If there is no specific mapping for a bundle, then the default entity type mapping is used (if defined).
As a bundle is a further specialisation of an entity, shouldn't be the rdf-mapping additional, thus not replacing the entity-mapping?

@hook_entity_info:
What about using a similar way to describe the mapping as in #551406: Centralize entity properties meta data? (Look at the API docs in the patch). I mean we could introduce a general description of entity properties there, where the mapping is a part of it. That way, modules may extend so they can make use of it too. E.g. a rdf module which wants to output RDF/XML or so probably needs some more callbacks to retrieve the data.

I still think that would make much sense to do

@drupal_rdf_attributes:
* The function violates module namespace.
* The function just generates a 'property' tag, but that's not fitting for every case. Imo one should just specify the mapping, and then the function should create appropriate attributes using rel/property/content whatever necessary. So any changes to metadata of any module, are automatically reflected. Look up rdfa_attributes() in the patch from #575508: building RDFa support on centralized metadata to see how that could work.

I took a look at the default.profile patch. My guess is that pifr is getting fatal error on the call to rdf_set_mappings(). Simple fix - add rdf module to the array of modules in profiles/default/default.info. Install profiles are much simpler in D7 - just add your module to PROFILE.info and it gets enabled.

This also explains why your tests are not executed. The module is not enabled.

All in all, this patch looks good. I'll need to do a deeper dive, but here are some of my comments:

+++ modules/rdf/rdf.crud.inc	2009-10-08 23:37:04 +0000
@@ -0,0 +1,87 @@
+ *   The entity type the mapping refers to.
+++ modules/rdf/rdf.install	2009-10-09 14:46:46 +0000
@@ -0,0 +1,40 @@
+        'description' => 'The name of the entity type a mapping applies to (node, user, comment, etc.).',

I think it would be useful to mention 'node, comment, user' when talking about 'entity type'. Entity type is an abstract concept, so I think it helps when you make it more concreted in the phpDoc.

+++ modules/rdf/rdf.module	2009-10-09 19:04:20 +0000
@@ -0,0 +1,335 @@
+ * RDFa primer...

This is a place holder?

+++ modules/rdf/rdf.module	2009-10-09 19:04:20 +0000
@@ -0,0 +1,335 @@
+function rdf_save_mapping($mapping) {

It is not clear from the documentation why rdf_save_mapping() cames to be, and when you have to call it. Feels like an odd function -- still investigating. A couple of other functions around saving RDFa confusing me too. Haven't looked at the older patch, but if seems like the old patch was slightly simpler. Will have to review in more detail.

About rdf.api.php

What are bundels?

/**
 * Allow modules to define RDF mappings for bundles.

First mention of bundles. Is a bundle a grouping of something?

Stealing from http://rdfstore.sourceforge.net/rdfapi_perl/rdfExtension.html
"In order to distinguish the RDF Model from an RDF Model, the latter will be referred to as "bundles". "

Could we incorporate this in this first line somehow? Does that make more sense? Hmmm not for me.

Maybe just drop bundle here? But there should be some explanation of bundle further on right?

 * Allow modules to define RDF mappings.

RDFa or RDF

 *
 * Modules defining their own bundles can specify which RDF semantics should be
 * used to annotate these bundles. These mappings are then used for the
 * automatic RDFa output in the HTML code. 

Is it RDFa or RDF ... should all text use RDFa?

non-bundles example please

 * 
 * @return
 *   An associative array of mappings. Most keys will be bundle names, but 
 *   mappings can also be defined for non-bundles. Each bundle mapping is an

"Most keys will be ... for non-bundles"

What is the distinction between a bundle and non-bundle? Can I see this at once when looking at a hook_rdf_mapping?

return value documentation unclear.

 *   array whose keys must correspond to existing field instances in the
 *   bundle with an array of RDF properties as value. The 'rdftype' key is a
 *   special case which is used to define the type of the instance, its
 *   value shoud be an array of RDF classes. 
 */

It should be something like

array whose keys correspond to _bundle_ or _non-bundle_ with array to existing field instances

and maybe

The 'rdftype' key is reserved for specifying ....

The return value doc is not giving much useful information on how to "Do It Yourself"

better example please with inline doc

function hook_rdf_mapping() {

/* Here we return a bundle for 'blog' and a non-bundle for ....
*
* The special key rdftype classifies the blog node as a sioc:Post
* (Semantically Interlinked Online Communities)
* The 'created' date is a created-data defined by Dublin Core
*
* The 'non-bundle example is ...
*/

  return array(
    'blog' => array(
      'rdftype' => array('sioc:Post'),
      'title'   => array('dc:title'),
      'created' => array(

What about this created sub array

Is that a natural consequence of defining a RDFa bundle?

        'property' => array('dc:date', 'dc:created'),
        'datatype' => 'xsd:dateTime',
        'callback' => 'date_iso8601',
      ),
      'name'    => array('foaf:name'),
      'uid'     => array('sioc:has_creator', 'foaf:maker'),
    ),

A non-bundle example

    'non-bundle ?? ' => _how_
  );
} 

hook_form_alter

What should I do for adding a field to a node. My guess is taxonomy would be a good example too.

my two cents :)

On the front page I get

Notice: Array to string conversion in drupal_attributes() (line 2530 of /home/clemens/htdocs/drupal-7/www/includes/common.inc).

with just one article with two taxonomy terms.

Cause :

      $data = implode(' ', $data);

with this

array(2) {
   [0]=>   array(2) {
     ["value"]=>     string(7) "Article"
     ["safe"]=>     string(7) "Article"
   }
   [1]=>   array(3) {
     ["value"]=>     string(7) "Article"
     ["format"]=>     NULL
     ["safe"]=>     string(7) "Article"
   }
} 

Not sure whether this comes from RDF.
--edit--
Is it from rdf_preprocess_node(&$variables) { because the notice are from view node/1 and not from frontpage

+++ modules/rdf/rdf.api.php	2009-10-14 10:48:19 +0000
@@ -0,0 +1,44 @@
+ * Modules defining their own bundles can specify which RDF semantics should be
+ * used to annotate these bundles. These mappings are then used for the
+ * automatic RDFa output in the HTML code.

this can just be "automatic RDFa output in HTML code", no need for the 'the's there.

+++ modules/rdf/rdf.api.php	2009-10-14 10:48:19 +0000
@@ -0,0 +1,44 @@
+ *   -mapping: A mapping is an array whose keys must correspond to existing
+ *   field instances in the bundle with an array of RDF properties as value.
+ *   The 'rdftype' key is a special case which is used to define the type of
+ *   the instance, its value shoud be an array of RDF classes.

Is it possible to spell these out?

+++ modules/rdf/rdf.crud.inc	2009-10-14 10:48:26 +0000
@@ -0,0 +1,89 @@
+ * @param $mapping
+ *   An associative array represeting an RDF mapping structure.

With what keys? Is this defined elsewhere? Can we have an @see?

+++ modules/rdf/rdf.crud.inc	2009-10-14 10:48:26 +0000
@@ -0,0 +1,89 @@
+ * @return bool

This should be "Boolean to indicated that something something happened."

+++ modules/rdf/rdf.crud.inc	2009-10-14 10:48:26 +0000
@@ -0,0 +1,89 @@
+  $num_rows = db_delete('rdf_mapping')->condition('type', $type)
+    ->condition('bundle', $bundle)->execute();
+
+  return ($num_rows > 0);
+}

With these, could we do return (bool) db_delete(). Not major, and ymmv.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+ * RDFa primer...

Can we have a proper @todo here?

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+/*
+ * Load all public RDF CRUD API functions. Drupal currently has no
+ * mechanism for auto-loading core APIs, so we have to load them on
+ * every page request.
+ */

This either needs /** at the top, or just // style comments,but not /* ... */.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+function rdf_save_mapping(array $mapping) {

Type hints should start with a capital letter. Array $mapping

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+ * Implements hook_modules_installed().

Isn't the standard "Implement", I know there's an issue somewhere to change this though.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+/**
+ * Implements hook_modules_uninstalled().
+ */
+function rdf_modules_uninstalled($modules) {
+// TODO remove the RDF mappings.
+}

How critical is this TODO. And again needs to be @todo here.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+ *   array(
+ *     'property' => array('dc:created'),
+ *     'datatype' => 'xsd:dateTime',
+ *     'callback' => 'date_iso8601',
+ *   )

Can probably use @code here?

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+
+  if (isset($mapping['callback']) && isset($data)) {
+    $callback = $mapping['callback'];
+
+    if (function_exists($callback)) {
+      $attributes['content'] = call_user_func($callback, $data);
+    }
+    if (isset($mapping['datatype'])) {
+      $attributes['datatype'] = $mapping['datatype'];
+    }
+  }

This could do with some inline comments. $data isn't much fun as a function parameter, anything more descriptive?

Also why 'call_user_func' - wouldn't $function work here?

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+function rdf_comment_view($comment){
+  $comment->rdf_mapping = rdf_get_mapping('comment', $comment->node_type);
+}

Why hook_comment_view() here, but hook_$foo_load() everywhere else? We have a hook_comment_load() now.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+ * @return string

We don't add types to @param or @return statements - breaks api.drupal.org

+++ modules/rdf/rdf.test	2009-10-14 10:48:39 +0000
@@ -0,0 +1,194 @@
+    // We need to trigger rdf_modules_installed() because
+    // hook_modules_installed() is not automatically invoked during testing.
+    rdf_modules_installed(array('rdf_test'));

Could the forced cache_clear() in rdf_modules_installed() be added here instead? I haven't read the issue linked from earlier in the patch though.

+++ modules/rdf/rdf.test	2009-10-14 10:48:39 +0000
@@ -0,0 +1,194 @@
+   * Test that the mappings are correctly returned and processed by the hook_rdf_mapping().
+   */

Exceeds 80 chars.

+++ modules/rdf/rdf.test	2009-10-14 10:48:39 +0000
@@ -0,0 +1,194 @@
+
+class RdfMarkupTestCase extends DrupalWebTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => t('RDFa markup'),
+      'description' => t('Test RDFa markup generation.'),
+      'group' => t('RDF'),
+    );
...
+  function testDrupalRdfaAtributes() {
+    $date = 1252750327;
+    $isoDate = date('c', $date);
+
+    $expected_type = 'xsd:dateTime';
+    $expected_property = array('dc:created');
+    $expected_value = $isoDate;
+
+    $mapping = rdf_get_mapping('test_entity', 'test_bundle');
+    $attributes = drupal_rdfa_attributes($mapping['created'], $date);
+
+    $this->assertEqual($expected_type, $attributes['datatype']);
+    $this->assertEqual($expected_property, $attributes['property']);
+    $this->assertEqual($expected_value, $attributes['content']);
+  }
+
+}
+

Do we really need a class per test function? If the setUp() isn't completely different, would be a lot cleaner to just have one class.

That's pretty much just comment nitpicks though. Code stil looks very clean since looking at it last time too. Nice!

I'm on crack. Are you, too?

Fixed nearly everything

Do we really need a class per test function? If the setUp() isn't completely different, would be a lot cleaner to just have one class.

I think it's fine for now.

Could the forced cache_clear() in rdf_modules_installed() be added here instead? I haven't read the issue linked from earlier in the patch though.

Again, I think it's fine for now.

@bjaspan, @scor and myself spent one hour reviewing this patch in person. Here are our comments:

+++ modules/node/node.module	2009-10-11 12:18:50 +0000
@@ -750,6 +750,47 @@ function node_type_set_defaults($info =
+        'title'   => array(

Should this be associated with the field instead?

+++ modules/node/node.module	2009-10-11 12:18:50 +0000
@@ -750,6 +750,47 @@ function node_type_set_defaults($info =
+        'body'    => array(

Should this be associated with the field instead?

+++ modules/rdf/rdf.crud.inc	2009-10-14 10:48:26 +0000
@@ -0,0 +1,89 @@
+ * RDF CRUD API, handling RDF mapping creation and deletion.

Explain how we store the mapping, why we store the mapping in the database.

+++ modules/rdf/rdf.install	2009-10-14 12:50:34 +0000
@@ -0,0 +1,40 @@
+        'serialize' => TRUE,

Might be redundant; only use by drupal_write_record().

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+ * RDFa primer...

Is this a @todo? Maybe just link to an primer online.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+define('RDF_DEFAULT_BUNDLE', '');

Document what the default bundle is.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+function rdf_modules_installed($modules) {

The same code needs to be called when the RDF module is enabled on previously installed modules.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+      $mapping_array = call_user_func($module . '_rdf_mapping');

We should be able to use module_invoke(). Or, better, we should be able to use module_invoke_all()?

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+// TODO remove the RDF mappings.

Should be @todo instead of TODO.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+function drupal_rdfa_attributes($mapping, $data = NULL) {

Explain why some mappings have a callback and why others don't.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+function rdf_node_load($nodes, $types) {

We'd prefer to do this in the entity loader, if possible.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+    $node->rdf['user'] = array('uri' => _rdf_generate_user_uri($node->uid));

Add a @todo that we need to clean this up once we split the user object from the node object.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+    $user->rdf = array('uri' => _rdf_generate_user_uri($user->uid));

Remove.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+function rdf_comment_view($comment){

Needs to be rdf_comment_load().

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+function _rdf_generate_user_uri($uid) {

Remove.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+function date_iso8601($date) {

Should either be namespaced or moved to a better file.

+++ modules/rdf/rdf.module	2009-10-14 12:50:34 +0000
@@ -0,0 +1,370 @@
+  // TODO use a theme_timestamp() function instead? see http://drupal.org/node/574954

Should be @todo instead of TODO.

dmitrig01's last patch is only 3k?

For the rdf_node_load() (and friends), I think Dries meant something like #605442: Add a generic hook_entity_load().

http://api.drupal.org/api/function/field_extract_ids/7

Should let you pass in entity type and entity, and get bundle back - which will save the switch.

Subscrrrrrrribing!

Looking good. Few final nitpicks from me but this is all minor and could potentially be handled in followup issues/

+++ modules/node/node.module	2009-10-17 00:04:41 +0000
@@ -754,6 +754,47 @@ function node_type_set_defaults($info = 
+        'body'    => array(
+          'property' => array('content:encoded'),
+        ),

Should we leave this to field API or is this special cased for a reason?

+++ modules/rdf/rdf.crud.inc	2009-10-14 10:48:26 +0000
@@ -0,0 +1,89 @@
+ * @return bool
+ */
+function rdf_delete_mapping($type, $bundle) {
+  $num_rows = db_delete('rdf_mapping')->condition('type', $type)
+    ->condition('bundle', $bundle)->execute();
+
+  return ($num_rows > 0);
+}

This should be "return boolean to indicate whether the mapping was deleted or not" (or similar). Not sure if we also want to explicitly cast for the return return (bool) ($num_rows > 0);. Doesn't really matter.

+++ modules/rdf/rdf.install	2009-10-17 01:07:07 +0000
@@ -0,0 +1,50 @@
+function rdf_install() {
+  // The installer does not trigger hook_modules_installed() so it needs to
+  // triggered programmatically on the modules which defined RDF mappings.
+  $modules = module_implements('rdf_mapping');
+  rdf_modules_installed($modules);
+}

Can we open a followup for this one? Looks like a major annoyance. Now that rdf_install() has this though, are the hunks in setUp() required?

+++ modules/rdf/rdf.module	2009-10-17 01:23:35 +0000
@@ -0,0 +1,325 @@
+ * RDFa primer...

Needs a @todo.

+++ modules/rdf/rdf.module	2009-10-17 01:23:35 +0000
@@ -0,0 +1,325 @@
+module_load_include('inc', 'rdf', 'rdf.crud');

We've been round in circles on this in another issue. I think current best bet is require_once DRUPAL_ROOT since module_load_include() depends on the database.

Or why can't the crud.inc functions just go straight inside rdf.module?

+++ modules/rdf/rdf.module	2009-10-17 01:23:35 +0000
@@ -0,0 +1,325 @@
+/**
+ * Implements hook_entity_load().
+ */
+function rdf_entity_load($entities, $type) {
+  foreach ($entities as $entity) {
+    // Extracts the bundle of the entity being loaded.
+    list($id, $vid, $bundle) = field_extract_ids($type, $entity);
+    $entity->rdf_mapping = rdf_get_mapping($type, $bundle);
+  }
+}
+

Glad this worked. field_extract_ids() probably belongs under the entity_* namespace now, but I imagine we're too late to get that in for D7 :(

+++ modules/rdf/rdf.module	2009-10-17 01:23:35 +0000
@@ -0,0 +1,325 @@
+  // Add RDFa markup to the title of the node. Because the RDFa markup is added
+  // to the h2 tag which might contain HTML code, we specify an empty datatype
+  // to ensure the value of the title read by the RDFa parsers is a literal.
+  $variables['title_attributes_array']['property'] = empty($variables['node']->rdf_mapping['title']['property']) ? NULL : $variables['node']->rdf_mapping['title']['property'];
+  $variables['title_attributes_array']['datatype'] = '';

Can't let field RDF integration handle this now title is field?

+++ modules/rdf/rdf.test	2009-10-14 10:48:39 +0000
@@ -0,0 +1,194 @@
+  /**
+   * Test that the mappings are correctly returned and processed by the hook_rdf_mapping().
+   */

Exceeds 80 chars.

+++ modules/rdf/rdf.test	2009-10-14 10:48:39 +0000
@@ -0,0 +1,194 @@
+    // Test that NULL is returned when an entity type, bundle pair has no mapping.

And this one.

I'm on crack. Are you, too?

This patch includes additional fixes in responses to Catch in from #131 (don't worry, I'm with Scor in Boston, I am not totally unsupervised)

- Body is (as of last couple patches) now in node_rdf_mapping() only and not special cased in any way.
- This version of the patch puts CRUD functions into rdf.module and save an include call on every page load.
- Fixed long code comments.

Also in this patch, Scor:
- added support for the body of comment. Since it is not a field, we need a special case in rdf_comment_view and wrap the body in a div which include the proper RDFa markup. This could be improve by (1) moving this into a $body_attributes added in comment.tpl.php (though it's still not clean because content can contains field), and (2) better/cleaner solution is to have #538164: Comment body as field (Comment subject and body should be fields! RDF wants).

Everything the last patch said it had.

I'm happy to see such nice progress being made on this issue. Scor and I chatted over IRC yesterday, where he explained to me a remaining problem around getting a needed span tag around node and comment authors and dates. Please see #607456: RDF still needs one more mechanism to get attributes on a themer accessible element for my proposal.

Ok, there's some absolutely legitimate debate on whether the patch in #607456: RDF still needs one more mechanism to get attributes on a themer accessible element is something we really want in core, since it's a semi-duplication of what the render() system was meant to solve, but just didn't get a chance to be used in all the places that needed it.

However, the equivalent solution can live entirely within the rdf module as follows:

function rdf_theme() {
  return array(
    'rdf_template_variable_wrapper' => array(
      'arguments' => array('content' => NULL, 'attributes' => array(), 'context' => array(), 'inline' => TRUE),
    ),
  );
}

function rdf_process(&$variables, $hook) {
  if (!empty($variables['rdf_variable_attributes_array'])) {
    foreach ($variables['rdf_variable_attributes_array'] as $variable_name => $attributes) {
      $context = array('hook' => $hook, 'variable_name' => $variable_name, 'variables' => $variables);
      $variables[$variable_name] = theme('rdf_template_variable_wrapper', array('content' => $variables[$variable_name], 'attributes' => $attributes, 'context' => $context));
    }
  }
}

function theme_rdf_template_variable_wrapper($variables) {
  $output = $variables['content'];
  if (!empty($output) && !empty($variables['attributes'])) {
    $attributes = drupal_attributes($variables['attributes']);
    $output = $variables['inline'] ? "<span$attributes>$output</span>" : "<div$attributes>$output</div>";
  }
  return $output;
}

Then, the change I'm suggesting in http://drupal.org/node/607456#comment-2163706 can still be done, just replacing 'variable_attributes_array' with 'rdf_variable_attributes_array'.

This would then only be usable by the rdf module, instead of being a generic facility, but the rdf module is our only use-case that needs it (at least in core). My suggestion is to re-roll the patch with this, and if #607456: RDF still needs one more mechanism to get attributes on a themer accessible element lands, then it'll be pretty simple to update this patch accordingly.

If anyone disagrees with this approach, please say so. I'm just offering an opinion.

@scor: Correct me if I'm wrong, but as I understood our conversation, it would be possible to get rid of most of what's in rdf_preprocess_username() if we were able to get another span around it. That's what theme_rdf_template_variable_wrapper() allows. In rdf_preprocess_node(), you can add $variables['rdf_variable_attributes_array']['name'] = SOME_ATTRIBUTES_ARRAY and in rdf_preprocess_comment(), you can add $variables['rdf_variable_attributes_array']['author'] = SOME_ATTRIBUTES_ARRAY. Was there something about this technique that didn't work / didn't capture everything that RDF needs?

scor is working on an improvement to the way that a div is added to the comment content, but that can still be contained within the rdf module. However, IMO, it's an oversight in core that we don't have $content_attributes as a generic variable available in all tpl file. Please see #608036: Add content_attributes variable for tpl files, so RDF can be implemented better.. Hopefully, that can land, and then the RDF patch can be made even better.

I committed #608036: Add content_attributes variable for tpl files, so RDF can be implemented better..

This patch looks mostly ready to fly.

1) Please use diff -up, so reviewers can see the context of the changes.

2) A couple of minor, but also potentially major issues (especially regarding CRUD functions):

+++ modules/comment/comment.module	2009-10-18 21:30:46 +0000
@@ -2494,3 +2494,35 @@ function comment_filter_format_delete($f
+          'property' => array('dc:title'),
...
+          'property' => array('dc:date', 'dc:created'),

It is *very* unnatural to me to have a singular key "property" that expects a list of multiple properties. We currently do this in hook names only, and even that is very questionable. So I wonder whether we can rename this key to 'properties'?

+++ modules/comment/comment.module	2009-10-18 21:30:46 +0000
@@ -2494,3 +2494,35 @@ function comment_filter_format_delete($f
+        'body'    => array(
...
+        'pid'     => array(

(and elsewhere) Please do not align these.

+++ modules/comment/comment.test	2009-10-18 23:57:41 +0000
@@ -864,3 +864,52 @@ class CommentRSSUnitTest extends Comment
+  public static function getInfo() {
+    return array(
+      'name' => t('RDFa comment markup'),
+      'description' => t('Test RDFa markup in comments.'),
+      'group' => t('RDF'),

t() must be removed from getInfo().

+++ modules/node/node.tpl.php	2009-10-19 03:00:50 +0000
@@ -13,7 +13,7 @@
  * - $date: Formatted creation date (use $created to reformat with
- *   format_date()).
+ *   format_date()). This data is excepted to be sanitized beforehand.
  * - $name: Themed username of node author output from theme_username().
@@ -88,8 +88,8 @@
-            print t('Submitted by !username on @datetime',
-              array('!username' => $name, '@datetime' => $date));
+            print t('Submitted by !username on !datetime',
+              array('!username' => $name, '!datetime' => $date));

...seems to be the same case for $name, so I don't think we need this sanitisation "warning" in the variable description.

+++ modules/rdf/rdf.api.php	2009-10-17 01:50:01 +0000
@@ -0,0 +1,44 @@
+ * @return
+ *   An array of mappings. Each mapping has three mandatory keys:
+ *   -type: The name of an entity type
+ *   -bundle: The name of a bundle
+ *   -mapping: A mapping is an array whose keys must correspond to existing
+ *   field instances in the bundle with an array of RDF properties as value.
+ *   The 'rdftype' key is a special case which is used to define the type of
+ *   the instance, its value shoud be an array of RDF classes.

Please read http://drupal.org/node/1354 about proper Doxygen formatting (here: lists).

+++ modules/rdf/rdf.api.php	2009-10-17 01:50:01 +0000
@@ -0,0 +1,44 @@
+function hook_rdf_mapping() {
...
+    array(
...
+      )
+    ),

(potentially elsewhere) Missing trailing comma.

+++ modules/rdf/rdf.install	2009-10-17 01:07:07 +0000
@@ -0,0 +1,50 @@
+function rdf_install() {
+  // The installer does not trigger hook_modules_installed() so it needs to
+  // triggered programmatically on the modules which defined RDF mappings.

s/needs to/needs to be/

s/programmatically//

s/on the modules/for modules/

s/which defined/defining/

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+/**
+ * @file
+ * Enables semantically enriched output for Drupal sites.
+ *
...
+ * Example mapping from node.module:
+ *   array(
...
+ *  );
+ */

Missing @code and @endcode Doxygen directives.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+ * Modules can provide mappings of their bundles' data and metadata to RDFa
+ * properties using the appropriate vocabularies. This module takes care of
+ * injecting that data into variables available to themers in the .tpl files.
+ * Drupal core themes ship with RDFa output enabled.

After reading more of this patch, it's not clear to me what exactly a bundle is - and - what a bundle is NOT.

This explanation should come right before this paragraph.

And this explanation should also clarify the meaning of "mapping", right after clarifying the meaning of "bundle".

Afterwards, you can proceed with technical details, but before doing so, the reader has to understand WTF you are talking about. :)

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+/**
+ * Defines the empty string as the name of the bundle to store default
+ * RDF mappings of a type's properties (fields, et. al.).
+ */
+define('RDF_DEFAULT_BUNDLE', '');

uhm... if there is only one, and if it is defined as empty string... why do we need the constant...?

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+function rdf_theme() {
+  return array(
+    'rdf_template_variable_wrapper' => array(
+      'arguments' => array('content' => NULL, 'attributes' => array(), 'context' => array(), 'inline' => TRUE),

What is 'context' and why do we need it? It's not used in that theme hook.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+ /**
+ * Wraps a template variable in an HTML element with the desired attributes.

(and massively elsewhere) Wrong alignment/indentation of PHPDoc.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+function theme_rdf_metadata($variables) {
...
+    $output .= '<span' . drupal_attributes($attributes) . ' />';

Did you verify and absolutely ensure that this is valid XHTML Strict?

IIRC, only DIVs are allowed to be empty.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+ /**
+ * Process function for wrapping some content with an extra tag.
+ */
+function rdf_process(&$variables, $hook) {

s/Process/Template process/

s/some content/content/

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+function rdf_process(&$variables, $hook) {
+  if (!empty($variables['rdf_variable_attributes_array'])) {
...
+  if (!empty($variables['metadata_attributes_array'])) {

These two could use a bit more inline documentation; not repeating what's visible in-code, but quickly explaining what is being done here and why.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+ * @return array

(perhaps elsewhere) We do not define data types in Doxygen.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+function rdf_modules_installed($modules) {
...
+    if (function_exists($module . '_rdf_mapping')) {
+      $mapping_array = call_user_func($module . '_rdf_mapping');

Define $function first and then test function_exists(). Afterwards, directly invoke $function(). No need for cuf() here.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+        // Only the bundle mappings are saved in the database.
+        if ($mapping['bundle'] != RDF_DEFAULT_BUNDLE) {
+          rdf_save_mapping($mapping);
+        }

As mentioned earlier, this condition is the exact same as !empty($mapping['bundle']), so I still don't see the need for this constant.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+ *   Example:
+ *   array(
...
+ *   )

Missing @code and @endcode directives.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+function drupal_rdfa_attributes($mapping, $data = NULL) {
...
+    $callback = $mapping['callback'];
...
+    if (function_exists($callback)) {
+      $attributes['content'] = call_user_func($callback, $data);

No need for cuf() here, just invoke $callback($data);

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+/**
+ * Implements hook_entity_load().

(and elsewhere) The current core standard (ugh...) is "Implement" (argh...). That's a total mess, but it's the current standard. :(

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+function rdf_preprocess_node(&$variables) {
...
+    $title_attributes['property'] = empty($variables['node']->rdf_mapping['title']['property']) ? NULL : $variables['node']->rdf_mapping['title']['property'];
+    $title_attributes['content'] = $variables['node_title'];
+    $title_attributes['about'] = $variables['node_url'];
+    drupal_add_html_head('<meta' . drupal_attributes($title_attributes) . ' />');

It seems like #552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag would really help here.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+
+function rdf_preprocess_field_formatter_taxonomy_term_link(&$variables) {

Missing PHPDoc.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+function _rdf_get_default_mapping($type) {
...
+        if ($mapping['bundle'] == RDF_DEFAULT_BUNDLE) {
+          $default_mappings[$mapping['type']] = $mapping['mapping'];
+        }
...
+  return empty($default_mappings[$type]) ? array() : $default_mappings[$type];

Interestingly, you *are* using empty() here already... ;)

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+ * RDF CRUD API, handling RDF mapping creation and deletion.

Instead of describing this all over again in all functions, consider to create a

@defgroup rdf RDF API functionality

around those functions.

See ajax.inc for examples.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+function rdf_create_mapping($type, $bundle, $mapping) {
+  $fields = array(
+    'type' => $type,
+    'bundle' => $bundle,
+    'mapping' => serialize($mapping)
+  );
+
+  db_insert('rdf_mapping')->fields($fields)->execute();
+
+  return $mapping;
+}
...
+function rdf_update_mapping($type, $bundle, $mapping) {
+  $fields = array('mapping' => serialize($mapping));
+  $num_rows = db_update('rdf_mapping')->fields($fields)
+    ->condition('type', $type)->condition('bundle', $bundle)->execute();
+
+  return (bool) ($num_rows > 0);
+}

These really look like you could remove them altogether and use drupal_write_record() instead.

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+function rdf_read_mapping($type, $bundle) {
+  $query = db_select('rdf_mapping')->fields(NULL, array('mapping'))
+    ->condition('type', $type)->condition('bundle', $bundle)->execute();
+
+  $mapping = unserialize($query->fetchField());
+
+  if (!is_array($mapping)) {
+    $mapping = array();
+  }

1) DBTNG coding style needs work, should look like:

$mapping = db_select('rdf_mapping')
  ->fields(NULL, array('mapping'))
  ->condition('type', $type)
  ->condition('bundle', $bundle)
  ->execute()
  ->fetchField();

2) As already contained in 1), you should fetch the value already, and unserialize is afterwards.

3) Before unserializing, you check for FALSE and conditionally return an array().

+++ modules/rdf/rdf.module	2009-10-19 02:36:35 +0000
@@ -0,0 +1,557 @@
+function rdf_delete_mapping($type, $bundle) {
+  $num_rows = db_delete('rdf_mapping')->condition('type', $type)
+    ->condition('bundle', $bundle)->execute();

Ditto. (DBTNG coding-style)

+++ modules/rdf/rdf.test	2009-10-17 22:55:50 +0000
@@ -0,0 +1,193 @@
+class RdfMappingHookTestCase extends DrupalWebTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => t('RDF mapping hook'),

t() again.

+++ modules/rdf/rdf.test	2009-10-17 22:55:50 +0000
@@ -0,0 +1,193 @@
+
+class RdfMarkupTestCase extends DrupalWebTestCase {

(and elsewhere) Missing PHPDoc for test cases.

I'm on crack. Are you, too?

scor is about to re-roll. This is a very nice patch, leveraging some elegant solutions we managed to get in via other issues, so I'm marking this RTBC already.

Please wait for the next patch, and, the bot coming back green. :)

Addressing two non-style issues in sun's comprehensive review in comment #152 which will not be changed:

• We define RDF_DEFAULT_BUNDLE as an empty string because it is not intuitive and because it could potentially be changed to something else (and a grep on '' is really hard, heh).
• We use the singular "property" because that's how it gets output in RDFa markup, even when there are multiple properties (for example <span property="dc:date dc:created" content="2009-10-18T19:42:31-04:00" datatype="xsd:dateTime">Sun, 10/18/2009 - 19:42</span>), so while counterintuitive it on balance slightly less confusing as is.

I reviewed this patch once again, and decided to finally commit it to CVS HEAD. There are some extra things we can do as follow-up patches, but it is very solid as is. Great job to all the people involved, especially scor!

We need a CHANGELOG.txt entry though.

Tagging for the upgrade guide. GREAT job folks!!

Yes, absolutely, great job scor and others who worked so hard on this!!

Here's another patch to clean up and document some of the theme layer related code. This patch doesn't include scor's changelog patch, so #160 needs to be committed separately. Given that #160 doesn't need testbot running on it, I hope it's okay to have submitted this one prior to that one being committed.

I committed the patch in #160 and the one in #161. Thanks all!

So here is a "slight revamp" that should cover most of the issues I mentioned earlier.

Additionally, making it use the proper function name pattern of subject_verb(), which I only mentioned in IRC.

Also makes proper use of db_merge() and thereby kills two RDF API functions.

And, sorry for this one, but the order of functions in rdf.module don't make any sense. We should clean this up now instead of in X years.

sorry, I forgot the serialize().

Pass again? :)

Incorporated scor's review from #169.

Congrats everyone !

Just to check, as I'm not able to give a close look at the actual code : does the implementation support two bundles with the same name on different entity types ? (e.g a node type named 'user')
#470242: Namespacing for bundle names recently fixed the problem with Field API itself, but last time I checked (something like a month ago now), the RDF patch was still prone to break on this case.

>Clearly nobody will SQL SELECT on the RDF mappings directly, so serialization into a single field is not a problem.

Not being able to figure out the API just to select the data out and get a triple, this seems to be the only way to go ........ opposed to clearly not.

THANKS for all the work here!

Now also containing a @defgroup rdf RDFa API.

There is more to clean up here, especially with regard to making the documentation grokable for ordinary people who are not so familiar with RDFa yet. Additionally, I discussed with scor in IRC that we should also rename rdf_get_mapping() and rdf_mapping_load() after this patch went in, because the latter is a private API function only.

However, those further clean-ups, we want to tackle in separate follow-up issues. So it would be great if we could commit this one and move forward with the remaining stuff.

Since this patch slaughters kittens in a most unholy way, it would be wonderful to get a summary of all of the changes in this patch.

This patch covers all remaining issues from my original review in #152, because not all were incorporated in the committed patch.

To summarize:

- PHPDoc fixes: "Implementation of" in summaries, missing PHPDoc for tests, proper formatting of lists, etc.

- A couple of coding standards issues like white-space, indentation, etc.

- Changes RDF API function names from rdf_[verb]_[subject]() to rdf_[subject]_verb().

- Removes rdf_insert_mapping() and rdf_update_mapping() in favor of a single rdf_mapping_save(&$mapping), which leverages db_merge().

- Moves a couple of functions around to achieve a clean order of functions in rdf.module: 1) RDF API functions 2) Hook implementations 3) Template/rendering (pre)processors and theme functions.

Thanks!

Technically, some of these are API changes, but since the API just went in like yesterday I'm guessing we won't cause too much hardship by committing this now.

Committed to HEAD.

Thanks! However, if you're going to get nitpicky about "API changes" for the API we just introduced here, then we need to do the remaining fix I mentioned before in this very issue.

This patch performs this remaining change, which has the following reasoning:

- The previous patch revamped the introduced CRUD functions to use the same pattern we use elsewhere, namely: rdf_mapping_load() and rdf_mapping_save().

- Further discussion in IRC revealed (partially due to JerryH's mentioning above) that rdf_mapping_load() is NOT a public API function. It should only be used internally.

- Instead, rdf_get_mapping() is that public API function, and therefore, it needs to be renamed to rdf_mapping_load() -- while the original rdf_mapping_load() becomes _rdf_mapping_load(), i.e. a private helper function to query the database.

If something's a private helper function, let's prefix it with an underscore.

+++ modules/rdf/rdf.module	20 Oct 2009 17:50:39 -0000
@@ -119,9 +119,9 @@ function _rdf_get_default_mapping($type)
-function rdf_mapping_load($type, $bundle) {
+function _rdf_mapping_load($type, $bundle) {

uhm, yeah?

duh. totally misread your comment. :)

Committed this to HEAD too.

Makes sense. Thanks for catching that. What do you think of also adding a class of "rdf-meta" to the span, an "rdf.css" file with a ".rdf-meta {display:none}" rule, and removing the "Tip for themers" part of the PHPDoc for this function?

+++ modules/rdf/rdf.module
@@ -634,13 +634,11 @@ function theme_rdf_template_variable_wrapper($variables) {
- * Tip for themers: while this default implementation results in valid markup
- * for the XHTML+RDFa doctype, you may need to override this in your theme to be
- * valid for doctypes that don't support empty spans. Or, if empty spans create
- * visual problems in your theme, you may want to override this to set a
- * class on them, and apply a CSS rule of display:none for that class.
+ * Tip for themers: if empty spans create visual problems in your theme, you may
+ * want to override this to set a class on them, and apply a CSS rule of
+ * display:none for that class.

Can we keep the note about the document type to avoid bogus bug reports in core?

This review is powered by Dreditor.

@scor, @sun: how about this?

Cool.

Yeah, I think this makes sense.

Committed to HEAD. Are we done here..?

@scor: Re: #196: #653916: Remove unnecessary 'rdf-meta' class from HTML output.