Improve rdf.module documentation [#815866]

Comment	File	Size	Author
#13	815866_rdf_improve_docs_5.patch	29.5 KB	psynaptic

#10	815866_rdf_improve_docs_4.patch	29.39 KB	psynaptic

#3	815866_rdf_improve_docs_3.patch	7.58 KB	scor

	rdf_improve_docs.patch	7.53 KB	scor

Comment #1

scor CreditAttribution: scor commented 2 June 2010 at 11:05

Status:

Active

» Needs review

Log in or register to post comments

Comment #2

aspilicious CreditAttribution: aspilicious commented 2 June 2010 at 12:00

Why an @see to an inknown link?

http://www.w3.org/TODO-RDF-LINK

Log in or register to post comments

Comment #3

scor CreditAttribution: scor commented 2 June 2010 at 14:57

File	Size
815866_rdf_improve_docs_3.patch	7.58 KB

@aspilicious: good point :)

also improved rdf_help().

Log in or register to post comments

Comment #4

aspilicious CreditAttribution: aspilicious commented 2 June 2010 at 14:59

inknown ==> unknown :p

Log in or register to post comments

Comment #5

effulgentsia CreditAttribution: effulgentsia commented 3 June 2010 at 17:15

+++ modules/rdf/rdf.module
@@ -22,16 +22,18 @@ function rdf_help($path, $arg) {
+ * classes and properties. This module takes care of injecting these mappings
+ * into variables available to themers in the theme functions as well as in .tpl
+ * files. All Drupal core themes are coded to be RDFa compatible.

How about "... into variables available to theme functions and templates. All ..."

+++ modules/rdf/rdf.module
@@ -398,6 +400,8 @@ function rdf_entity_load($entities, $type) {
+    // Pages with many comments can show poor performance so the values below
+    // are processed ahead of time so they may be cached as part of the entity.
     $comment->rdf_data['date'] = rdf_rdfa_attributes($comment->rdf_mapping['created'], $comment->created);
     $comment->rdf_data['nid_uri'] = url('node/' . $comment->nid);

I think this comment is fine, but I may open a separate issue questioning this strategy. From what I can tell, this only improves performance if you have a contrib module enabled that caches entity loads across page requests, so my inclination is that this optimization belongs in a contrib module. Anyway, separate issue. Given the code in HEAD, I like that this patch adds code comments explaining the rationale.

78 critical left. Go review some!

Log in or register to post comments

Comment #6

effulgentsia CreditAttribution: effulgentsia commented 3 June 2010 at 17:39

+++ modules/rdf/rdf.module
@@ -398,6 +400,8 @@ function rdf_entity_load($entities, $type) {
+    // Pages with many comments can show poor performance so the values below
+    // are processed ahead of time so they may be cached as part of the entity.

How about: "This information isn't needed until rdf_preprocess_comment(), but set it here to optimize performance for websites that implement an entity cache."

Powered by Dreditor.

Log in or register to post comments

Comment #7

psynaptic CreditAttribution: psynaptic commented 14 September 2010 at 18:46

I'm working on a re-roll of this patch. The documentation for RDF_DEFAULT_BUNDLE is missing some words:

Implementations of hook_rdf_mapping() should use this constant for the 'bundle' key when defining a generic set of RDF mappings for an entity type. The defined with this constant (the default bundle mapping ) will be used when a new bundle is installed if there is no mapping defined for the bundle.

I'm trying to work out exactly what RDF_DEFAULT_BUNDLE does but seems to be defined as an empty string. It is used in rdf_mapping_load in this code:

function rdf_mapping_load($type, $bundle = RDF_DEFAULT_BUNDLE) {
  // Retrieves the bundle specific mapping from the entity info.
  $entity_info = entity_get_info($type);
  if (!empty($entity_info['bundles'][$bundle]['rdf_mapping'])) {
    return $entity_info['bundles'][$bundle]['rdf_mapping'];
  }

That would turn into:

return $entity_info['bundles']['']['rdf_mapping'];

I'm a bit confused at why this is so. Could someone please clarify this for me.

I'd also like to know what happens when custom modules set their bundle key to RDF_DEFAULT_BUNDLE. By the sound if it there are some default mappings that can be applied to custom bundles if necessary.

Log in or register to post comments

Comment #8

scor CreditAttribution: scor commented 14 September 2010 at 19:15

The documentation for RDF_DEFAULT_BUNDLE is missing some words:

Good catch, the second sentence is no good. What it should say is that each specific bundle will inherit the entity_type default mappings unless the bundle specifies its own mappings. A good example is in node.module, where we specify dc:title for the title field so that all bundles of type node inherit this mapping without the need to explicitly declare it (though it's possible to override this if necessary).

I'm trying to work out exactly what RDF_DEFAULT_BUNDLE does but seems to be defined as an empty string.

yes, and rdf_mapping_load() will simply return _rdf_get_default_mapping($type) if you call rdf_mapping_load() with just $type.

I'd also like to know what happens when custom modules set their bundle key to RDF_DEFAULT_BUNDLE. By the sound if it there are some default mappings that can be applied to custom bundles if necessary.

Well, modules defining their own custom entity type should define an RDF mapping for their entity type and optionally mappings for the bundles they defines, though bundle mappings are not so crucial if all the bundles should inherit the same mappings. Bundle specific mappings are only necessary of there are some fields in some bundles which do not follow the default entity type mapping.

Log in or register to post comments

Comment #9

psynaptic CreditAttribution: psynaptic commented 14 September 2010 at 19:21

That makes it much clearer for me, thanks scor!

I'll try to make this crystal clear in the documentation.

Log in or register to post comments

Comment #10

psynaptic CreditAttribution: psynaptic commented 15 September 2010 at 15:30

File	Size
815866_rdf_improve_docs_4.patch	29.39 KB

Re-rolled patch attached.

Log in or register to post comments

Comment #11

15 September 2010 at 16:01

Status:

Needs review

» Needs work

The last submitted patch, 815866_rdf_improve_docs_4.patch, failed testing.

Log in or register to post comments

Comment #12

scor CreditAttribution: scor commented 15 September 2010 at 16:21

Great thorough work psynaptic! Only some minor fixes to do below.

+++ modules/rdf/rdf.module
@@ -184,7 +188,7 @@ function _rdf_get_default_mapping($type) {
  *   The bundle the mapping refers to.
  *
  * @return
- *   A RDF mapping structure or FALSE if no record was found.
+ *   An RDF mapping structure or an empty array if no record was found.
  */
 function _rdf_mapping_load($type, $bundle) {
   $mapping = db_select('rdf_mapping')

good catch!

+++ modules/rdf/rdf.module
@@ -158,14 +162,14 @@ function _rdf_get_default_mapping($type) {
-        if ($mapping['bundle'] === RDF_DEFAULT_BUNDLE) {
+        if ($mapping['bundle'] === 2) {
           $default_mappings[$mapping['type']] = $mapping['mapping'];

huh? Note the failed review above is most likely related to that.

+++ modules/rdf/rdf.module
@@ -684,14 +696,18 @@ function rdf_preprocess_comment(&$variables) {
   if (!empty($comment->rdf_mapping['pid'])) {
-    // Relation to parent node.
+    // Adds the relation to parent node.

Adds the relation to *the* parent node?

+++ modules/rdf/rdf.module
@@ -674,7 +686,7 @@ function rdf_preprocess_comment(&$variables) {
     // Adds RDFa markup to the subject of the comment. Because the RDFa markup is

just spotting that this line is > 80

+++ modules/rdf/rdf.module
@@ -816,7 +833,7 @@ function theme_rdf_template_variable_wrapper($variables) {
+ * We can express it in RDFa via empty <span> tags. These aren't visible and give

> 80

+++ modules/rdf/rdf.module
@@ -832,14 +849,16 @@ function theme_rdf_template_variable_wrapper($variables) {
+ ¶
\ No newline at end of file

there should be a empty newline at the end of the file. removing the space in the line above should fix this.

Powered by Dreditor.

Log in or register to post comments

Comment #13

psynaptic CreditAttribution: psynaptic commented 17 September 2010 at 16:21

Assigned:	Unassigned	» psynaptic
Status:	Needs work	» Needs review

File	Size
815866_rdf_improve_docs_5.patch	29.5 KB

I have no idea how that 2 got in!

I have re-rolled the patch. Happy to make any more changes if anyone spots anything.

Log in or register to post comments

Comment #14

scor CreditAttribution: scor commented 17 September 2010 at 19:39

Status:

Needs review

» Reviewed & tested by the community

That's good to go, thanks @psynaptic!

Log in or register to post comments

Comment #15

sun

German

Karlsruhe

CreditAttribution: sun commented 29 September 2010 at 00:50

#13: 815866_rdf_improve_docs_5.patch queued for re-testing.

Log in or register to post comments

Comment #16

sun

German

Karlsruhe

CreditAttribution: sun commented 29 September 2010 at 00:50

Lovely to see more docs on RDF! Awesome!

Log in or register to post comments

Comment #17

sun

German

Karlsruhe

CreditAttribution: sun commented 30 September 2010 at 01:58

Version:

7.x-dev

» 8.x-dev

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

Log in or register to post comments

Comment #18

scor CreditAttribution: scor commented 30 September 2010 at 02:27

ok, I see there was a tsunami of D7 to D8 push, but without any rationale behind it (from what I can read in the comment above). I went on IRC but too late, I was told I missed the fireworks! I don't see how such a documentation-only patch can affect the long waited D7 beta release! Can someone explain please? or post a link to some kind of announcement, or IRC logs? (ok, there is one string change in there, but if that really bothers anyone, we can remove it).