Problem/Motivation

Currently, we attach an RDF mapping nested array to each entity. There are a number of issues with the current API.

  1. The mapping is attached on entity load even if it isn't used in the rest of the rendering process, resulting in a waste of processing time.
  2. The nested array structure can be hard to read.
  3. Mappings can be defined for non-fields, which means that there is a mapping for data which isn't part of our internal model.

Proposed resolution

Store each RDF mapping as a config entity (RdfMapping). This would be modelled on EntityDisplay in its API.

<?php
$article_mapping
rdf_get_mapping('node', 'article');
$article_mapping->setFieldMapping('field_start_date', array(
 
'properties' => array('schema:startDate'),
 
'datatype callback' => 'date_iso8601',
);
?>

Fixes problem 1 by only accessing the RDF mappings when needed. Therefore, if a site uses JSON-LD but not RDFa, the RDF mappings would only be loaded on application/ld+json requests, not on text/html requests.

Fixes problem 2 by using the RdfMapping config entity. These can be documented clearly so that it is easier for developers to understand the configuration options.

Makes it possible to fix problem 3 by using config entity system. We can validate that a mapping corresponds to a field in the config entity validation. This is not addressed in this patch.

Remaining tasks

Review.

API changes

Removes rdf_mapping attribute from entity
In D7, the rdf_mapping attribute is attached to entities using hook_entity_load. It is then used in functions such as hook_preprocess_field to assemble the correct attributes array. Instead, client code should now use $mapping_manager->getBundleMapping($entity_type, $bundle)->get(); or $mapping_manager->getFieldMapping($entity_type, $bundle, $field_name)->get();
Removes rdf_mapping_save
In D7, a nested array contains all bundle and field mappings, and can be saved using rdf_mapping_save. Instead, client code should load the mapping config entity using $mapping_manager->getBundleMappingConfig($entity_type, $bundle)->get(); or $mapping_manager->getFieldMappingConfig($entity_type, $bundle, $field_name)->get();, and then save necessary changes using the config API.
Removes hook_rdf_mapping
In D7, if a module created an entity type or bundle, it could define a default mapping for that entity and its fields. Instead, this can be handled with config files in the module's config directory.
Removes global entity type mappings (RDF_DEFAULT_BUNDLE)
In D7, a module could define a default mapping for an entity type. For example, all user bundles have the type "sioc:UserAccount". This kind of defaulting can cause problems, #1228872: RDF default mappings override empty values. Instead, we do not handle global entity type mappings in core. Someone can choose to support this in contrib using the MapBundleForOutput and MapFieldForOutput events.
Changes mapping attribute names
In D7, the mapping attributes are rdftypes for bundles, and predicates, datatype, and callback for fields. This changes to types for bundles, and properties, datatype (remains the same), and datatype_callback for fields.

Original report by fago

Since we've got an Entity Field API now writing any data on an entity that is not a properly registered field is highly discouraged. Thus, we should fix rdf module to not do so, but instead leverage the new Entity Field API.

Instead of having $entity->rdf_mapping populated in hook_rdf_load() we could migrate to a computed field which uses whatever API to lazy-load the mappings whenever needed.

In addition to that I wonder whether
- we really need per entity rdf mappings? To me it looks like they are per entity type / bundle only?
- we shouldn't tie the RDF mappings solely to data types of the Typed Data API. So e.g. you configure the default mapping of a field type there. Then, we'd need to allow overrides per data, either have a totally separated API that makes mappings that refer to the typed data structure - or even have built-in support for an rdf-mapping key in typed data definitions?

I must say I like the idea of having a totally separated API that refers to the typed data API data structures more as it keeps things better separated. Thoughts?

Files: 
CommentFileSizeAuthor
#123 rdf-1869600-123.patch6.52 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#118 1869600-118-refactor-rdf.patch148.39 KBjesse.d
PASSED: [[SimpleTest]]: [MySQL] 58,689 pass(es).
[ View ]
#118 interdiff.txt727 bytesjesse.d
#115 1869600-115-refactor-rdf.patch152.08 KBjesse.d
PASSED: [[SimpleTest]]: [MySQL] 58,425 pass(es).
[ View ]
#115 interdiff.txt727 bytesjesse.d
#108 1869600-108-refactor-rdf.patch148.38 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 58,174 pass(es), 6 fail(s), and 1 exception(s).
[ View ]
#108 interdiff.txt32.05 KBlinclark
#101 1869600-101-refactor-rdf.patch147.47 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 57,594 pass(es).
[ View ]
#98 1869600-98-refactor-rdf.patch147.42 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 57,580 pass(es).
[ View ]
#98 interdiff.txt937 byteslinclark
#91 1869600-91-refactor-rdf.patch148.17 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 57,322 pass(es).
[ View ]
#91 interdiff.txt21.47 KBlinclark
#88 1869600-88-refactor-rdf.patch146.96 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 55,955 pass(es).
[ View ]
#86 1869600-86-refactor-rdf.patch146.96 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1869600-86-refactor-rdf.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#86 interdiff.txt1.66 KBlinclark
#84 1869600-84-refactor-rdf.patch146.96 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 56,630 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#84 interdiff.txt9.11 KBlinclark
#83 1869600-83-refactor-rdf.patch138.66 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 55,401 pass(es).
[ View ]
#83 interdiff.txt19.34 KBlinclark
#82 1869600-82-refactor-rdf.patch134.89 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 55,045 pass(es).
[ View ]
#82 interdiff.txt2.37 KBlinclark
#80 1869600-80-refactor-rdf.patch134.56 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 55,350 pass(es), 1 fail(s), and 11 exception(s).
[ View ]
#78 1869600-78-refactor-rdf.patch135.59 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 55,449 pass(es), 2 fail(s), and 14 exception(s).
[ View ]
#78 interdiff.txt4.17 KBlinclark
#76 1869600-76-refactor-rdf.patch131.42 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#76 interdiff.txt3.88 KBlinclark
#74 1869600-74-refactor-rdf.patch130.68 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#70 1869600-70-refactor-rdf.patch130.68 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#70 interdiff.txt123.04 KBlinclark
#66 1869600-66-refactor-rdf.patch153.04 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 56,688 pass(es), 1 fail(s), and 6 exception(s).
[ View ]
#64 1869600-64-refactor-rdf.patch126.86 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#64 interdiff.txt51.17 KBlinclark
#58 1869600-58-refactor-rdf.patch128.53 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 56,754 pass(es), 101 fail(s), and 410 exception(s).
[ View ]
#58 interdiff.txt2.04 KBlinclark
#57 1869600-57-refactor-rdf-do-not-test.patch127.96 KBlinclark
#53 1869600-53-refactor-rdf.patch148.7 KBscor
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1869600-53-refactor-rdf.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#48 1869600-48-refactor-rdf.patch151.1 KBscor
PASSED: [[SimpleTest]]: [MySQL] 49,942 pass(es).
[ View ]
#48 interdiff.txt45.41 KBscor
#45 1869600-45-refactor-rdf.patch118.55 KBscor
FAILED: [[SimpleTest]]: [MySQL] 48,982 pass(es), 39 fail(s), and 41 exception(s).
[ View ]
#45 interdiff.txt639 bytesscor
#41 1869600-41-refactor-rdf.patch113.7 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 50,007 pass(es), 37 fail(s), and 180 exception(s).
[ View ]
#41 interdiff.txt9.43 KBlinclark
#40 1869600-39-rdf-refactor.patch116.89 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#39 1869600-39-rdf-refactor.patch0 byteslinclark
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#34 1869600-33-refactor-rdf.patch117.03 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 49,254 pass(es), 3 fail(s), and 125 exception(s).
[ View ]
#33 1869600-33-refactor-rdf.patch0 byteslinclark
PASSED: [[SimpleTest]]: [MySQL] 49,712 pass(es).
[ View ]
#31 1869600-31-refactor-rdf.patch104.25 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 49,215 pass(es), 4 fail(s), and 6 exception(s).
[ View ]
#29 1869600-29-refactor-rdf.patch100.98 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 49,313 pass(es), 4 fail(s), and 28 exception(s).
[ View ]
#27 1869600-27-refactor-rdf.patch96.28 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 49,312 pass(es), 11 fail(s), and 30 exception(s).
[ View ]
#24 1869600-24-refactor-rdf.patch96.28 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#22 1869600-22-refactor-rdf.patch96.04 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1869600-22-refactor-rdf.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 1869600-20-refactor-rdf.patch96.04 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#18 1869600-18-refactor-rdf.patch77.09 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 49,536 pass(es), 7 fail(s), and 828 exception(s).
[ View ]
#14 1869600-11-refactor-rdf.patch61.45 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 49,475 pass(es), 7 fail(s), and 828 exception(s).
[ View ]
#14 interdiff.txt28.43 KBlinclark
#13 1869600-11-refactor-rdf.patch61.81 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 34,454 pass(es), 11,557 fail(s), and 8,040 exception(s).
[ View ]
#13 interdiff.txt28.8 KBlinclark
#11 1869600-11-refactor-rdf.patch228.49 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#11 interdiff.txt195.48 KBlinclark
#10 1869600-10-refactor-rdf.patch54.41 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 49,489 pass(es), 7 fail(s), and 828 exception(s).
[ View ]
#10 interdiff.txt4.52 KBlinclark
#9 1869600-09-refactor-rdf.patch52.95 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#9 interdiff.txt13.11 KBlinclark
#7 1869600-7-refactor-rdf__for-testing.patch98.73 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 49,486 pass(es), 66 fail(s), and 446 exception(s).
[ View ]
#7 1869600-7-refactor-rdf__for-review.txt46.12 KBlinclark

Comments

I agree we should no longer carry around the RDF mapping inside each entity. That's the approach I took in Entity RDF where the RDF mappings are stored in the Entity property info and available with the rest of the entity property info.

- we really need per entity rdf mappings? To me it looks like they are per entity type / bundle only?

In the majority of cases, this is true. But there are use cases for allowing RDF mapping alteration on a per entity bases, this use case was requested in contrib where a node of type event would get mapped to http://schema.org/Event but additionally it could also have a more specific sub-type of event such as http://schema.org/SportsEvent depending on the choice of the user creating the node. This could be for example controlled by a taxonomy vocabulary listing all of the possible sub-type of events, each term being mapped to the schema.org type. This use case is definitely not for core, but should be possible for contrib to alter the RDF mapping before they are used to serialize the entity.

Issue tags:-typed data

I must say I like the idea of having a totally separated API that refers to the typed data API data structures more as it keeps things better separated. Thoughts?

This actually closely matches the concept of the RdfMappingManager, which I added in #1852812: Use cache to get entity type/bundle metadata from JSON-LD @type URI. This manager is a service added to the container.

This means that instead of having the mapping be part of the entity info at all, we would simply get the manager from the service and request the mapping by its typed data IDs.

The mapping functionality for data publishing hasn't been posted yet, I've been working on it over the past two days.

Issue tags:+typed data

No idea why that tag was removed.

But there are use cases for allowing RDF mapping alteration on a per entity bases... This use case is definitely not for core, but should be possible for contrib to alter the RDF mapping before they are used to serialize the entity.

The way I have it working now, this is possible. The RdfMappingManager uses MappingEvents. It dispatches events in 4 circumstances:

  • Mapping a site schema class to RDF types
  • Mapping a site schema property to RDF predicates
  • Mapping an array of incoming RDF types to a site schema type URI (from which the Typed Data IDs can be derived)
  • Mapping an array of incoming RDF predicates to a site schema property URI (ditto, but not implemented yet)

The site schema classes and properties are aware of the entity_type/bundle that they correspond to. I could add getters which expose this to modules.

For the use case you outline, a contrib module would subscribe to the MAP_BUNDLE_FOR_OUTPUT event. It would grab the site schema class from the event and check the entity type of the bundle. If it is the specified entity type, it could add its mapping to the ones that were loaded from config.

I'll be posting the patch that does this soon.

ad #2: Great to see you are working on that!

This means that instead of having the mapping be part of the entity info at all, we would simply get the manager from the service and request the mapping by its typed data IDs.

Sounds good.With #1869250: Various EntityNG and TypedData API improvements we can "identify" a data property by the root item's type + the property path, e.g. comment.comment_body.0.format - maybe that would be a good base for allowing per-entity-type mappings?

Apart from that we could do per type (-> field type) mapping defaults, which then could be overridden.

Apart from that we could do per type (-> field type) mapping defaults, which then could be overridden.

Ooooo, I like where this is going...

Status:Active» Needs review
StatusFileSize
new46.12 KB
new98.73 KB
FAILED: [[SimpleTest]]: [MySQL] 49,486 pass(es), 66 fail(s), and 446 exception(s).
[ View ]

The attached patch is still rough, but it demonstrates the concepts. It builds on the patch in #1852812: Use cache to get entity type/bundle metadata from JSON-LD @type URI, which will probably be committed tomorrow.

Site Schema
A site schema is the site-generated vocabulary. Mappings are between site schema terms and their external vocabulary equivalent. For example, site-syn:node/article/body_value would map to schema:articleBody. There are two site schemas per site, as required for #1797210: Decide how to negotiate between the 2 JSON-LD serializations. The SiteSchema functionality still needs to be fully implemented to produce different vocabulary structures, so the current SiteSchema implementation is bit of a placeholder.
RDF Mapping Manager
The RDF mapping manager figures out which site schema term it is mapping from the Typed Data IDs and site schema id which are passed in. It then dispatches an event, which allows any module to add mappings. In its own event subscriber, the RDF module adds the site schema URI (which we should make optional) and the saved mappings defined in the config file. This isn't currently cached, but it could be.
RDF mapping config entity
This would store the RDF mappings that were saved in .install files, etc. There is an entity for bundles and one for fields because we want to collect different data about each (types for bundles / properties, datatype, etc for fields). In addition, we could potentially add flags to the field config entity to handle #1813328: Enable literal handling in common RDF-ish domain models.

This patch also:

Gets rid of "default mappings" for entity types
It removes the default mapping from node module, and adds the mappings in standard.install. As we've discussed above, this isn't really required for most use cases. It causes undesirable effects for some use cases, as described in #1228872: RDF default mappings override empty values. We can definitely make it possible for contrib to use defaults as described above.
Removes the rdf_mapping attribute from the entity
By using the RDF mapping manager, we no longer have to attach the mapping to the entity itself. However, I've only changed rdf_preprocess_node to account for this.

This will fail pretty much all of the RDFa tests. I think we should refactor those considerably, and work on that is already underway in other issues.

Status:Needs review» Needs work

The last submitted patch, 1869600-7-refactor-rdf__for-testing.patch, failed testing.

StatusFileSize
new13.11 KB
new52.95 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

The patch that introduced the SiteSchema and RdfMappingManager (which was included in the __for-testing patch above) were committed to core today.

Changes since #7:

Removes rdf_mapping_save
All CRUD will happen through the RdfMappingManager. The functions for saving are RdfMappingManager::saveBundleMappingConfig and RdfMappingManager::saveFieldMappingConfig. I added "Config" at the end because I think it is important to differentiate between the mapping that is in the config file and the mapping that is returned after the event is dispatched. Alternately, I would be ok with changing the getter to something like RdfMappingManager::getPreparedBundleMapping, in which case we could drop the Config from the end.
Begins the removal of hook_rdf_mapping
D7 moved away from hook_node_info towards creating each bundle, field, and field instance individually in install files. The hook_rdf_mapping style of hook should be phased out. Instead, modules that want to define a mapping for a specific bundle or fields should create those individually. I didn't remove the implementations of the hook yet because they will have to be converted.
Updates the CrudTest
The CRUD test is updated to reflect the new RdfMappingManager functions.
Changes array keys used in mappings
I propose that we use 'types' instead of 'rdftypes', 'properties' instead of 'predicates', and 'datatype_callback' instead of 'callback'.

StatusFileSize
new4.52 KB
new54.41 KB
FAILED: [[SimpleTest]]: [MySQL] 49,489 pass(es), 7 fail(s), and 828 exception(s).
[ View ]

I forgot that there was a call to rdf_mapping_save() in the standard.install file. This patch switches to using the save functions of the RdfMappingManager.

Status:Needs work» Needs review
StatusFileSize
new195.48 KB
new228.49 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

I realized that the patch in #9 was basically wrapping the config API. Instead, I think we should just add getConfigMapping functions. Then, client code can just use the config API on the config entity that they get back.

Status:Needs review» Needs work

The last submitted patch, 1869600-11-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new28.8 KB
new61.81 KB
FAILED: [[SimpleTest]]: [MySQL] 34,454 pass(es), 11,557 fail(s), and 8,040 exception(s).
[ View ]

Whoops, forgot that I had deleted edit module because it was causing an exception in simpletest.

StatusFileSize
new28.43 KB
new61.45 KB
FAILED: [[SimpleTest]]: [MySQL] 49,475 pass(es), 7 fail(s), and 828 exception(s).
[ View ]

Gah, I'm starting to see why people don't patch on Saturday nights. Forgot to remove a debugging line from index.php

Status:Needs review» Needs work

The last submitted patch, 1869600-11-refactor-rdf.patch, failed testing.

Thanks for the updates. I started having a look at the described architecture, while I had no close look at the code yet.

I must say that the suggested architecture looks very sound to me :-) Some random thoughts:
- Why do we use events vs hooks? Any special reasons? (just wondering)
- How does it handle bundles - are mappings generally per bundle? What if the entity has no bundles but e.g. introduces bundles later on? Should we implement fallback behaviour e.g. have a generic per entity-type mapping and allow per-bundle mapping to override it? Or even support a generic fallback routine, e.g. have entity_type:bundle:whatever:id as key and fallback to the least specific mapping?

Thanks for reviewing the architecture, glad is seems sound :)

Why do we use events vs hooks? Any special reasons?

This decision came out of one of the REST G+ hangouts. Using an event allows a module to add multiple subscribers (IIRC) and allows a subscriber to stop propagation of an event. It also should allow a site developer to easily reorder and remove subscribers, which you can do with hook_module_implements_alter, but using that a lot seems like a dirty hack.

How does it handle bundles - are mappings generally per bundle? What if the entity has no bundles but e.g. introduces bundles later on?

My understanding is that if an entity type does not have a bundle, then it will be assigned a bundle that uses the same name as its entity type. The mapping would then correspond to this entity type/bundle. Though I'm sure you can tell me if my understanding is wrong ;)

Regarding fallbacks, I believe we want to avoid implicit behavior as much as possible. One of the lessons learned in the Drupal 7 implementation of RDF was that "helping" users in this way causes problems, #1228872: RDF default mappings override empty values.

Status:Needs work» Needs review
StatusFileSize
new77.09 KB
FAILED: [[SimpleTest]]: [MySQL] 49,536 pass(es), 7 fail(s), and 828 exception(s).
[ View ]

This patch just moves all of the mapping classes to their own directory.

Status:Needs review» Needs work

The last submitted patch, 1869600-18-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new96.04 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

This patch adds tests for the new mapping events and switches the user mapping to the new system, which should get rid of some of the exceptions.

Status:Needs review» Needs work

The last submitted patch, 1869600-20-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new96.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1869600-22-refactor-rdf.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Small copy paste error was breaking install profile.

Status:Needs review» Needs work

The last submitted patch, 1869600-22-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new96.28 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Had to merge in changes in HEAD.

Status:Needs review» Needs work

The last submitted patch, 1869600-24-refactor-rdf.patch, failed testing.

I'm fairly certain the new error is because of #1701014: Validate config object names. I'm guessing that using colons in the name is not permitted.

Status:Needs work» Needs review
StatusFileSize
new96.28 KB
FAILED: [[SimpleTest]]: [MySQL] 49,312 pass(es), 11 fail(s), and 30 exception(s).
[ View ]

Fixed config issue by using a semicolon instead of a colon.

Status:Needs review» Needs work

The last submitted patch, 1869600-27-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new100.98 KB
FAILED: [[SimpleTest]]: [MySQL] 49,313 pass(es), 4 fail(s), and 28 exception(s).
[ View ]

This patch fixes the CRUD test, removes the MappingHookTest, and fixes the two namespace tests.

Status:Needs review» Needs work

The last submitted patch, 1869600-29-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new104.25 KB
FAILED: [[SimpleTest]]: [MySQL] 49,215 pass(es), 4 fail(s), and 6 exception(s).
[ View ]

This fixes the taxonomy term mapping to use the new system. It removes the mappings that don't actually get placed in HTML.

Status:Needs review» Needs work

The last submitted patch, 1869600-31-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,712 pass(es).
[ View ]

This patch adds the patch from #1867096: Rewrite RdfaMarkupTest to parse RDFa. It removes the rdf_rdfa_attributes test... it doesn't make sense to test that with the standard profile. It could be added back in as its own test, but I'd rather wait. The functionality is already implicitly tested, the test itself is currently rather strangely coded, and we will be removing most of the function anyway.

StatusFileSize
new117.03 KB
FAILED: [[SimpleTest]]: [MySQL] 49,254 pass(es), 3 fail(s), and 125 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1869600-33-refactor-rdf.patch, failed testing.

+++ b/core/modules/rdf/lib/Drupal/rdf/Mapping/MapBundleForOutputEvent.php
@@ -0,0 +1,63 @@
+  /**
+   * @return array
+   *   An array of CURIE strings/arrays.
+   */
+  public function getTypes() {
+    return $this->types;
+  }
+}

My understanding from the rest of the code of that class is that while addTypes() can accept a CURIE as string which would get converted into a single element array, the return value of getTypes() is always an array. What are you referring to with "strings/arrays"?

+++ b/core/modules/rdf/lib/Drupal/rdf/Mapping/MapFieldForOutputEvent.php
@@ -0,0 +1,71 @@
+  /**
+   * An array of CURIEs to use for the RDF types.
+   *
+   * @var array
+   */
+  protected $predicates;
+
+  protected $datatype;
+
+  protected $datatype_callback;

the documentation doesn't match or is missing.

+++ b/core/modules/rdf/lib/Drupal/rdf/Mapping/MapFieldForOutputEvent.php
@@ -0,0 +1,71 @@
+  protected $datatype;
+
+  protected $datatype_callback;

I believe these are the same per your comment in #9, i.e. there should only be datatype_callback?

I propose that we use 'types' instead of 'rdftypes', 'properties' instead of 'predicates', and 'datatype_callback' instead of 'callback'.

I don't have a strong preference either way, so I'm happy to see that change. The reason I used 'rdftypes' and 'predicates' in D7 was to avoid confusion with 'types' and 'properties' which are generic terms and used everywhere in Drupal to mean different things than what they mean in RDF. That said, I admit 'predicates' can be confusing too, so let's go with 'types' and 'properties' and 'datatype_callback'.

What are you referring to with "strings/arrays"?

Sorry, I was getting ahead of myself. #1778410: Throw exception when RDF namespaces collide introduces the idea of using a "CURIE array" to manage the prefix indirection (see #15 in that issue).

I believe these are the same per your comment in #9, i.e. there should only be datatype_callback?

I didn't suggest removing the 'datatype', I just didn't mention it because I wasn't proposing a change to the attribute name. Datatype is the datatype's URI, the datatype callback is a callable which transforms the data to a particular format.

Issue summary:View changes

Updated the summary.

Issue summary:View changes

Clarify proposal.

Issue summary:View changes

Add remaining tasks.

Issue summary:View changes

Fix HTML code tag issue.

My understanding is that if an entity type does not have a bundle, then it will be assigned a bundle that uses the same name as its entity type. The mapping would then correspond to this entity type/bundle. Though I'm sure you can tell me if my understanding is wrong ;)

Yep, that's the current state. It's a field API relict though and I'd love to change this at some point - such that bundles are just optional. This would match more with the thinking of #1380720: Rename entity "bundle" to "subtype".

Regarding fallbacks, I believe we want to avoid implicit behavior as much as possible. One of the lessons learned in the Drupal 7 implementation of RDF was that "helping" users in this way causes problems, #1228872: RDF default mappings override empty values.

Agreed. With sub-types in mind I was just thinking of implementing "inheritance" - e.g. you define it on the entity type leven and the bundle inherits it as long as it does not override it.

Status:Needs work» Needs review
StatusFileSize
new0 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

This is just a reroll.

StatusFileSize
new116.89 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Rather, this is just a reroll

StatusFileSize
new9.43 KB
new113.7 KB
FAILED: [[SimpleTest]]: [MySQL] 50,007 pass(es), 37 fail(s), and 180 exception(s).
[ View ]

This patch cleans up the handling of the config entity for mappings.

Status:Needs review» Needs work
Issue tags:-Entity Field API, -typed data

The last submitted patch, 1869600-41-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review

#41: 1869600-41-refactor-rdf.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Entity Field API, +typed data

The last submitted patch, 1869600-41-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new639 bytes
new118.55 KB
FAILED: [[SimpleTest]]: [MySQL] 48,982 pass(es), 39 fail(s), and 41 exception(s).
[ View ]

Rerolling after #1801304: Add Entity reference field went in.

I've also made a small fix to remove the numerous "Undefined index: properties" notices seen in the tests results.

What about making this patch a bit easier + shorter to review if we left the renaming of Drupal\rdf\RdfMappingException to Drupal\rdf\Mapping\RdfMappingException in a separate follow up issue?

In reviewing/testing this patch, I'm realizing how easier it will be when the RDF tests patches have been committed: #1869914: Refactor RdfMappingDefinitionTestCase and split it in dedicated test cases and #1895064: Ensure user name is language neutral in RDFa and update tests to parse RDFa :)

Status:Needs review» Needs work

The last submitted patch, 1869600-45-refactor-rdf.patch, failed testing.

I don't think that moving the RdfMappingException is a major contributor to the size of this patch. The technique I use for reviewing large patches is applying the patch and then using a GUI to look at the changes.

Status:Needs work» Needs review
StatusFileSize
new45.41 KB
new151.1 KB
PASSED: [[SimpleTest]]: [MySQL] 49,942 pass(es).
[ View ]

Like I said in the call last week, I'm comfortable with the approach suggested by Lin here, where RDF mappings are stored as config entities and can be accessed on demand via the RDF mapping manager service. Module can subscribe to event if they need to alter the mappings, that's great. From an RDF perspetive, this addresses a lot of the pain points of the D7 architecture, so I'd like to move forward with this. To this effect I've been continuing the integration of the new RDF mapping manager with the rest of Drupal, the only change I suggest to make to the RDF mappings (included in this patch) is to add 'mapping_type' for covering the case where we have 'rel' mappings, that's necessary for taxonomy, image and content author relations. The rest of my contributions to this patch are to align rdf.module with the new RDF mapping API so that all tests can pass.

Here is a changelog of the interdiff in no particular order:
- removed rdf_mapping_load() and rdf_test_rdf_mapping()
- removed hook_rdf_mapping() implementations: comment_rdf_mapping(), taxonomy_rdf_mapping()
- introduced 'mapping_type' in RDF mapping manager to reflect the old 'type' (e.g. rel). this is necessary for some fields such as taxonomy or file/image
- added comment and user RDF mappings to rdf_test.install and added it as dependency of comment tests
- converted tracker module and tests to new RDF mapping manager
- remove testAttributesInMarkup3(): default mappings are no longer supported
- update rdf_test_install() and testAttributesInMarkup2() to use RDF mapping manager
- update XPath expressions to account for site-syn terms (these will go away once all tests are done using Easyrdf)
- rename all 'rdftype' to 'types'
- rename predicates to properties and callback to datatype_callback in rdf.module
- re-introduce rdf_comment_load() which is required for comment RDFa output
- add comment mappings to standard.install
- various documentation updates (overall documentation not complete yet)

Thanks, that's great! I don't have a chance to fully review ATM, but wanted to comment about using 'rel'...

Didn't we plan to only use RDFa Lite? I think we should really make that a priority. Do you see the 'mapping_type' as a stopgap?

Just to record it here, scor said in the G+ hangout that he agrees that we should move to only using 'property' (RDFa Lite). This will happen in a separate issue, though.

I expect to go through this tonight and add more documentation, plus potentially make a few alterations to the config entities now that I have a little bit more experience with them.

wow, impressive work!

I had overall look at the patch and I'm happy with the approach taken. In particular mapping-event classes miss quite some docs, but I suppose that's not 100% finished yet.

'mapping_type' for covering the case where we have 'rel'

Couldn't that be determined based upon typed data, e.g. if it's an entity reference have 'rel'?

+ $mapping_manager = drupal_container()->get('rdf.mapping_manager');
+ $bundle_mapping = $mapping_manager->getBundleMapping('taxonomy_term', $term->vid);
+ $name_field_mapping = $mapping_manager->getFieldMapping('taxonomy_term', $term->vid, 'name');

hm, instead of specifically requesting a mapping for the bundle, couldn't we just say "get me the mapping for the given entity object?" We could still have config-entities that implement mappings per bundle, but the general API could be general. So we'd have a general mapping event and its up to the subscriber to decide on how to group the mappings. Maybe by entity type, by bundle or even maybe by entity-id? Core would implement it by bundle, but the API would allow doing per-entity mappings if necessary as it was in d7. Thoughts?

With #1868004: Improve the TypedData API usage of EntityNG will get new data types like entity:node:article, entity:node and entity. This might open the opportunity to base this upon typed data API instead of the entity API. Thus, it would enable us to potentially map any typed data structure to RDF using the API. I see that this is aiming for more and might be too much, but at least I want to throw out my thoughts here just in case you think that makes sense.
Fortunately, with d8 mostly everything will be an entity already. Some config remains not, while it's typed data enabled via the config schema - not sure that's a good use case for having it on typed data level though. I'd see it more useful for arbitrary aggregated data, e.g. custom data structures that re-mix entity fields for listing purposes only (like views?)

Couldn't that be determined based upon typed data, e.g. if it's an entity reference have 'rel'?

yes, absolutely! I left it "as is" here so as to keep this issue focused and limit its scope, as this patch is already getting quite large, but it will need follow up. I agree with you. For example in D7/ RDFx we do something similar by inspecting the type of the property (generateEntityDrupalWrapper, EntityValueWrapper, EntityListWrapper or EntityStructureWrapper) and generate the RDF from there.

StatusFileSize
new148.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1869600-53-refactor-rdf.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

rerolled patch after #1869914: Refactor RdfMappingDefinitionTestCase and split it in dedicated test cases and #1895064: Ensure user name is language neutral in RDFa and update tests to parse RDFa have been committed. Given the major changes in the tests, I ended up apply the old patch #48 manually instead of rebasing, which means I have no interdiff to provide. Note that I removed MappingDefinitionTest.php since the new mapping config manager functionality is tested elsewhere. All RDF tests are passing on my machine.

Next step is to improve documentation, unless you had some refactoring you wanted to do, Lin?

Status:Needs review» Needs work
Issue tags:-Entity Field API, -typed data

The last submitted patch, 1869600-53-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review

#53: 1869600-53-refactor-rdf.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Entity Field API, +typed data

The last submitted patch, 1869600-53-refactor-rdf.patch, failed testing.

StatusFileSize
new127.96 KB

This is just a reroll. Installation will fail because of changes to the config system, so making it do-not-test for now. Will be posting a revised version that will install shortly.

Status:Needs work» Needs review
StatusFileSize
new2.04 KB
new128.53 KB
FAILED: [[SimpleTest]]: [MySQL] 56,754 pass(es), 101 fail(s), and 410 exception(s).
[ View ]

OK, here is a patch that installs. I haven't made any substantial changes from #53 yet.

I've created a sandbox to track progress on this, https://drupal.org/project/2011168. I'll be pushing up to that after I get each test to pass. Once I have all the tests passing, I'll post a patch.

thanks Lin. The link in #59 doesn't work, this is probably the right link: https://drupal.org/sandbox/linclark/2011168

Status:Needs review» Needs work

The last submitted patch, 1869600-58-refactor-rdf.patch, failed testing.

So now that we aren't targeting the Deploy and CreateJS use cases specifically, I'm not sure whether we should support multiple site schemas.

If we do, I think we should consider making some changes:

  • For syndication site schema (the one that will map to Schema.org, etc) do not output a site generated URI. Just output the external vocabulary terms
  • Change the term SiteSchema to something like DomainModel. Not 100% happy with that, even, but I think it makes more sense than SiteSchema if we aren't creating a vocab for syndication.
  • Change the identifiers from "syndication" and "content deployment" to "external" and "internal".

I don't see the need to keep both schema types any more, let's just support the syndication schema that allows mapping each field and its compound elements to an external set of terms (schema.org, or other vocabularies). If we do that, do we even need to come up with a name for it, since there would be only one schema? I think this would simplify things.

Status:Needs work» Needs review
StatusFileSize
new51.17 KB
new126.86 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Posting because I've made some large changes to the tests. The current RdfaMarkupTest uses standard_profile when it doesn't need to.

The first test in RdfaMarkupTest is just ensuring that rdf_rdfa_attributes works as expected, so that can definitely be a DrupalUnitTestBase. It could even potentially be a PhpUnitTest if we felt confident that the config structure wasn't going to change. This is now RdfaAttributesTest.

The second test was ensuring that file fields, image fields, and taxonomy term fields all exposed data properly in node teasers. I've split these out into separate tests per field type. These separate tests can now extend from base classes like FileFieldTestBase, so we don't have to replicate as much of the field creation and node creation logic. I think it's also a better separation, because we should probably be testing all of these fields' different formatters anyway.

Status:Needs review» Needs work

The last submitted patch, 1869600-64-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new153.04 KB
FAILED: [[SimpleTest]]: [MySQL] 56,688 pass(es), 1 fail(s), and 6 exception(s).
[ View ]

This patch fixes the tests and also fixes a merge fail which had over written some of our changes to files outside of RDF module.

Status:Needs review» Needs work

The last submitted patch, 1869600-66-refactor-rdf.patch, failed testing.

I think it would make sense to switch the mapping config API to be closer to entity display, since the tasks are conceptually close to each other. So the mapping API would look like this:

<?php
$article_mapping
rdf_get_mapping('node', 'article');
$article_mapping->setFieldMapping('field_start_date', array(
 
'properties' => array('schema:startDate'),
 
'datatype callback' => 'date_iso8601',
);
?>

Issue summary:View changes

Add task

+1. This looks better DX than the former:

<?php
$config
= $rdf_mapping_manager->getFieldMappingConfig('node', 'article', 'field_start_date');
$field_mapping = array(...);
$config->setData($field_mapping)->save();
?>

edit: s/UX/DX

Status:Needs work» Needs review
StatusFileSize
new123.04 KB
new130.68 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

This patch removes a lot of the previous work. It takes out the SiteSchema and MappingEvents and simply uses an RdfMapping config object.

There's some clean-up to be done, but setting to NR to see what the test bot says.

Status:Needs review» Needs work
Issue tags:-Entity Field API, -typed data

The last submitted patch, 1869600-70-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review

#70: 1869600-70-refactor-rdf.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Entity Field API, +typed data

The last submitted patch, 1869600-70-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new130.68 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Not sure why the testbot is failing on that, since I can install just fine on my computer. Tried updating my local and rerolling.

Status:Needs review» Needs work

The last submitted patch, 1869600-74-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.88 KB
new131.42 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

This should at least run.

Status:Needs review» Needs work

The last submitted patch, 1869600-76-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.17 KB
new135.59 KB
FAILED: [[SimpleTest]]: [MySQL] 55,449 pass(es), 2 fail(s), and 14 exception(s).
[ View ]

Doh! forgot to add the RdfMapping plugin.

Status:Needs review» Needs work

The last submitted patch, 1869600-78-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new134.56 KB
FAILED: [[SimpleTest]]: [MySQL] 55,350 pass(es), 1 fail(s), and 11 exception(s).
[ View ]

So I get 3 different tests results, depending on whether I run it locally through the UI, locally using the script (which gives me a lot of fails), or on testbots (which only gives one real fail). I'm going to try going back to the version of ImageFileAttributeTest which passed via UI but blew up in the script to see whether the testbot is ok with it.

Status:Needs review» Needs work

The last submitted patch, 1869600-80-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
new134.89 KB
PASSED: [[SimpleTest]]: [MySQL] 55,045 pass(es).
[ View ]

Ok, I think this one should pass all tests. It needs more comments before RTBC, though.

StatusFileSize
new19.34 KB
new138.66 KB
PASSED: [[SimpleTest]]: [MySQL] 55,401 pass(es).
[ View ]

This patch just removes some docs for hook_rdf_mapping and adds code style changes based on Code Sniffer.

StatusFileSize
new9.11 KB
new146.96 KB
FAILED: [[SimpleTest]]: [MySQL] 56,630 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

This patch adds an upgrade path and tests for the upgrade path. It only upgrades stored mappings. This doesn't necessarily reflect all the data exposed on a site, since D7 had the concept of default mappings which were computed at runtime.

I think this patch is ready for some feedback. Here's a summary of what it does:

  1. Adds an RdfMapping config entity, which replaces the separate BundleRdfMapping and FieldRdfMapping config entities
  2. Adds a procedural wrapper (rdf_get_mapping) for getting an RdfMapping object, similarly to the entity_get_display procedural wrapper
  3. Removes hook_rdf_mapping and the rdf_mapping database table and moves the following hook_rdf_mapping implementations to config:
    • comment_rdf_mapping
    • forum_rdf_mapping
    • user_rdf_mapping
  4. Removes rdf_mapping_load(), _rdf_get_default_mapping(), _rdf_mapping_load(), _rdf_mapping_load_multiple(), rdf_mapping_save(), and rdf_mapping_delete(),
  5. No longer attaches mappings to entity when loaded.
  6. Explicitly sets mappings for tests in setUp(), rather than relying on mapping hooks
  7. Splits RdfaMarkupTest into 4 tests: RdfaAttributesTest, FileFieldAttributesTest, ImageFieldAttributesTest, and TaxonomyTermFieldAttributesTest
  8. Removes MappingHookTest and rdf_test module
  9. Removes the relics of the earlier approach:
    • MappingSubscriber
    • MapTypesFromInputEvent
    • RdfConstants
    • RdfMappingEvents
    • RdfMappingException
    • RdfMappingManager
    • SiteSchema, et al
    • MappingDefinitionTest
    • RdfMappingEventTest
    • SiteSchemaTest
    • rdf.services.yml
    • rdf_test_mapping module
  10. Adds upgrade path and tests.

Some Questions/Follow-ups

  1. Should mapping config be stored with the module (like forum is)? In this case, user's mapping should be moved to user module.
  2. Need a follow up to figure out how upgrade for default mappings should be handled.
  3. What's the comment in hook_comment_load() about?

Issue summary:View changes

Updated summary.

Status:Needs review» Needs work

The last submitted patch, 1869600-84-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.66 KB
new146.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1869600-86-refactor-rdf.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The problem was that setting datatype_callback to NULL made it pass the 'isset' test, so rdf_rdfa_attributes was trying to call an empty function. I changed it both so that we don't set NULL values in the mapping and that the check is !empty instead of isset.

Status:Needs review» Needs work

The last submitted patch, 1869600-86-refactor-rdf.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new146.96 KB
PASSED: [[SimpleTest]]: [MySQL] 55,955 pass(es).
[ View ]

Rerolled.

Thanks for pushing this forward, Lin! great progress. I need to review the patch in more details, but in the meantime, here is some quick feedback on the questions from #84. In general I think we should open follow up issues and avoid adding more changes to this patch which is growing large already.

1. Should mapping config be stored with the module (like forum is)? In this case, user's mapping should be moved to user module.

not feeling strongly about this, but my feeling is that it should be stored with the module, but I could be convinced otherwise if a good reason comes up.

3. What's the comment in hook_comment_load() about?

This was for performance reasons, but given the massive changes in the Entity API in core, this is no longer relevant and can be removed.

In looking at the configurables docs more, this should have a ConfigEntityInterface.

StatusFileSize
new21.47 KB
new148.17 KB
PASSED: [[SimpleTest]]: [MySQL] 57,322 pass(es).
[ View ]

This patch adds an RdfMappingInterface, which extends ConfigEntityInterface to include the additional methods.

It also differentiates between the RDF mapping config and the prepared mapping. This will be important for #1778410: Throw exception when RDF namespaces collide.

Status:Needs review» Needs work

The last submitted patch, 1869600-91-refactor-rdf.patch, failed testing.

+++ b/core/modules/rdf/lib/Drupal/rdf/Plugin/Core/Entity/RdfMapping.php
@@ -0,0 +1,179 @@
+  /**
+   * Entity type to be mapped.
+   *
+   * @var string
+   */
+  public $targetEntityType;
+
+  /**
+   * Bundle to be mapped.
+   *
+   * @var string
+   */

Where is the notion of target coming from? Entity type is called $targetEntityType but the bundle is called simply $bundle. Should bundle also be named $targetBundle? (after we know why target is used here).

--- /dev/null
+++ b/core/profiles/standard/config/rdf.mapping.taxonomy_term.tags.yml
@@ -0,0 +1,10 @@
+id: user.user
+targetEntityType: user
+bundle: user
+types:
+  - 'skos:Concept'

id, targetEntityType and bundle for taxonomy_term are inaccurate here (most likely a copy paste oversight). I wonder how this wasn't caught by the tests. It looks like custom mappings are set during test setUp() in mappings are set in TaxonomyAttributesTest.php, maybe that's why tests are passing?

The key targetEntityType comes directly from the EntityDisplay config object, on which this is based. If you use entityType, then you override the config entity's actual entity type (i.e. rdf_mapping).

Regarding the other question, yes, we don't have a test for the standard profile at this point. I'm not sure whether we should hold up this issue on that, since we should change the standard profile to use Schema.org instead of the current mapping.

Status:Needs work» Needs review

#91: 1869600-91-refactor-rdf.patch queued for re-testing.

The key targetEntityType comes directly from the EntityDisplay config object, on which this is based. If you use entityType, then you override the config entity's actual entity type (i.e. rdf_mapping).

ok. Looks like targetEntityType is used pretty often in core in the yml files, so I guess developers will get used to it.

Regarding the other question, yes, we don't have a test for the standard profile at this point. I'm not sure whether we should hold up this issue on that, since we should change the standard profile to use Schema.org instead of the current mapping.

I agree.

Status:Needs review» Needs work

The second remark in #93 needs to be resolved.

Status:Needs work» Needs review
StatusFileSize
new937 bytes
new147.42 KB
PASSED: [[SimpleTest]]: [MySQL] 57,580 pass(es).
[ View ]

This patch will no longer apply, so I rerolled, removed the todo, and fixed the issue from #93.

Status:Needs review» Needs work

I've reviewed this patch once more today and it is ready to go. This is the most important patch for RDF in Drupal 8. It aligns the RDF module with the new Entity API and it also enables RDF to use the new config system to store the RDF mappings. I tested the patch on a fresh D8 site, and was able to change the mappings for node and user in the appropriate YAML files, import them and observe the change in the mappings used in the RDFa markup. I also enabled the forum module which features a custom set of mappings for the content type and the forum taxonomy vocabulary which were all working too.

This patch will need a reroll though, but please set it to RTBC directly once rerolled. Thanks for your work on this Lin.

Besides two minor issues with the documentation it looks good to me.

+  /**
+   * Unique UUID for the config entity.
+   *
+   * @var string
+   */
+  public $uuid;

Unique Universally Unique ID does not sound right.

+  /**
+   * {@inheritdoc}
+   */
+  public function save() {
+    // Build an ID if none is set.
+    if (empty($this->id)) {
+      $this->id = $this->id();
+    }
+    return parent::save();
+  }

"Build an ID if none is set" would imply using isset() instead of empty(). Not sure what is actually wanted here and whether it would make a difference.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new147.47 KB
PASSED: [[SimpleTest]]: [MySQL] 57,594 pass(es).
[ View ]

I rerolled and made the first change to the comments that was suggested. The second comment exists in EntityDisplay and Breakpoint as well, so if we're going to change it it should be consistent. I think it is fine.

Setting to RTBC as requested by scor in #99.

Status:Reviewed & tested by the community» Needs work

This looks like an amazing clean-up, great!

+++ b/core/modules/rdf/lib/Drupal/rdf/RdfMappingInterface.php
@@ -0,0 +1,90 @@
+  /**
+   * Gets the mapping for the bundle-level data.
+   *
+   * @return array
+   *   The bundle mapping.
+   */
+  public function getBundleMapping();
+
+  /**
+   * Gets the mapping config for the bundle-level data.
+   *
+   * @return array|null
+   *   The bundle mapping, or NULL if there is no mapping.
+   */
+  public function getBundleMappingConfig();

What's the difference between the bundle mapping and it's config. From reading the interface I've no clue - so at least documentation needs work, maybe method names also.

+++ b/core/modules/rdf/lib/Drupal/rdf/RdfMappingInterface.php
@@ -0,0 +1,90 @@
+   * Gets the prepared mapping for a field on the bundle.

Aha - I guess I get it. One is setting the actual mapping and the other applies the mapping, right?

If so, I'd suggest

applyFieldMapping($field_name);
getFieldMapping($field_name)
applyBundleMapping()
getBundleMapping()

Imo, the term mapping already refers to the mapping and not to the result of applying a mapping.

+++ b/core/modules/rdf/rdf.module
@@ -768,17 +573,20 @@ function rdf_preprocess_comment(&$variables) {
@@ -796,13 +604,16 @@ function rdf_field_attach_view_alter(&$output, $context) {

This hook will go away with #1969728: Implement Field API "field types" as TypedData Plugins I think. Could that be moved to a regular entity hook?

Also, how does that affect performance? Is there a need for caching mappings?

Issue tags:+Needs profiling

We quickly discussed caching in IRC. CMI is already cached and making that better is a general issue that we are working on so not a specific problem of RDF mappings, assuming it's not doing crazy things ( I haven't looked at the patch yet).

However, let's do some profiling to make sure that we're not introducing a regression here.

Thanks for taking a look.

Aha - I guess I get it. One is setting the actual mapping and the other applies the mapping, right?

I see you're point about the term mapping referring to the config itself. I wouldn't consider what is being done in getFieldMapping to be applying the mapping, though... I'd say a closer fit is "preparing".

What would you everyone think about something like getPreparedFieldMapping() or prepareFieldMapping()?

This hook will go away with #1969728: Implement Field API "field types" as TypedData Plugins I think. Could that be moved to a regular entity hook?

Yes, I think it could. Wim mentioned something about a new hook, hook_entity_view_alter(), which I still have to investigate. I would prefer to tackle that in a separate issue if that's OK with everyone.

Also, how does that affect performance? Is there a need for caching mappings?

It should actually help performance since rdf_entity_load was being called for the whole list of entities, as yched pointed out in #1920498: Have rdf.module only act on renderable entities. CMI's caching will also help us out here.

Status:Needs work» Needs review

#101: 1869600-101-refactor-rdf.patch queued for re-testing.

This hook will go away with #1969728: Implement Field API "field types" as TypedData Plugins I think. Could that be moved to a regular entity hook?

Yes, I think it could. Wim mentioned something about a new hook, hook_entity_view_alter(), which I still have to investigate. I would prefer to tackle that in a separate issue if that's OK with everyone.

I agree we should follow up in a separate issue. I've created #2021723: Switch rdf_field_attach_view_alter() to a regular entity hook implementation.

What's the difference between the bundle mapping and it's config. From reading the interface I've no clue - so at least documentation needs work, maybe method names also.

What would you everyone think about something like getPreparedFieldMapping() or prepareFieldMapping()?

I agree getBundleMapping() and getBundleMappingConfig() names are too similar. I'm not sure it's clear that prepareFieldMapping() would return the mappings. From a DX standpoint, should we keep getBundleMapping() as is since it's used quite often and is DX friendly, and instead rename getBundleMappingConfig() which is not as developer facing and more of a low level method?

Also, how does that affect performance? Is there a need for caching mappings?

Any performance improvement to the CMI caching will benefit RDF as well. The needs of the RDF module are no different than the needs to load the fields display settings when an entity is rendered. With this patch, the RDF mappings are loaded on demand. Compared to Drupal 7 where the RDF mappings are always loaded and attached/cloned into every single entity object, this patch should have a positive impact on the memory footprint.

I ran some basic page load test with :

50 article nodes on the front page

8.x:
Time per request:       626.836 [ms] (mean)
patch:
Time per request:       614.173 [ms] (mean)

Patch makes page load 2.1% faster

Node with 50 comments

8.x:
Time per request:       702.810 [ms] (mean)
patch:
Time per request:       707.957 [ms] (mean)

Patch makes page load 0.7% slower

Given that there are many other patches waiting on this megapatch to be committed, I would like if we could proceed with this patch and explore performance improvements in a follow up issue to improve CMI.

@performance: Thanks, that seems reasonable.

What would you everyone think about something like getPreparedFieldMapping() or prepareFieldMapping()?

That should work also, but imo what's important is also that the docs explain which one to use in which situation. E.g. I'd have no idea which one I'd should go with without actually looking at the implementation to check what it actually does.

StatusFileSize
new32.05 KB
new148.38 KB
FAILED: [[SimpleTest]]: [MySQL] 58,174 pass(es), 6 fail(s), and 1 exception(s).
[ View ]

Ok, rerolled the patch, changed the method names, and added some more documentation. I'll be on a flight from 10pm Eastern and won't arrive to my destination until 2pm Irish time, so if it needs to be rerolled in between, feel free to.

Status:Needs review» Needs work

The last submitted patch, 1869600-108-refactor-rdf.patch, failed testing.

I was not able to reproduce this test fail on my local. When I tried via UI, it passed. When I tried via script, two other tests failed but this one passed.

Status:Needs work» Needs review
Issue tags:-Needs profiling, -Entity Field API, -typed data

#108: 1869600-108-refactor-rdf.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs profiling, +Entity Field API, +typed data

The last submitted patch, 1869600-108-refactor-rdf.patch, failed testing.

These are the offending lines from TaxonomyTermFieldAttributesTest.php:

<?php
   
// Create the node.
   
$node = $this->drupalCreateNode(array('type' => 'article'));
   
$node->set($this->fieldName, array(
      array(
'tid' => $term1->id()),
      array(
'tid' => $term2->id()),
    ));
   
// Render the node.
   
$node_render_array = entity_view_multiple(array($node), 'teaser');
?>

This code used to work a couple of days ago (ecfbc27) on my machine and in #101. Not sure what change in core broke this.

#1965208: Convert TaxonomyTermReferenceItem to extend EntityReferenceItem

if I replace 'tid' with 'target_id' in testNodeTeaser(), the test is passing locally.

StatusFileSize
new727 bytes
new152.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,425 pass(es).
[ View ]

Updates patch in 108 replacing 'tid' with 'target_id' in testNodeTeaser() as suggested by scor.

Test passed locally.

Status:Needs work» Needs review

Status:Needs review» Needs work

+++ b/core/modules/forum/forum.module
@@ -1312,29 +1312,3 @@ function _forum_update_forum_index($nid) {
diff --git a/core/modules/locale/interdiff.txt b/core/modules/locale/interdiff.txt
diff --git a/core/modules/locale/interdiff.txt b/core/modules/locale/interdiff.txt
new file mode 100644
index 0000000..e69de29

there are some unwanted files that sneaked in your patch.

Status:Needs work» Needs review
StatusFileSize
new727 bytes
new148.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,689 pass(es).
[ View ]

Rerolled without the extra files. Thanks for reviewing carefully!

Status:Needs review» Reviewed & tested by the community

Thanks Lin and Jesse for following up on fago's documentation suggestions and fixing the target_id. Like I said in #99, this patch is the main refactoring for RDF in Drupal 8. It aligns the RDF module with the new Entity API and it also enables RDF to use the new config system to store the RDF mappings. I tested this again (same steps as #99). Several other RDF issues are waiting on this patch to be committed. This is good to go!

The doc improvements look good - way better now, thanks!

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks! Schema.org here we come ...

Title:Refactor RDF mappings to be inline with the new Entity Field API[HEAD BROKEN] Refactor RDF mappings to be inline with the new Entity Field API
Category:task» bug
Priority:Normal» Critical
Status:Fixed» Active

Patch to come.

Status:Active» Needs review
StatusFileSize
new6.52 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Status:Needs review» Fixed

Committed the emergency patch in #123 to 8.x. Thanks for stepping up Tim! Much appreciated.

Title:[HEAD BROKEN] Refactor RDF mappings to be inline with the new Entity Field APIRefactor RDF mappings to be inline with the new Entity Field API
Category:bug» task
Priority:Critical» Normal

Title:Refactor RDF mappings to be inline with the new Entity Field APIChange notice: Refactor RDF mappings to be inline with the new Entity Field API
Status:Fixed» Active
Issue tags:+Needs change record

Thanks Tim for jumping in.

We'll need to write a change notification for this: https://drupal.org/contributor-tasks/draft-change-notification.

Priority:Normal» Critical
Issue tags:-Needs profiling

Creating a change notice is by definition a critical task AFAIK.

Also: profiling was performed.

Posted a follow-up for node_rdf_mapping(). #2030953: Remove node_rdf_mapping().

Title:Change notice: Refactor RDF mappings to be inline with the new Entity Field APIReviewing change notice: Refactor RDF mappings to be inline with the new Entity Field API
Assigned:Unassigned» kay_v

Reviewing the above change notices for conformity with the guidelines for change notifications.

Status:Needs review» Fixed

The 4 change notifications listed in #128 contain all the information requested in the guidelines for writing up change notifications; they look great to me and I've marked the issue as fixed.

@linclark, the author of the change notifications, also noted verbally at the 2013 NYC Camp Core Sprint, that there are changes coming to RDF mapping that will require updates to related change notifications (e.g. Replaced rdf_mapping_save|load|delete with RdfMapping configurable; the Drupal 8 example code in that notification currently uses the Drupal 7 default RDF namespaces (e.g. 'types' => array('sioc:Post') as a placeholder).

Title:Reviewing change notice: Refactor RDF mappings to be inline with the new Entity Field APIRefactor RDF mappings to be inline with the new Entity Field API
Priority:Critical» Normal
Issue tags:-Needs change record

Great, thanks Kay! I'm changing the issue title and priority back now.

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

Issue summary:View changes

Updated to reflect todo status.