Problem/Motivation
Currently, we attach an RDF mapping nested array to each entity. There are a number of issues with the current API.
- 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.
- The nested array structure can be hard to read.
- 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.
$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, andpredicates
,datatype
, andcallback
for fields. This changes totypes
for bundles, andproperties
,datatype
(remains the same), anddatatype_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?
Comment | File | Size | Author |
---|---|---|---|
#123 | rdf-1869600-123.patch | 6.52 KB | tim.plunkett |
#118 | 1869600-118-refactor-rdf.patch | 148.39 KB | jesse.d |
#118 | interdiff.txt | 727 bytes | jesse.d |
#115 | 1869600-115-refactor-rdf.patch | 152.08 KB | jesse.d |
#115 | interdiff.txt | 727 bytes | jesse.d |
Comments
Comment #1
scor CreditAttribution: scor commentedI 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.
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.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedNo idea why that tag was removed.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedThe way I have it working now, this is possible. The RdfMappingManager uses MappingEvents. It dispatches events in 4 circumstances:
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.
Comment #5
fagoad #2: Great to see you are working on that!
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.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedApart from that we could do per type (-> field type) mapping defaults, which then could be overridden.
Ooooo, I like where this is going...
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedThe 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.
This patch also:
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.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedThe patch that introduced the SiteSchema and RdfMappingManager (which was included in the __for-testing patch above) were committed to core today.
Changes since #7:
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedWhoops, forgot that I had deleted edit module because it was causing an exception in simpletest.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedGah, I'm starting to see why people don't patch on Saturday nights. Forgot to remove a debugging line from index.php
Comment #16
fagoThanks 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?
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for reviewing the architecture, glad is seems sound :)
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.
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.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch just moves all of the mapping classes to their own directory.
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedSmall copy paste error was breaking install profile.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedHad to merge in changes in HEAD.
Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commentedI'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.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedFixed config issue by using a semicolon instead of a colon.
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch fixes the CRUD test, removes the MappingHookTest, and fixes the two namespace tests.
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedThis fixes the taxonomy term mapping to use the new system. It removes the mappings that don't actually get placed in HTML.
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #36
scor CreditAttribution: scor commentedMy 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"?
the documentation doesn't match or is missing.
I believe these are the same per your comment in #9, i.e. there should only be datatype_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'.
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedSorry, 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 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.
Comment #37.0
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdated the summary.
Comment #37.1
Anonymous (not verified) CreditAttribution: Anonymous commentedClarify proposal.
Comment #37.2
Anonymous (not verified) CreditAttribution: Anonymous commentedAdd remaining tasks.
Comment #37.3
Anonymous (not verified) CreditAttribution: Anonymous commentedFix HTML code tag issue.
Comment #38
fagoYep, 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" in the UI text and help.
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.
Comment #39
Anonymous (not verified) CreditAttribution: Anonymous commentedThis is just a reroll.
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedRather, this is just a reroll
Comment #41
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch cleans up the handling of the config entity for mappings.
Comment #43
Anonymous (not verified) CreditAttribution: Anonymous commented#41: 1869600-41-refactor-rdf.patch queued for re-testing.
Comment #45
scor CreditAttribution: scor commentedRerolling 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 :)
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.
Comment #48
scor CreditAttribution: scor commentedLike 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)
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks, 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?
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedJust 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.
Comment #51
fagowow, 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.
Couldn't that be determined based upon typed data, e.g. if it's an entity reference have 'rel'?
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?)
Comment #52
scor CreditAttribution: scor commentedyes, 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.
Comment #53
scor CreditAttribution: scor commentedrerolled 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?
Comment #55
mlncn CreditAttribution: mlncn commented#53: 1869600-53-refactor-rdf.patch queued for re-testing.
Comment #57
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #58
Anonymous (not verified) CreditAttribution: Anonymous commentedOK, here is a patch that installs. I haven't made any substantial changes from #53 yet.
Comment #59
Anonymous (not verified) CreditAttribution: Anonymous commentedI'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.
Comment #60
scor CreditAttribution: scor commentedthanks Lin. The link in #59 doesn't work, this is probably the right link: https://drupal.org/sandbox/linclark/2011168
Comment #62
Anonymous (not verified) CreditAttribution: Anonymous commentedSo 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:
Comment #63
scor CreditAttribution: scor commentedI 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.
Comment #64
Anonymous (not verified) CreditAttribution: Anonymous commentedPosting 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.
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch fixes the tests and also fixes a merge fail which had over written some of our changes to files outside of RDF module.
Comment #68
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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:
Comment #68.0
Anonymous (not verified) CreditAttribution: Anonymous commentedAdd task
Comment #69
scor CreditAttribution: scor commented+1. This looks better DX than the former:
edit: s/UX/DX
Comment #70
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #72
Anonymous (not verified) CreditAttribution: Anonymous commented#70: 1869600-70-refactor-rdf.patch queued for re-testing.
Comment #74
Anonymous (not verified) CreditAttribution: Anonymous commentedNot sure why the testbot is failing on that, since I can install just fine on my computer. Tried updating my local and rerolling.
Comment #76
Anonymous (not verified) CreditAttribution: Anonymous commentedThis should at least run.
Comment #78
Anonymous (not verified) CreditAttribution: Anonymous commentedDoh! forgot to add the RdfMapping plugin.
Comment #80
Anonymous (not verified) CreditAttribution: Anonymous commentedSo 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.
Comment #82
Anonymous (not verified) CreditAttribution: Anonymous commentedOk, I think this one should pass all tests. It needs more comments before RTBC, though.
Comment #83
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch just removes some docs for hook_rdf_mapping and adds code style changes based on Code Sniffer.
Comment #84
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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:
rdf_mapping_load()
,_rdf_get_default_mapping()
,_rdf_mapping_load()
,_rdf_mapping_load_multiple()
,rdf_mapping_save()
, andrdf_mapping_delete()
,setUp()
, rather than relying on mapping hooksSome Questions/Follow-ups
Comment #84.0
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdated summary.
Comment #86
Anonymous (not verified) CreditAttribution: Anonymous commentedThe 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.
Comment #88
Anonymous (not verified) CreditAttribution: Anonymous commentedRerolled.
Comment #89
scor CreditAttribution: scor commentedThanks 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.
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.
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.
Comment #90
Anonymous (not verified) CreditAttribution: Anonymous commentedIn looking at the configurables docs more, this should have a ConfigEntityInterface.
Comment #91
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #93
scor CreditAttribution: scor commentedWhere 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).
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?
Comment #94
Anonymous (not verified) CreditAttribution: Anonymous commentedThe 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.
Comment #95
Anonymous (not verified) CreditAttribution: Anonymous commented#91: 1869600-91-refactor-rdf.patch queued for re-testing.
Comment #96
scor CreditAttribution: scor commentedok. Looks like targetEntityType is used pretty often in core in the yml files, so I guess developers will get used to it.
I agree.
Comment #97
scor CreditAttribution: scor commentedThe second remark in #93 needs to be resolved.
Comment #98
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch will no longer apply, so I rerolled, removed the todo, and fixed the issue from #93.
Comment #99
scor CreditAttribution: scor commentedI'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.
Comment #100
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedBesides two minor issues with the documentation it looks good to me.
Unique Universally Unique ID does not sound right.
"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.
Comment #101
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.
Comment #102
fagoThis looks like an amazing clean-up, great!
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.
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.
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?
Comment #103
BerdirWe 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.
Comment #104
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for taking a look.
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()
orprepareFieldMapping()
?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.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.
Comment #105
Anonymous (not verified) CreditAttribution: Anonymous commented#101: 1869600-101-refactor-rdf.patch queued for re-testing.
Comment #106
scor CreditAttribution: scor commentedI 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.
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?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
Patch makes page load 2.1% faster
Node with 50 comments
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.
Comment #107
fago@performance: Thanks, that seems reasonable.
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.
Comment #108
Anonymous (not verified) CreditAttribution: Anonymous commentedOk, 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.
Comment #110
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.
Comment #111
Anonymous (not verified) CreditAttribution: Anonymous commented#108: 1869600-108-refactor-rdf.patch queued for re-testing.
Comment #113
scor CreditAttribution: scor commentedThese are the offending lines from TaxonomyTermFieldAttributesTest.php:
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.
Comment #114
scor CreditAttribution: scor commented#1965208: Convert TaxonomyTermReferenceItem to extend EntityReferenceItem
if I replace 'tid' with 'target_id' in testNodeTeaser(), the test is passing locally.
Comment #115
jesse.d CreditAttribution: jesse.d commentedUpdates patch in 108 replacing 'tid' with 'target_id' in testNodeTeaser() as suggested by scor.
Test passed locally.
Comment #116
jesse.d CreditAttribution: jesse.d commentedComment #117
scor CreditAttribution: scor commentedthere are some unwanted files that sneaked in your patch.
Comment #118
jesse.d CreditAttribution: jesse.d commentedRerolled without the extra files. Thanks for reviewing carefully!
Comment #119
scor CreditAttribution: scor commentedThanks 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!
Comment #120
fagoThe doc improvements look good - way better now, thanks!
Comment #121
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks! Schema.org here we come ...
Comment #122
tim.plunkettPatch to come.
Comment #123
tim.plunkettSee #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API.
Comment #124
Dries CreditAttribution: Dries commentedCommitted the emergency patch in #123 to 8.x. Thanks for stepping up Tim! Much appreciated.
Comment #125
tim.plunkettDries committed the fix.
http://drupalcode.org/project/drupal.git/commit/79aa8a6
Comment #126
scor CreditAttribution: scor commentedThanks Tim for jumping in.
We'll need to write a change notification for this: https://drupal.org/contributor-tasks/draft-change-notification.
Comment #127
Wim LeersCreating a change notice is by definition a critical task AFAIK.
Also: profiling was performed.
Comment #128
Anonymous (not verified) CreditAttribution: Anonymous commentedI created the following change notices
Comment #129
webflo CreditAttribution: webflo commentedPosted a follow-up for node_rdf_mapping(). #2030953: Remove node_rdf_mapping().
Comment #130
kay_v CreditAttribution: kay_v commentedReviewing the above change notices for conformity with the guidelines for change notifications.
Comment #131
kay_v CreditAttribution: kay_v commentedThe 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).
Comment #132
Anonymous (not verified) CreditAttribution: Anonymous commentedGreat, thanks Kay! I'm changing the issue title and priority back now.
Comment #133.0
(not verified) CreditAttribution: commentedUpdated to reflect todo status.