This issue is part of a series of patches which came out of the RDF code sprint. See the list of all RDF patches for Drupal code. Please read State of the RDF in Drupal core after the code sprint which explains the overall approach of the RDF effort in core.

This is the patch to create the core RDF module. It deals with managing the mappings defined by the modules and provides a helper function for generating some of the RDFa attributes for the theme layer. Tests included. It is fairly small in itself. We discussed whether this should be a module or a include library. We went with a module for now but this can still change, especially since maybe half of the RDF code in core will be sprinkled in the other modules defining their RDF mappings (see other separate patches:
#493086: RDF #2: node module
#493094: RDF #3: field module
#493056: Add RDF to the user module

Files: 
CommentFileSizeAuthor
#195 493030-empty_span-195.patch2.73 KBeffulgentsia
Passed on all environments.
[ View ]
#193 493030-193_empty_span.patch1.35 KBscor
Passed on all environments.
[ View ]
#190 493030-190_empty_span.patch497 bytesscor
Passed on all environments.
[ View ]
#182 drupal.rdf-mapping-load.patch7.19 KBsun
Passed: 14428 passes, 0 fails, 0 exceptions
[ View ]
#177 drupal.rdf-cleanup.177.patch51.53 KBsun
Passed: 14381 passes, 0 fails, 0 exceptions
[ View ]
#175 drupal.rdf-cleanup.170_2.patch47.49 KBscor
Passed: 14437 passes, 0 fails, 0 exceptions
[ View ]
#171 drupal.rdf-cleanup.170.patch46.83 KBsun
Passed: 14261 passes, 0 fails, 0 exceptions
[ View ]
#170 drupal.rdf-cleanup.169.patch48.9 KBsun
Passed: 14287 passes, 0 fails, 0 exceptions
[ View ]
#167 drupal.rdf-cleanup.166.patch48.36 KBsun
Failed: Failed to run tests.
[ View ]
#165 drupal.rdf-cleanup.164.patch48.35 KBsun
Failed: Failed to install HEAD.
[ View ]
#163 drupal.rdf-cleanup.patch35.5 KBsun
Failed: Failed to install HEAD.
[ View ]
#161 rdf-theming-cleanup-493030-161.patch11.33 KBeffulgentsia
Unable to apply patch rdf-theming-cleanup-493030-161.patch
[ View ]
#160 rdf_changelog.patch587 bytesscor
Passed: 14276 passes, 0 fails, 0 exceptions
[ View ]
#157 rdf.module44.patch48.54 KBscor
Failed: Failed to apply patch.
[ View ]
#155 rdf.module43.patch48.42 KBscor
Failed: Failed to apply patch.
[ View ]
#151 rdf.module42.patch44.79 KBscor
Failed: Failed to apply patch.
[ View ]
#149 rdf.module41.patch46.85 KBscor
Failed: Failed to apply patch.
[ View ]
#145 rdf.module40.patch40.67 KBscor
Failed: Failed to install HEAD.
[ View ]
#143 rdf.module39.patch40.69 KBscor
Failed: 13501 passes, 0 fails, 10 exceptions
[ View ]
#141 rdf.module38.patch40.1 KBscor
Failed: 13489 passes, 0 fails, 448 exceptions
[ View ]
#137 rdf.modulerdf-omnibus-606114-136.patch.patch35.3 KBmlncn
Failed: Failed to apply patch.
[ View ]
#136 rdf.modulerdf-omnibus-606114-135.patch.patch35.33 KBmlncn
Failed: Failed to install HEAD.
[ View ]
#135 rdf.module37.patch32.89 KBscor
Failed: Failed to install HEAD.
[ View ]
#134 rdf.module36.patch31.41 KBscor
Failed: Failed to install HEAD.
[ View ]
#132 rdf.module35.patch31.41 KBscor
Failed: 13271 passes, 0 fails, 297 exceptions
[ View ]
#130 rdf.module34.patch29.3 KBscor
Failed: Failed to install HEAD.
[ View ]
#128 rdf.module33.patch28.47 KBscor
Failed: 13265 passes, 3 fails, 0 exceptions
[ View ]
#125 rdf.module32.patch28.86 KBscor
Failed: Failed to install HEAD.
[ View ]
#118 rdf.module.patch3.42 KBdmitrig01
Failed: Failed to install HEAD.
[ View ]
#116 rdf.module31.patch29.83 KBscor
Failed: Failed to install HEAD.
[ View ]
#111 rdf.module30.patch28.24 KBscor
Failed: 13741 passes, 0 fails, 568 exceptions
[ View ]
rdf.module26.patch25.67 KBscor
Failed: Failed to apply patch.
[ View ]
#105 rdf.module26.patch26.6 KBscor
Failed: Failed to install HEAD.
[ View ]
#104 rdf.module25.patch28.65 KBscor
Failed: Failed to apply patch.
[ View ]
#103 comment.module25.patch674 bytesscor
Failed: Failed to apply patch.
[ View ]
rdf.module24_main.patch30.67 KBscor
Failed: 13759 passes, 4 fails, 2 exceptions
[ View ]
#99 rdf.module24.patch32.61 KBscor
Failed: Failed to install HEAD.
[ View ]
rdf.module23_tiny.patch32.58 KBscor
Failed: 13691 passes, 4 fails, 2 exceptions
[ View ]
rdf.module23.patch34.52 KBscor
Failed: Failed to install HEAD.
[ View ]
#88 rdf.module23.patch34.52 KBscor
Failed: Failed to install HEAD.
[ View ]
#81 rdf.module22.patch24.92 KBscor
Failed: 13801 passes, 5 fails, 40 exceptions
[ View ]
#79 rdf.module20.patch21.73 KBscor
Failed: 12989 passes, 3 fails, 1376 exceptions
[ View ]
#75 rdf.patch.txt27.45 KBfago
#74 rdf.module19.patch21.6 KBscor
Failed: 12986 passes, 4 fails, 1416 exceptions
[ View ]
#71 rdf.patch.txt22.95 KBfago
#61 rdf.patch.txt23 KBfago
#59 rdf.module18.patch22.43 KBscor
Failed: 12919 passes, 3 fails, 10 exceptions
[ View ]
#57 field.module18.patch5.18 KBscor
Failed: Failed to apply patch.
[ View ]
#55 rdf.module17.patch28.77 KBStefan Freudenberg
Failed: Failed to apply patch.
[ View ]
blog.module17.patch1.23 KBStefan Freudenberg
Failed: Failed to apply patch.
[ View ]
#51 rdf.module16.patch28.59 KBStefan Freudenberg
Failed: Failed to apply patch.
[ View ]
#49 rdf.module15.patch44.08 KBStefan Freudenberg
Failed: Failed to apply patch.
[ View ]
#48 rdf.module14.patch684 byteskriskras
Failed: Failed to apply patch.
[ View ]
#46 rdf.module13.patch28.1 KBStefan Freudenberg
Failed: 12850 passes, 1 fail, 10 exceptions
[ View ]
#40 block-level-attributes4.patch2.25 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#38 rdf.module12.patch24.11 KBscor
Failed: 12794 passes, 0 fails, 10 exceptions
[ View ]
#37 rdf.module11.patch24.1 KBscor
Failed: 11918 passes, 778 fails, 778 exceptions
[ View ]
#36 block-level-attributes3.patch1.44 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#35 block-level-attributes2.patch1.07 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#32 block-level-attributes.patch1.39 KBpwolanin
Failed: 12700 passes, 341 fails, 12305 exceptions
[ View ]
#30 rdf.module10.patch20.9 KBscor
Failed: 12731 passes, 1 fail, 0 exceptions
[ View ]
#26 rdf.module09.patch19.46 KBscor
Failed: Failed to apply patch.
[ View ]
#22 rdf.module08.patch19.34 KBStefan Freudenberg
Failed: 12424 passes, 4 fails, 0 exceptions
[ View ]
#19 rdf.module07.patch19.11 KBscor
Failed: 12405 passes, 4 fails, 0 exceptions
[ View ]
#16 rdf.module06.patch19.46 KBscor
Failed: Failed to apply patch.
[ View ]
#8 rdf.module05.patch17.46 KBscor
Failed: Failed to apply patch.
[ View ]
#6 rdf.module04.patch16.67 KBscor
Failed: Failed to apply patch.
[ View ]
#2 rdf.module03.patch16.05 KBscor
Failed: Failed to apply patch.
[ View ]
#1 rdf.module02.patch13.06 KBscor
Failed: 11563 passes, 0 fails, 12 exceptions
[ View ]
rdf.module1.patch11.66 KBscor
Failed: Failed to apply patch.
[ View ]

Comments

scor’s picture

StatusFileSize
new13.06 KB
Failed: 11563 passes, 0 fails, 12 exceptions
[ View ]

forgot to include the changes on the system module required to get the proper RDFa output on the page level. patch updated.

scor’s picture

StatusFileSize
new16.05 KB
Failed: Failed to apply patch.
[ View ]

adding some theme changes to allow RDFa serialization at the page level in template_preprocess_page() and in theme_username(). code is ugly but we could not figure out a way to do it better without refactoring the theme_username function. we need advice on how to improve the code in the theme layer. Is there any other issue/patch which could make our life easier?

yched’s picture

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 :-)

Frando’s picture

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:

<?php
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

<?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.

scor’s picture

@yched: we picked 'bundle' because we could not find a better term, but we agreed it should be more generic than 'bundle' and should include things like all the content types, comment, user, taxonomy... suggestions: object type, entity type (fago).

@Frando: you're right, we are not happy either with the current RDFa serialization through the theme layer. The optimization with drupal_render() looks much better!

scor’s picture

StatusFileSize
new16.67 KB
Failed: Failed to apply patch.
[ View ]

I rerolled the patch (now with -p) with comments from chx on IRC. improved some docs.

Xano’s picture

Subscribing.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new17.46 KB
Failed: Failed to apply patch.
[ View ]

rerolling. setting to needs review to get more opinions on the issue below.

pending issues:
- improve the way we handle the RDFa in the theme layer (based on Frando's idea)
- should rename 'bundle' to something else. leaving it 'bundle' until we can agree on something more generic. does 'entity type' make sense for everyone?

yched’s picture

'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 ?

fago’s picture

>'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?

scor’s picture

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

right, an 'entity type' (node, user, taxo term) granularity is not enough. The RDF API in core developped to be more granular than that and work on the granularity level of the bundle of the Field API.

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

yes, the uri for the user comes with the $user object (same as the username).

fago’s picture

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.

scor’s picture

this is in the patch of #493056: Add RDF to the user module (listed at the top of this issue):

+      // Generate an RDF resource URI for each user account.
+      foreach ($queried_users as $account) {
+        $account->rdf = array('uri' => 'user/' . $account->uid . '#user');
+      }

(we changed #this to #user).

We kept them separate, do you think these issues should be merged?

fago’s picture

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?

dmitrig01’s picture

Status:Needs review» Needs work

docs for hook_rdf_mapping_alter aren't correct

<?php
+
+
// $Id: $
?>

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

<?php
function rdf_get_mapping
?>

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

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

ummm

<?php
$variables['rdfa_page_about_typeof'] = '';
$variables['rdfa_page_title'] = '';
?>

strange variable names

The TODOs in theme.inc seem pretty necessary.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new19.46 KB
Failed: Failed to apply patch.
[ View ]

thanks fago and dmitrig01 for the reviews!

you were right, fago, the user URI was regenerated in theme.inc which is not consistent. I centralized everything in the rdf.module and added a couple of tests. Note that the URI of a user is now generated in user_load(), but because node_load does not load the user via user_load and instead load its data from the users table directly, we need to add the user URI to the node object in rdf_node_load(). I've put the uri into node->rdf['user']['uri']. I think we'll have to refactor a bit the structure of the mappings in the node object, because we are dealing several objects (user, node) which are flatten into the same Drupal node object.

I addressed all dmitrig01's comments as well:
- fixed docs (added missing & and remove the void return)
- added static cache to $rdf_mapping
- added docs which will hopefully give more sense to the variables (and renamed one of them).
- removed the TODOs, this was from the beginning of the sprint when we were not unsure whether we would make a module or not.
- left the title_rendered TODO as we need to think more on the best way to fix that. any opinion?

<?php
  $variables
['title_rendered']    = $variables['title'] ? '<h1 class="title" id="page-title"' . $variables['rdfa_page_title'] . '>' . $variables['title'] . '</h1>' : '';
?>

and call the internal variable mappings not mapping?

we talked about using mappings (plural) during the sprint but we agreed on the singular name. I'm happy switching to plural if it make more sense to the native English speakers (which I'm not).

catch’s picture

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.

Status:Needs review» Needs work

The last submitted patch failed testing.

scor’s picture

StatusFileSize
new19.11 KB
Failed: 12405 passes, 4 fails, 0 exceptions
[ View ]

rerolling patch.

fago’s picture

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.

scor’s picture

These are the critical tasks currently open for the core RDF API.

1. idea: put the page mapping in rdf module via hook_page_alter()
$page is modifiable by hook_page_alter
each module could implement hook_page_alter to add the mappings to the $page object directly themselves

2. rename $rdfa_page_title to $rdfa_page_title_attributes

3. why don't we have the function date_iso8601 defined?

4. check that all the mappings of node are outputed

5. date can be implemented as hidden span tag

6. to avoid modifying the tpl files too much, wrap the value (e.g. title) with a span tag containing the RDFa. Provide both $field_rdfa and $field_raw in case someone wants to use the raw value (in the case where they want to disable RDFa or use the raw value in a tag attribute for instance).

Stefan Freudenberg’s picture

StatusFileSize
new19.34 KB
Failed: 12424 passes, 4 fails, 0 exceptions
[ View ]

rerolling patch.

lilou’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

bangpound’s picture

The node_type table needs a rdf_mapping column added to prevent SQL JOINs that might harm performance. The node types are cached statically only.

We considered using hook_schema_alter to add rdf_mapping column to the node_types table, but the problem we face is this:

Adding a column to the schema does not automatically add the column to the table when RDF module is enabled, so we'd need to use db_add_field in the hook_install or hook_enable of rdf.module. Then the $ret parameter of the db_add_field function is extraneous. But let's pretend we create an empty array for that parameter and just do nothing with it when the db_add_field function has been executed.

If rdf.module were added to an install profile, the schema for node_type will be returned to the installer with the rdf_mapping column on it. Then when RDF module is enabled, it would attempt to add the column again. What a foolish error!

So the only reasonable option is to add the rdf_mapping column to the node_schema declaration even though the optional rdf module might never be enabled for a site.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new19.46 KB
Failed: Failed to apply patch.
[ View ]

moving the rdf_mapping attachment to the node object in the RDF module. rename drupal_function_exists() to function_exists().

catch’s picture

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?

effulgentsia’s picture

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.
bangpound’s picture

We've discussed the problem of where to store RDF mappings on the different node types. Drupal core appears to use 3 different ways.

  1. variables like node_*_options, comment_*, etc. Some of these variables are complex arrays like RDF mappings. This has the benefit of making the variables configurable with a hook_form_alter.
  2. separate tables à la D6 CCK or vocabuarly node type.
  3. In the node_type table. (the current method)

There's no clear ideal method, and Drupal core seems to be a bit scattered about it. Clearly nobody will SQL SELECT on the RDF mappings directly, so serialization into a single field is not a problem.

scor’s picture

StatusFileSize
new20.9 KB
Failed: 12731 passes, 1 fail, 0 exceptions
[ View ]

lots of fixes from this morning sprint:

* normalize the format of the mappings to a compulsory property key (datatype and callback are still optional), e.g.:
'title' => array(
'property' => array('dc:title'),
)

* moved the code for generation of the RDFa attributes for nodes in teaser mode from theme.inc to rdf_preprocess_node() in the RDF module

* renamed $rdfa_page_title to $rdfa_page_title_attributes (suggested by DamZ) (issue #21-2)

* fixed the missing function date_iso8601 (issue #21-3)

Status:Needs review» Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new1.39 KB
Failed: 12700 passes, 341 fails, 12305 exceptions
[ View ]

delta patch to add other attributes to each block

Status:Needs review» Needs work

The last submitted patch failed testing.

bangpound’s picture

Status:Needs work» Needs review

theme_username is a serious hazard. I wish I had known about it sooner. It needs to be called with more parameters and it needs to be a more usable function.

Changing theme_username to use MODULE_preprocess_HOOK is tempting, but it's not quite possible without violating the principles behind theme preprocess functions. If we clone the $object and change the name property, we're still passing this back in the $variables array. At that point, we're effectively ruling out other modules from having the same access to the theme parameters. rdf_preprocess_username would be the only preprocess function that could safely change the $object passed to theme_username.

The rest of the changes to theme_username really want to affect output, so any refactoring of this patch's changes to theme_username are contingent on #553038: Add postprocess phase for theme() first and foremost.

pwolanin’s picture

Status:Needs review» Needs work
StatusFileSize
new1.07 KB
Failed: Failed to apply patch.
[ View ]

try again with added isset/empty to avoid exceptions

pwolanin’s picture

StatusFileSize
new1.44 KB
Failed: Failed to apply patch.
[ View ]

ignore that one - here's the right one.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new24.1 KB
Failed: 11918 passes, 778 fails, 778 exceptions
[ View ]

correct a silly typo in one of the tests which failed at #30.

adds a great patch by bangpound and effugentsia to improve theme_username() and save us some dirty hacks. The code will get even lighter if #553038: Add postprocess phase for theme() is committed.

scor’s picture

StatusFileSize
new24.11 KB
Failed: 12794 passes, 0 fails, 10 exceptions
[ View ]

the default value of the new parameter for theme_username() was missing and was breaking a lot of tests.

Status:Needs review» Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new2.25 KB
Failed: Failed to apply patch.
[ View ]

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.

effulgentsia’s picture

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.

pwolanin’s picture

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.

ronald_istos’s picture

subscribe

Bèr Kessels’s picture

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'

clemens.tolboom’s picture

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?

Stefan Freudenberg’s picture

StatusFileSize
new28.1 KB
Failed: 12850 passes, 1 fail, 10 exceptions
[ View ]

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

Status:Needs review» Needs work

The last submitted patch failed testing.

kriskras’s picture

StatusFileSize
new684 bytes
Failed: Failed to apply patch.
[ View ]

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

Stefan Freudenberg’s picture

StatusFileSize
new44.08 KB
Failed: Failed to apply patch.
[ View ]

Changed return value of drupal_rdfa_attributes to array.

clemens.tolboom’s picture

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

Stefan Freudenberg’s picture

StatusFileSize
new28.59 KB
Failed: Failed to apply patch.
[ View ]

Rerolled patch.

scor’s picture

Title:core RDF module» RDF #1: core RDF module

changing title per sun's suggestion.

effulgentsia’s picture

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.

Stefan Freudenberg’s picture

StatusFileSize
new28.77 KB
Failed: Failed to apply patch.
[ View ]

And with the correct patch.

Stefan Freudenberg’s picture

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’s picture

Status:Needs work» Needs review
StatusFileSize
new5.18 KB
Failed: Failed to apply patch.
[ View ]

rerolling to take advantage of the freshly committed #565792: Refactor theme_username so that RDF patch can be accepted. We now have moved all the RDF logic out of theme.inc! yay!

fago’s picture

Status:Needs review» Needs work

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.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new22.43 KB
Failed: 12919 passes, 3 fails, 10 exceptions
[ View ]

oops, I have too much patches lying around. here is the patch I meant to post earlier.

Status:Needs review» Needs work

The last submitted patch failed testing.

fago’s picture

Status:Needs work» Needs review
StatusFileSize
new23 KB

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.

fago’s picture

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

<?php
  $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?

effulgentsia’s picture

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

effulgentsia’s picture

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

fago’s picture

@#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.

effulgentsia’s picture

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?

scor’s picture

Thanks for this patch. I'm aware of the theme_username() issue and discussed it with pwolanin, it's because we're missing on the "creator" relationship which is not related to theme_username() in fact, but more the job of the node module which calls it. I'm refactoring a lot of the code following the commits of the 2 theme layer patches and will post the new code asap.

re #62, we already started to move away from the span wrapper thanks to the patch #569362: Document $attributes, $title_attributes, and $content_attributes template variables so I think we should keep going in the same direction and be consistent.

fago’s picture

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?

fago’s picture

>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.

scor’s picture

the title of the node is ok in teaser mode but not displayed in full node more because it is pushed in the page template. To deal with this special case we could either:
1. add a empty span in the node.tpl.php like:
<?php

or
2. fiddle with page.tpl.php?
or
3. add a wrapper around the title before it is sent to page.tpl.php

I think I like 1. better as it keep it all in the node template file.

Also, we've decided to avoid altering the page template as much as possible but instead to use the block tpl file to add any attributes specific to the whole page (since the main content is contained in the block system-main). It also allows other blocks to potentially be annotated with some RDFa.

fago’s picture

StatusFileSize
new22.95 KB

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.

moshe weitzman’s picture

"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.

fago’s picture

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.

scor’s picture

StatusFileSize
new21.6 KB
Failed: 12986 passes, 4 fails, 1416 exceptions
[ View ]

The patch has been revamped to take advantage of the new attributes variables of the tpl files.

- All the RDFa is now added in rdf_preprocess_node():
--> node container RDFa (about and typeof, see documentation)
--> node title
--> date of the node
--> relation between the node and its author
- correct some minor spelling mistakes of some keys in the RDF mappings ('properties' => 'property')
- fix date_iso8601() not to use strtotime()
- remove extra variables from page.tpl.php (now done in node.tpl.php)
- enable the RDF module in the default profile

@fago: date_iso8601() was left without rdf namespace because not being RDF specific it should maybe be located in common.inc.

This how the date and author relationship RDFa markup are added:

<?php
  $date_attributes_array
= drupal_rdfa_attributes($variables['rdf_mapping']['created'], $variables['created']);
 
$variables['date'] = '<span ' . drupal_attributes($date_attributes_array) .  '>' . $variables['date'] . '</span>';

 
$author_attributes_array = array('rel' => $variables['rdf_mapping']['uid']['property']);
 
$variables['name'] = '<span ' . drupal_attributes($author_attributes_array) .  '>' . $variables['name'] . '</span>';
?>

The date and the author relation are both added via a span wrapper. It's not ideal but it works RDFa wise. Let's see how we can improve the code from a theming point of view in maybe using the ideas from #574954: Add theme_timestamp for RDFa?. node.tpl.php needs to be altered to allow HTML in the date output which is not ideal and might have some security implications.

fago’s picture

StatusFileSize
new27.45 KB

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?

Status:Needs review» Needs work

The last submitted patch failed testing.

fago’s picture

Status:Needs work» Needs review

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!

fago’s picture

Status:Needs review» Needs work
scor’s picture

Status:Needs work» Needs review
StatusFileSize
new21.73 KB
Failed: 12989 passes, 3 fails, 1376 exceptions
[ View ]

Add multivalue support for fields. (see #493094: RDF #3: field module for specific questions related for the Field API).
fix minor property name spelling and remove strtotime() from the tests. all RDF tests are now passing on my machine.

Status:Needs review» Needs work

The last submitted patch failed testing.

scor’s picture

StatusFileSize
new24.92 KB
Failed: 13801 passes, 5 fails, 40 exceptions
[ View ]

- rdf_get_mapping() now incorporates the accurate RDF mappings stored in node_type table. So the workflow of rdf_get_mapping() is now: optional bundle defined in hook_rdf_mapping() overrides the default node RDF mapping, which can be altered via drupal_alter().
- added preprocess function in the rdf module for the comments and some tests.
- fix minor typo in the date_iso8601() documentation.
- add the title of a node in full node mode in the head tag of the HTML since it is not outputed using the regular node.tpl.php. see #12 #493086: RDF #2: node module

Stefan Freudenberg’s picture

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.

scor’s picture

a div tag with the css rule display:inline is more appropriate. I asked JohnAlbin during DrupalCon and he could not think of a better way of working around this. That's what we've been using in the RDFa patch for CCK.

Dries’s picture

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.

Cliff’s picture

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.

bdunwood’s picture

Subscribe.

aaron’s picture

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

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new34.52 KB
Failed: Failed to install HEAD.
[ View ]

The RDF patches haven't been updated for a while as we were working on a slightly different approach, we now (1) use hook_entity_info to make the mapping available for each entity and we store all the mappings into a dedicated table rdf_mapping. This has lead to some changes in the API which I will detail here. In the end, we now have a more generic mapping storage and RDF can be used by any entity!

New mapping storage (RDF mapping CRUD API)

We don't store the mapping in the node table anymore, but instead in a dedicated table rdf_mapping, allowing us to store any entity type RDF mapping (as opposed to node-only before). There pretty much no performance overhead as the data stored in the rdf_mapping table is cached in the entity_info cache with the rest of the information about entities. We don't need to hack the node module anymore (in fact we were able to move most of the previous logic from node.module to rdf.module). This approach is more generic: any entity type can now declare RDF mapping for its bundles which are automatically stored in the RDF mapping storage upon installation. They can also define default mapping for bundles which don't have predefined mapping.

The other great thing about this new RDF mapping storage is that it allows to change the entities/bundles mappings either programmatically or via an UI the same way the Field UI allows to maintain fields. Imagine creating a new node type with its fields and map all this to RDF directly. All the nodes will then get annotated in RDFa automatically: you see the potential :)

RDF mapping within the entity_info

The other major improvement with this patch is that we now rely heavily on the centralized entity info cache. The content of the table rdf_mapping is added to each entity bundle information and then cached. We use hook_entity_info_alter() to add the RDF mapping to each respective bundle.

In a nutshell, here is the new workflow to help you understand the new approach.

1. Modules declare the RDF mapping for the entities and bundles they define via hook_rdf_mapping().

2. All these mappings are stored in the rdf_mapping table upon installation in rdf_modules_installed(), which is an implementation of hook_modules_installed() (writing the tests for this gave us a headache, see #594234: hook_modules_installed() not invoked during tests). If there is no specific mapping for a bundle, then the default entity type mapping is used (if defined).

3. Sometimes, entities type and bundles are only coded in an installation profile (as opposed to hook_node_info() in .module). It's the case of the Article and Page node types in the default profile. The RDF mapping CRUD API can then be used to define the appropriate mappings (see an example in default.install).

4. Once we have all the various mappings stored in the database, we include them in the entity_info object by implementing hook_entity_info_alter(). This object is then cached in the database and statically.

5. Whenever we need the mapping of a specific bundle, we call rdf_get_mapping() which extracts the relevant mapping from entity_get_info(). Right now we add the mapping to each object by implementing hook_node_load(), hook_user_load() etc, but we want to centralize this in the entity loading process. Please let us know if you have any idea on how to do this!

6. The rest of the workflow has not changed. Each mapping is added via the theme preprocess functions to the generated HTML to produce RDFa

Not all tests are working yet and we still need to improve the docs, but I thought it was important at this stage to get some feedback on the approach. We'll be back soon with an improved patch, in the meantime please review and let us know what you think of this workflow. Be quick, the end of the code slush is around the corner!

For those like Cliff willing to get up to speed, focus on the latest patch and just ask if you have any question! I'll be happy to explain or point you to the right comment if any.

Re. Dries comments at #84:

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?

To keep things more simple, we've removed hook_rdf_mapping_alter() for now as this can be achieve by implementing hook_entity_info_alter().

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.

That's lame, agreed! I changed to Allows to map the site data structure to RDF and export it in RDFa., hoping it's a bit more meaningful from a end-user point of view.

re RdfException , dictionnary and page level mapping, these were removed during the refactoring described above.

catch’s picture

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

scor’s picture

@catch, what do you think of using hook_entity_info_alter() for adding the mapping to each bundle? Is there a better way of doing that (I tend to avoid using alter hooks in core and leave that for contrib but if that's the only way for entity_info it's fine).

At the moment to attach the mapping to each bundle, we have to implement a hook_{entity}_load for each entity

<?php
/**
 * Implements hook_node_load().
 */
function rdf_node_load($nodes, $types) {
  foreach (
$nodes as $node) {
   
$node->rdf_mapping = rdf_get_mapping('node', $node->type);
  }
}
?>

is there a way to centralize that in entity.inc and do it automatically for all entities? that would save us even more code :)

catch’s picture

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.

fago’s picture

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.

scor’s picture

@catch

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).

We removed hook_rdf_mapping_alter() in favor of a more simple RDF module because the same thing can be achieved via hook_entity_info_alter(). However the order in which the implementations of hook_entity_info_alter() are called matters, i.e. we should ensure that rdf_entity_info_alter() is called first before any other module try to alter the RDF mapping. Is there any way to do that other than tweaking the value of weight in the system table?

@fago

As a bundle is a further specialisation of an entity, shouldn't be the rdf-mapping additional, thus not replacing the entity-mapping?

Can you elaborate a little on your idea? Do you mean that if no mapping is given for a bundle, then we should not use the default entity mapping and leave these blank? The default entity mappings are there as a shortcut for developers so they don't have to re-specify everything in each bundle: take for example the title field, in most cases for a node type, it's very likely to be dc:title, so I don't have to say that in each of my bundle. For a user entity type, on the other hand, in most cases the title (username here) will be foaf:name. And same for the date. As a result, in the bundles, you only specify what really is specific to a bundle (well, you can also reitarate what is defined in the default entity mapping if you like, but that's more verbose). This is for example all what the blog module has to define for its mapping:

<?php
/**
 * Implementation of hook_rdf_mapping().
 */
function blog_rdf_mapping() {
  return array(
    array(
     
'type' => 'node',
     
'bundle' => 'blog',
     
'mapping' => array(
       
'rdftype' => array('sioct:Weblog'),
      )
    ),
  );
}
?>

all the rest of the mappings (title, body, etc.) are inherited from the default node mapping.

Secondly, another benefit I see for the defaults is that anyone who hasn't cared to give mapping for a bundle will get (very generic) mapping for free, which is better than nothing. You could technically remove these mappings by implementing hook_entity_info_alter() and remove the rdf_mapping key for each bundle you do not want to be mapped to RDF.

I'm working on the other comments from fago.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new32.61 KB
Failed: Failed to install HEAD.
[ View ]

- Improve rdf_preprocess_node() so it does not produce notices in node preview mode (the RDF mapping are not available in preview mode because hook_node_load() is not invoked).

- Remove a redundant RDF mapping definition from rdf_test.module. For the tests,we now reuse the entity 'test_entity' and and the bundle 'test_bundle' which are created by the field_test module.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new674 bytes
Failed: Failed to apply patch.
[ View ]

removing the test for the comment.module since there are not finished and it will be part of a different patch.

scor’s picture

StatusFileSize
new28.65 KB
Failed: Failed to apply patch.
[ View ]

right patch attached. removing the tests for the comment.module since there are not finished and will be part of a different patch.

scor’s picture

StatusFileSize
new26.6 KB
Failed: Failed to install HEAD.
[ View ]

- extended the RDF mapping definition test to include all the different ways to define an RDF mapping for a content type. We now test mapping defined either:
1. via hook_rdf_mapping()
2. in a profile or an .install file via the RDF mapping CRUD API
3. via the default entity type mapping (when no mapping is defined for a bundle).

- added rdf_test.install to include a mapping defined in an install file (test added in rdf.test).

- removed the 'another_test_type' from rdf_test.module since it's no longer useful.

moshe weitzman’s picture

Status:Needs review» Needs 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.

scor’s picture

@moshe: we actually included the RDF module in default.info at #88 and that's when the bot started to misbehave. On my localhost, the testing module picks up any test whether the module is enabled or not. There are several modules in core which are not included in the default profile and their tests are executed regardless. (syslog, book, contact, etc.). Syslog for example is not included or required by any other module and gets its test executed. I think the issue with the default profile is a separate one and independent from the tests not being executed. Since the test bot does not give any error message and I don't have access to the bot logs, I've created #599086: Testbot futzing (please ignore this issue) to try to debug the test bot.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new28.24 KB
Failed: 13741 passes, 0 fails, 568 exceptions
[ View ]

We're back on track! The reason why the patch failed to install on the testing servers is because they appear to be using MySQL 5.0 which limits the key size to 1024 bytes, while our localhost setups on which we were testing use MySQL 5.1 which allows 3072 byte keys. And why are we reaching the key size limit? because we create a primary key on the pair (type, bundle) to ensure their unicity in the table. type is a varchar(255) as defined elsewhere in core and bundle is varchar(128) which is too big for a paired primary key (or a unique key for that matter). See more details on the debugging. I have unpublished some comments above to avoid too much noise in this issue so that we bring back the focus on RDF.

I have switched the length of 'type' to 128 in the meantime and I have opened #600490: Switch entity type column from varchar(255) to varchar(128).

Dries’s picture

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.

clemens.tolboom’s picture

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 :)

clemens.tolboom’s picture

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 :

<?php
      $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

<?php
 rdf_preprocess_node
(&$variables) {
?>

because the notice are from view node/1 and not from frontpage

Status:Needs review» Needs work

The last submitted patch failed testing.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new29.83 KB
Failed: Failed to install HEAD.
[ View ]

- #557292: TF #3: Convert node title to fields introduced a lot of changes in core (including the notices from #114). These should be fixed in this patch.

- Merged hook_rdf_mapping_default() into hook_rdf_mapping(). Now you can define RDF mapping for specific bundle and default entity mapping in the same hook. When defining a default entity mapping, use RDF_DEFAULT_BUNDLE as bundle value.

<?php
function node_rdf_mapping() {
  return array(
    array(
     
'type' => 'node',
     
'bundle' => RDF_DEFAULT_BUNDLE,
     
'mapping' => array(
       
'rdftype' => array('sioc:Item', 'foaf:Document'),
       
'title'   => array(
         
'property' => array('dc:title'),
        ),
       
'created' => array(
         
'property' => array('dc:date', 'dc:created'),
         
'datatype' => 'xsd:dateTime',
         
'callback' => 'date_iso8601',
        ),
       
'body'    => array(
         
'property' => array('content:encoded'),
        ),
      ),
    ),
  );
}
?>

- Refactored rdf_read_mapping() to always return an array.

- Fixed a bug in PHP where the DATE_ISO8601 constant and date('c') mismatch and therefore produce invalid RDFa markup. see http://bugs.php.net/bug.php?id=47184

- Fixed _rdf_get_default_mapping() to get the default bundle mappings via hook_rdf_mapping() without relying on the database because we need to have them readily available.

- Extended the RDF mapping definition functionality test to check more RDFa attributes in the page output.

catch’s picture

+++ 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?

dmitrig01’s picture

StatusFileSize
new3.42 KB
Failed: Failed to install HEAD.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch failed testing.

Dries’s picture

Status:Needs work» Needs review

@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 picture

Status:Needs review» Needs work
catch’s picture

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().

scor’s picture

Status:Needs work» Needs review
Issue tags:+Exception code freeze

- remove all the hook_TYPE_load() and use the new hook_entity_load() instead (#605442: Add a generic hook_entity_load() was just committed). We still need to switch on a per entity_type basis to extract the bundle name so that's not ideal yet.
- adapted rdf_preprocess_comment() since the date and timestamps properties have been renamed to created.
- removed _rdf_generate_user_uri which is not needed any more.

@dmitrig: waiting on your patch, I don't think you posted the right patch in #118.

scor’s picture

StatusFileSize
new28.86 KB
Failed: Failed to install HEAD.
[ View ]
catch’s picture

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.

webchick’s picture

Subscrrrrrrribing!

scor’s picture

StatusFileSize
new28.47 KB
Failed: 13265 passes, 3 fails, 0 exceptions
[ View ]

catch: you're awesome!

Status:Needs review» Needs work

The last submitted patch failed testing.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new29.3 KB
Failed: Failed to install HEAD.
[ View ]

- added rdf_install() to trigger rdf_modules_installed() in the installer.
- changed rdf_modules_installed() so we only save the bundle mappings to the database (with docs): While both default entity mappings and specific bundle mappings can be defined in hook_rdf_mapping(), we do not want to save the default entity mappings in the database because users are not expected to alter these. Instead they should alter specific bundle mappings which are stored in the database so that they can be altered via the RDF CRUD mapping API.
- fixed the default mapping of user.module: there is no 'title' field, it's 'name' in the case of the user.

@clemens.tolboom #113: bundles is a new term introduced by Drupal 7. You can see it as an extension of concept of content types in Drupal 6, except it does not only applies to node but any entity type (user, comment, term, etc.). It is not RDF specific.

I cannot reproduce the 3 fails the bot is complaining about on my localhosts.

catch’s picture

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?

scor’s picture

StatusFileSize
new31.41 KB
Failed: 13271 passes, 0 fails, 297 exceptions
[ View ]

- added support for theme_username. there is a lot of inline documentation. Almost all the attributes are easy to fill in, except the resource one which requires the parent element URL. $variables is a pain to work with as it can be anything. Right now I have a switch but only for node and comment. I'm looking for a better way to extract the path of an entity. $variables does not even include the type of entity it contains (only the bundle name), making it hard/impossible to guess the type entity you're dealing with. An alternative is to add span around the output of theme_username, as this would not require the parent URL anymore. See patched theme_username: all we would need to do is set $variables['parent_attributes_array'] in rdf_preprocess_username to have this wrapping span.

Status:Needs review» Needs work

The last submitted patch failed testing.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new31.41 KB
Failed: Failed to install HEAD.
[ View ]

tiny error in the previous patch.

scor’s picture

StatusFileSize
new32.89 KB
Failed: Failed to install HEAD.
[ View ]

- added the default mapping for the comment entity
- added an about attribute to comment (and improved code). still need to work out a way to annotate the body of comment because this sucker is not a Field. One solution is #538164: Comment body as field.

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

@catch #131: the title is still a particular case in the node.tpl.php and is not displayed like a normal field.

mlncn’s picture

StatusFileSize
new35.33 KB
Failed: Failed to install HEAD.
[ View ]

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).

mlncn’s picture

StatusFileSize
new35.3 KB
Failed: Failed to apply patch.
[ View ]

Everything the last patch said it had.

effulgentsia’s picture

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.

effulgentsia’s picture

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.

Status:Needs review» Needs work

The last submitted patch failed testing.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new40.1 KB
Failed: 13489 passes, 0 fails, 448 exceptions
[ View ]

RDF API:
- added short introduction documentation to rdf.module (with link RDFa primer)
- fix the mapping structure example in the docs of rdf.module
- improved file level documentation and added a doc block for RDF_DEFAULT_BUNDLE.
- add theme_rdf_template_variable_wrapper() and friends to support for clean wrapping of content like date (themers can alter it), thanks to effulgentsia and sun (#139).

comment:
- added RDF mapping for the parent (pid) of the default mapping for comment.
- added support for describing the relation from a comment to its parent.

taxonomy:
- made rdf_preprocess_field_formatter_taxonomy_term_link() use the mapping from entity.
- made theme_field_formatter_taxonomy_term_link() shorter.
- implemented preprocess hook for taxonomy_term_link field formatter.
- cleaned up taxonomy_rdf_mapping()
- implemented the possibility to add RDFa attributes to taxonomy term links.

system.module:
- removed dc namespace and renamed dcterms to dc. added the SIOC types and CommonTag namespaces.
- added the version attribute to the html tag of the page (this was added recently to the RDFa specs).

Note that I'm not the only one working on this: stefan-agaric (taxonomy!) and benjamin-agaric are doing a lot too.

Status:Needs review» Needs work

The last submitted patch failed testing.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new40.69 KB
Failed: 13501 passes, 0 fails, 10 exceptions
[ View ]

the user.module hunk was missing in our script to make the patch. added that.

Status:Needs review» Needs work

The last submitted patch failed testing.

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new40.67 KB
Failed: Failed to install HEAD.
[ View ]

fixed rdf_preprocess_username(): do not assume there is always a mapping for uid because sometimes this function is used outside the context of a node (contact.module for example)

effulgentsia’s picture

@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’s picture

@effulgentsia: so far I only used it for the date use case and I'm working on the others. I am not able to apply theme_rdf_template_variable_wrapper() on the body of comment (which unfortunately is not a Field). This is the workaround I'm using at the moment:

<?php
function rdf_comment_view($comment, $build_mode) {
 
// Here we have no solution other than wrapping the body content with a div
  // @todo This would not be necessary if the comment body was a field,
 
if (!empty($comment->rdf_mapping['body']['property'])) {
   
$attributes['property'] = $comment->rdf_mapping['body']['property'];
   
$comment->content['comment_body']['#prefix'] = '<div' . drupal_attributes($attributes) . '>';
   
$comment->content['comment_body']['#suffix'] = '</div>';
  }
}
?>

I wanted to get rid this implementation of hook_comment_view() and use theme_rdf_template_variable_wrapper() instead and this is what I tried in rdf_preprocess_comment():

<?php
 
if (!empty($comment->rdf_mapping['body']['property'])) {
   
$attributes['property'] = $comment->rdf_mapping['body']['property'];
   
$variables['rdf_variable_attributes_array']['content'] = $attributes;
  }
?>

and I gotFatal error: Only variables can be passed by reference in themes/garland/comment.tpl.php on line 23. get on IRC and we can talk further!

effulgentsia’s picture

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.

scor’s picture

StatusFileSize
new46.85 KB
Failed: Failed to apply patch.
[ View ]

- removed date_iso8601() from rdf.module as it now lives in common.inc, committed by Dries earlier today.
- committed patch #608036: Add content_attributes variable for tpl files, so RDF can be implemented better.
- implemented the new approach allowed by patch #608036: Add content_attributes variable for tpl files, so RDF can be implemented better. which solves #147. Note that while we save a level of wrapping, we still don't solve the lack of comment body and title of fields. The main restriction is that the extra fields attached to comments won't have their RDFa readable by the parsers. The reason is that we set the @property RDFa attribute on the whole $content in comment.tpl.php to capture the body of the comment. This body will also contain the eventual comment fields which will be ignored by RDFa processing rules as a result of the @property being set on the parent HTML element. If comment body was a field (like nodes) we would not have this problem: #538164: Comment body as field.
- added documentation about the $date due to the placeholder change in node.tpl.php. The date is HTML.
- fixed code style in rdf_preprocess_username().
- postComment call used wrong parameter order in RdfaCommentTestCase::testAttributesInMarkup().
- comment RDF test was looking for dc:creator as the property for comment author, but the RDF mapping for comment author is foaf:name. updated test to reflect mapping.

thanks to bangpound who joined us today to help with the tests.

Dries’s picture

scor’s picture

StatusFileSize
new44.79 KB
Failed: Failed to apply patch.
[ View ]

- added theme_rdf_metadata() to allow for extra RDFa metadata output inside templates. This is used to establish the link between each comment and its parent node and parent comment if it exists.
- this reroll does not include the now committed #608036: Add content_attributes variable for tpl files, so RDF can be implemented better.. thank you Dries!

sun’s picture

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?

sun’s picture

Status:Needs review» Reviewed & tested by the community

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. :)

scor’s picture

Status:Reviewed & tested by the community» Needs review

thanks sun for this thorough review. I'll have a go at it asap, most likely as a follow up patch. already using diff -up, just that the new files don't have any function above them :)

- added a key 'type' to the RDF mapping field structure. This key is used when generating the RDFa markup for fields which differs depending on whether the value of the field is of type text or resource (another bundle or URI). updated documentation to account for this new key.
- added rdf_preprocess_user_profile() to annotate the user profile page.
- added $attributes to user-profile.tpl.php (a core wide patch should be created as follow up).
- added blog_rdf_mapping() to blog.module
- added forum_rdf_mapping() to forum.module (that is all you need to enable RDFa on forum, and you get all the nodes, comments and their forum containers as RDFa!)

setting back to needs review due to the additions above.

scor’s picture

StatusFileSize
new48.42 KB
Failed: Failed to apply patch.
[ View ]

and I almost went to bed without posting the patch!

mlncn’s picture

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.
scor’s picture

StatusFileSize
new48.54 KB
Failed: Failed to apply patch.
[ View ]

- 'property' (or 'properties' for that matter) is somewhat vague and confusing. It can be an object or array property, or the property attribute in RDFa. Well, in the case of the RDF mappings, it's none of these. It's an RDF property, which is also known as predicate, and it's this word that we're going to use from now on. So whenever you see 'property' in the RDF module, it'll be because you are dealing with RDFa markup. At the RDF level we'll call these predicates. Note that once output in RDFa, these predicates will end up either in a property attribute for text type object (like title or body) and in a rel or rev attribute for a resource type object (author of a node, tag, etc.). I've explained that in the docs but let me know if it's not clear.
- @sun it's ok to use the empty element shorthand for span and div in XHTML+RDFa, see example at http://www.w3.org/TR/rdfa-syntax/#sec_6.1.1.5.1.
- removed t() tranlation function from the getInfo in comment.test and rdf.test
- fixed missing PHPDoc for rdf_preprocess_field_formatter_taxonomy_term_link()

Dries’s picture

Status:Needs review» Needs work

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.

webchick’s picture

Issue tags:+Needs Documentation

Tagging for the upgrade guide. GREAT job folks!!

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new587 bytes
Passed: 14276 passes, 0 fails, 0 exceptions
[ View ]

changelog patch.

effulgentsia’s picture

StatusFileSize
new11.33 KB
Unable to apply patch rdf-theming-cleanup-493030-161.patch
[ View ]

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.

Dries’s picture

Status:Needs review» Fixed

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

sun’s picture

Status:Fixed» Needs review
StatusFileSize
new35.5 KB
Failed: Failed to install HEAD.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new48.35 KB
Failed: Failed to install HEAD.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new48.36 KB
Failed: Failed to run tests.
[ View ]

sorry, I forgot the serialize().

Status:Needs review» Needs work

The last submitted patch failed testing.

scor’s picture

+++ modules/blog/blog.module 19 Oct 2009 19:03:53 -0000
@@ -185,7 +185,7 @@ function blog_block_view($delta = '') {
+ * Implement hook_rdf_mapping().

I thought we were supposed to use the third person in the Doxygen: Implements hook_rdf_mapping()

+++ modules/rdf/rdf.module 19 Oct 2009 21:25:34 -0000
@@ -310,68 +263,100 @@ function rdf_entity_info_alter(&$entity_
+function rdf_attributes($mapping, $data = NULL) {

using the rdf namespace is good, but these are specifically rdfa attributes: rdf_rdfa_attributes()?

+++ modules/rdf/rdf.module 19 Oct 2009 21:25:34 -0000
@@ -310,68 +263,100 @@ function rdf_entity_info_alter(&$entity_
+    // The mapping expressed the relationship between a resource and some

spotted a typo which was in the original patch: should be expresses.

This review is powered by Dreditor.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new48.9 KB
Passed: 14287 passes, 0 fails, 0 exceptions
[ View ]

Pass again? :)

sun’s picture

StatusFileSize
new46.83 KB
Passed: 14261 passes, 0 fails, 0 exceptions
[ View ]

Incorporated scor's review from #169.

yched’s picture

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.

scor’s picture

@yched: no problem with that, we took into account the bundle namespace issue ;)

JerryH’s picture

>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.

scor’s picture

StatusFileSize
new47.49 KB
Passed: 14437 passes, 0 fails, 0 exceptions
[ View ]

@JerryH can you open a follow up issue and post a link here please?

Would have RTBC'ed sun's patch but could not help fixing some of the docs (most switching to third person along the lines of #169). please someone else RTBC.

dman’s picture

THANKS for all the work here!

sun’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new51.53 KB
Passed: 14381 passes, 0 fails, 0 exceptions
[ View ]

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.

webchick’s picture

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.

scor’s picture

Improve documentation, code style and logic in CRUD. Reorder functions in rdf.module.

sun’s picture

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.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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.

sun’s picture

Status:Fixed» Needs review
StatusFileSize
new7.19 KB
Passed: 14428 passes, 0 fails, 0 exceptions
[ View ]

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.

scor’s picture

Status:Needs review» Reviewed & tested by the community

yes, we discussed these changes on IRC and agreed it makes the API more consistent. sorry webchick for the extra work :( thanks sun for the follow ups :)

webchick’s picture

Status:Reviewed & tested by the community» Needs work

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

sun’s picture

+++ 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?

webchick’s picture

duh. totally misread your comment. :)

Committed this to HEAD too.

scor’s picture

Status:Needs work» Fixed

thanks webchick!

scor’s picture

Status:Fixed» Needs work

oops, we still need documentation for this.

scor’s picture

scor’s picture

Status:Needs work» Needs review
StatusFileSize
new497 bytes
Passed on all environments.
[ View ]

- @sun it's ok to use the empty element shorthand for span and div in XHTML+RDFa, see example at http://www.w3.org/TR/rdfa-syntax/#sec_6.1.1.5.1.

I was wrong: this is true in the pure XHTML sense where pages are served as application/xhtml+xml. However since Drupal is still serving XHTML as text/html, we need to pay attention to the HTML compatibility guideline C.3 of the XHTML1 specs which is inherited by XHTML+RDFa 1.0. That means we cannot use the empty element shorthand for span tags. patch attached.

effulgentsia’s picture

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?

scor’s picture

Why applying the display: none rule on these empty spans? I'd rather keep these as short as possible. is it a measure to prevent any other css rule to spill on these spans? span tags are be default non obtrusive, and changing that in CSS is IMO bad design, unless you know good reasons for doing so?

scor’s picture

StatusFileSize
new1.35 KB
Passed on all environments.
[ View ]

@effulgentsia I see your point. The comment needed to be updated since the markup should now be valid in HTML doctypes too. Let me know how that sounds to you.

sun’s picture

+++ 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.

effulgentsia’s picture

StatusFileSize
new2.73 KB
Passed on all environments.
[ View ]

@scor, @sun: how about this?

scor’s picture

Looks good except I'm still not sure why we need to add a class to such an empty span. Is it proven that an empty span could break a theme? Have you seen such cases? To me the span tag is a very "weak" tag in the sense that it should not alter any layout. If a theme contains a CSS rule for all span tags then I would think this theme is flawed, though I'm not a themer so I'm not 100% sure here.
EDIT: and if a themer *really* wanted to set a CSS rule for all span tags and needed to add a class to catch these empty tags, she could always override theme_rdf_metadata() (but again I argue this sounds like a unusual case).

sun’s picture

Status:Needs review» Reviewed & tested by the community

Cool.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Yeah, I think this makes sense.

Committed to HEAD. Are we done here..?

scor’s picture

I think we're all set for this issue, given that #635282: Help File Fixup: dblog, php, rdf was also just committed, thanks webchick ;)

effulgentsia’s picture

Status:Fixed» Needs review

thekids requested that failed test be re-tested.

scor’s picture

Status:Needs review» Fixed

what's that? this is an old patch which was committed long time ago. Are spammers now bugging our testing bot?

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.