Posted by scor on June 16, 2009 at 12:20pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | RDF |
Issue Summary
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.
To try this patch and see some RDFa output, make sure to (1) patch and enable the RDF module in core and (2) switch to the Stark theme.
This patch extract all the Field instance mappings defined in the node_type table via hook_rdf_mapping(). It serializes the RDF model in RDFa via template_preprocess_field() and in field.tpl.php.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| field.module01.patch | 4.49 KB | Idle | Failed: Failed to apply patch. | View details |
Comments
#1
As far as Field API is concerned, this looks fine to me.
#2
reroll
#3
DamZ suggested to rename the $rdfa variable to $rdfa_attributes.
#4
Minor nit: field.module and field_sql_storage.module are required and so should not be in this list:
<?php+??? parent::setUp('field_sql_storage', 'field', 'field_test', 'rdf');
?>
I don't know enough about RDF in Drupal to see how this patch is even useful, it hardly seems like more than a no-op, but offhand I don't see how it is harmful either.
#5
patch reroll and remove 'field_sql_storage' and 'field' as per bjaspan's comment.
#6
Looks good. The doc in field.tpl.php looks like it should reference rdfa_attributes instead of instance?
#7
+??????? <div class="field-item <?php print $delta % 2 ? 'odd' : 'even'; ?>" <?php print $rdfa_attributes ?>><?php print render($item); ?></div>Since $item is passed through render(), adn there's no surrounding markup otherwise, it seems like we should just be able to put the $rdfa_attributes stuff into $item itself - that'd save cluttering the templates, and make things less prone to breakage if themes do custom field templates and leave that variable out. Not sure where the best spot in the process to do that is though - but to me it's a similar issue to http://drupal.org/node/493030#comment-1991422
#8
+1 on embedding that in the $items render struct. Possibly put drupal_rdfa_attributes($instance['rdf_mapping']) in a #prefix on the field render array using field_attach_view_alter() ?
Will each field values (as in multivalued fields) need its own rdf information, or is the rdf info relevant for the values as a whole ?
#9
reroll for the version 10 of the RDF patches.
#10
Added attributes to tpl variables.
#11
+++ modules/field/field.info.inc 2009-08-31 19:13:42 +0000@@ -206,7 +206,7 @@ function _field_info_collate_fields($res
-??? $info['fields'] = array();
+??? $info['field'] = array();
That fix just got committed in a separate patch, so this should be removed or the patch won't apply
+++ modules/field/field.module 2009-09-05 11:37:36 +0000@@ -669,9 +689,21 @@ function template_preprocess_field(&$var
+function template_process_field(&$variables) {
+? unset($variables['#attributes']['class']);
+? $variables['attributes'] = empty($variables['#attributes']) ? '' : ' ' . drupal_attributes($variables['#attributes']);
+}
+
+
+
/**
several blank lines after the functions
Beer-o-mania starts in -4 days! Don't drink and patch.
#12
Rerolled patch.
#14
My 1st remark in #11 still applies.
#15
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!
#16
re #7 and #8, we now use the new attributes in the field tpl file.
remarks in #11 fixed.
fix the Field support in the RDF module (multi value support)
this is how the RDF mappings are first retrieved and then added to the field.tpl.php attributes by the rdf module:
<?php
function rdf_preprocess_field(&$variables) {
? $instance = $variables['instance'];
? $mapping = rdf_get_mapping($instance['bundle']);
? $field_name = $instance['field_name'];
? if (!empty($mapping) && !empty($mapping[$field_name])) {
??? foreach ($variables['items'] as $delta => $item) {
????? $variables['item_attributes_array'][$delta] = drupal_rdfa_attributes($mapping[$field_name], $item['#item']['value']);
??? }
? }
}
?>
the above works, but for consistency, I would like the rdf mappings to be already attached to the instance object prior to the preprocess level so that they are present in $variables (the same way we have the mappings attached to nodes, users, etc. during their loading). However I didn't find a way to do it. I looked into the following hooks without any sucess:
- hook_field_read_instance (does allow to alter the instance to add the rdf mapping to it)
- hook_field_attach_load
- hook_field_attach_view_alter
any idea how to achieve that? any hook similar to hook_node_load() for instances?
#17
@scor: I'm not sure I get what your problem is.
Your test code does
+??? $this->field_instance_b = array(+????? 'field_name' => $this->field_b['field_name'],
+????? 'bundle' => 'test_bundle',
+????? 'rdf_mapping' => array(
+??????? 'property' => array(
+????????? 'dc:something',
+????????? 'dc:somethingelse',
+??????? ),
+????? ),
+??? );
+??? field_create_instance($this->field_instance_b);
With the way field_create_instance() and field_update_instance() currently work, the 'rdf_mapping' property should be saved in the $instance definition and will be present when it is read back. Isn't that what you want ?
#18
@yched, yes this is what I want, but I would like to let other modules modify these mappings if possible (or add them if they are not present in the initial instance definition). It's not a big deal if this is not possible with the current implementation of the field API, but just wondering.
#19
This looks good to me. Instead of using 'dc:something' and 'dc:somethingelse', it might be nicer to use 'dc:foo' and 'dc:bar'. Should be RTBC after a reroll.
#20
@scor #18:
"Modify" probably makes sense. "Add if not present" sounds more problematic: in some cases the mappings would be present in the stored $instance definition, in some cases they would be altered-in later ? sounds like a lack of consistency to me.
At any rate, this raises the question of an _alter hook on $field and $instance definitions, which we've been avoiding so far - because, to be honest, I'm not sure of the actual use cases, nor what's the exact extent of potentiel weirdness this introduces.
Besides, with the current behavior (field_[create|update]_instance() saves anything that's in the $instance array, including 'non canonical Field API properties'), doing a field_update_instance() on an $instance where _alter has been run results in the alterations being saved and becoming 'official' parts of the $instance next time it is read up from the db. Not our usual _alter pattern, doesn't look too stable to me.
#21
rerolling with Dries' comments.
#22
uhm, how is it possible this test is working when rdf_get_mapping() doesn't exist yet?!? Am I missing something!?
@altering: The proposed hook_rdf_mapping() already has a alter, so why bother with another possibility to alter? I think field_rdf_mapping() is just nice for modules providing field instances per default - else modules should stay with hook_rdf_mapping().
@yched: Yep that pattern looks dangerous to me too. That way the changes would be still in place when the origin module is gone.
#23
@yched and @fago: agreed, I'm happy with the current features of Field API/RDF API
the tests in this patch are not triggered because the rdf module is not present. It would make more sense to wait for the main RDF patch #493030: RDF #1: core RDF module to be committed before committing this one.
#24
The last submitted patch failed testing.
#25
this was integrated in the main RDF patch #493030: RDF #1: core RDF module.