Problem/Motivation

When an entity is POSTed using JSON-LD, the @type attribute is used to identify the bundle of the entity. The value of the @type attribute has to be a URI. We want modules to be able to specify which URIs are acceptable and, when a URI it recognizes comes in, let the system know which kind of entity it should create.

Proposed resolution

Use an event to pass the array of @type URIs around to different modules. First, RDF module itself looks to see whether it recognizes the URI as a site schema URI. If it does, it stops propagation of the event. Otherwise, it is passed to the other subscribers, which see if they recognize it. If a subscriber does recognize it, they find the corresponding site schema URI and set that.

This requires two new concepts:

SiteSchema—a site generated vocabulary, which gives each Drupal bundle and field instance their own URI.
RdfMappingManager—This handles the mapping event.

Original report by linclark

This issue is a spin-out from #1838596: Add deserialize for JSON-LD. In particular, see comments #23, #24, #35, #36.

Problem/Motivation

When an entity is POSTed using JSON-LD, the @type attribute is used to identify the bundle of the entity. The value of the @type attribute has to be a URI.

There is a patch for RDF module which gives each bundle a URI, #1831286: Provide machine-readable description of entity/field/property. The question now is how to use that URI to figure out the entity type and bundle name to be used with entity_create().

Proposed resolution

There are three options

  • Run a subrequest on the @type URI (restricted to the same domain)
  • Create an index in the database, using the URI as the key
  • Extract route parameters from the @type URI

Here are the pros and cons I see.

Subrequest

Pro:

  • Maintains the opacity of the URI, which increases backwards compatibility and RESTful-ness.
  • It gives us the option to later use URIs from other domains. This would require a number of other considerations.

Con:

  • If the subrequest for the schema fails, we don't have the data.

Creating an index

Pro:

  • Maintains the opacity of the URI
  • Don't have to mint terms in the Drupal.org namespace

Con:

  • Have to maintain the index.
  • Limits us to only accepting @type URIs within the same domain.

Parsing the route

Pro:

  • Don't have to mint terms in the Drupal.org namespace

Con:

  • Requires working backwards from a route to a URI template, which feels like rewriting parts of the routing system.
  • Limits our flexibility with URI templates
Files: 
CommentFileSizeAuthor
#49 1852812-49-type-uri-mapper.patch61.45 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 49,106 pass(es).
[ View ]
#49 interdiff.txt34.56 KBlinclark
#44 1852812-43-type-uri-mapper.patch60.37 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 49,371 pass(es).
[ View ]
#44 interdiff.txt610 byteslinclark
#43 interdiff.txt7.02 KBlinclark
#43 1852812-42-type-uri-mapper.patch60.41 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 49,398 pass(es).
[ View ]
#40 1852812-40-type-uri-mapper.patch59.38 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 49,408 pass(es).
[ View ]
#40 interdiff.txt10.54 KBlinclark
#37 1852812-37-type-uri-mapper.patch59.27 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 49,377 pass(es).
[ View ]
#37 interdiff.txt7.97 KBlinclark
#33 1852812-33-type-uri-mapper.patch59.74 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 49,435 pass(es).
[ View ]
#31 1852812-31-type-uri-mapper.patch58.28 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 48,484 pass(es), 725 fail(s), and 176 exception(s).
[ View ]
#30 1852812-30-type-uri-mapper.patch58.25 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 48,507 pass(es), 722 fail(s), and 175 exception(s).
[ View ]
#28 1852812-28-type-uri-mapper__for-testing.patch37.04 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 49,400 pass(es), 0 fail(s), and 3,886 exception(s).
[ View ]
#28 1852812-28-type-uri-mapper__for-review.patch16.64 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1852812-28-type-uri-mapper__for-review.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 interdiff.txt7.76 KBlinclark
#27 1852812-27-type-uri-mapper__for-testing.patch30.84 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 49,247 pass(es).
[ View ]
#27 1852812-27-type-uri-mapper__for-review.txt10.03 KBlinclark
#27 interdiff.txt4.13 KBlinclark
#23 1852812-type-uri-mapper__for-testing.patch24.3 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 34,378 pass(es), 11,343 fail(s), and 16,539 exception(s).
[ View ]
#23 1852812-type-uri-mapper__for-review.txt9.08 KBlinclark

Comments

fago’s picture

First, though, I would like someone to explain what the disadvantages of the subrequest approach are.

Limiting to local routes works, ESI like caching sounds good also, but still it sounds like a lot being involved for such a "simple task" as getting the entity type from the route. I agree with you that parsing the URL isn't a nice solution, but the router system should be able to provide us with the entity type.

The other point is that the serializer system goes with $class as $target what doesn't really match our system. I had the some troubles with the symfony vaildator, i.e. they assume that everything is bound to the class. But for us, it's the other way round we've metadata based upon a name which specifies a class also.
Thus, I'd see it as natural to pass on the $entity_type, or more generally probably any data type from the typed data API to the serializer component as target "type" and get the metadata from there. I guess we should figure out with upstream whether passing in a type-name as $class would be ok or whether changing that would be possible.

I must say that the more I think about it, parsing the schema sounds wrong to me. What if I'd not implement JSON-LD but just my custom json without any schema? How should the deserializer then figure out the target entity type? It ought to be in the $class parameter.

linclark’s picture

Issue tags:-Stalking Crell

The other point is that the serializer system goes with $class as $target what doesn't really match our system.... How should the deserializer then figure out the target entity type? It ought to be in the $class parameter.

We had a long discussion about this in IRC, and I believe that clarified the concerns raised here. Basically, the code calling serialize deserialize will have to determine which class to use as the target class based on the route. REST module currently has a serialization_class annotation property for that.

Because the plugins implemented in the REST module are not entity type/bundle specific, we should simply use the parent class (e.g. Entity) as the serialization_class. This will ensure that the correct denormalizer is used. That denormalizer will then get the entity type/bundle from the data (using the @type URI).

This leaves the original question of this issue open.

EDIT: reworded for clarification

linclark’s picture

Issue tags:+Stalking Crell
linclark’s picture

Issue tags:+Stalking Crell

Actually....

I'm liking the idea of the index more as I think about it. It could allow us to use external route URIs. For example, I could have the following database tables:

uri entity type bundle
http://mysite.com/site-schema/content-staging/node/article node article
http://mysite.com/site-schema/syndication/node/article node article
site_schema_uri external_equivalent_uri
http://mysite.com/site-schema/content-staging/node/article http://staging.mysite.com/site-schema/content-staging/node/article

Then, when processing the @type, it could do the following.

Get @type URI.

If @type URI equals any uri in Table 1
    Create entity
Else if @type URI equals any external_equivalent_uri in Table 2
    Use corresponding site_schema_uri to look up term in Table 1
    Create entity
Else
    Dereference @type URI to get the external schema
    Retrieve list of equivalentClass URIs defined in the external schema
    If schema->equivalentClass URI equals any uri in Table 1
        Create entity

In this scenario, if someone posted a JSON-LD object that used mysite:node/article or staging-mysite:node/article, it would get handled by the index. But it would also support friend_site:post as long as the schema available at friend_site:post asserted that friend_site:post and mysite:node/article are equivalent classes.

We wouldn't enable retrieving external schemas just yet because there will be security concerns to consider. But we could enable mapping within the index without much trouble.

fago’s picture

This will ensure that the correct denormalizer is used. That denormalizer will then get the entity type/bundle from the data (using the @type URI).

Does that mean the system would accept e.g. terms at /node routes then? I do not think it should.

linclark’s picture

Let me make sure I understand the concern... so you are asking whether you could use @type: http://mysite.com/site-schema/content-staging/node and have it create a node without a bundle? If that is the question, then no, we would always require that the @type include both the entity type and the bundle. For those entities without bundles, the bundle name would be the same as the entity type, just as with Entity::bundle().

fago’s picture

Nope.

I'm asking whether I can send the payload I'd send to create a term at /taxonomy/term also to /node and it will work? That shouldn't work, so I just wanted to verify that.

linclark’s picture

That concern isn't addressed by serialization. The REST plugin system that Klaus is defining is what handles routes, what operations are available on them, etc. That is completely orthogonal to this issue.

To be clear, these URIs are identifiers. They simply identify a particular bundle in our system using a full URI. They aren't REST routes. They don't accept any POST or PUT requests. The routes that do accept POST and PUT requests are registered in \Drupal\rest\EventSubscriber\RouteSubscriber::dynamicRoutes().

linclark’s picture

Just clarified the issue with fago in IRC. For the case where a node @type is submitted to a taxonomy term route, I would say that it's the job of the REST plugin to verify that the entity returned from deserialize is the correct entity type. Otherwise, we'd have to extend Serializer to take entity_type instead of target class, which would also make it incompatible with non-entities in our own system.

fago’s picture

Otherwise, we'd have to extend Serializer to take entity_type instead of target class, which would also make it incompatible with non-entities in our own system.

Yeah, that is what I've been suggesting all the time. ;-) To make it general, you'd use a type as registered by the typed data API. As discussed, symfony uses both $type and $class already, so I think this could need some clarifications upstream anyway.

linclark’s picture

That couples the Serializer to the entity system. We explicitly want to support non-entities as well. From my understanding, that's exactly why we have the plugin system in REST... so that a contrib module could register REST routes for non-entities.

I don't see much of a downside to having the EntityResource check whether the returned entity is the correct type... that check only needs to happen for entities, anyways, so that's where it belongs. I do see a downside to coupling our Serializer to the entity system.

linclark’s picture

Additionally, if we change the signature to deserialize($data, $entity_type, $format), we could no longer use recursion to denormalize the field values, since field types don't have an entity_type.

fago’s picture

I'm suggesting to use deserialize($data, $type, $format), with $type being any valid TypeData type. If we'd like to have more interopability with possibly co-existing symfony libraries, we could do "Drupal.Core.TypeData.$type" also.

linclark’s picture

I'm not sure that any other libraries would see the value in something that looked so similar to a PSR-0 class name without actually being a class name. It also means that the FieldItemNormalizer would have to do complex computation to figure out whether Drupal.Core.TypeData.integer should be handled by it, rather than just checking whether IntegerItem is a subclass of FieldItemBase.

fago’s picture

At that level the interface does not say it's supposed to be class, but as said the upstream usage of $class vs $type is not clear to me at all.

<?php
/**
* Deserializes data into the given type.
*
* @param mixed $data
* @param string $type
* @param string $format
*
* @return object
*/
   
public function deserialize($data, $type, $format);
?>
linclark’s picture

The $type is what gets passed to all deserialize functions. For example, here is the function body of Serializer::deserialize

    final public function deserialize($data, $type, $format)
    {
        if (!$this->supportsDecoding($format)) {
            throw new UnexpectedValueException('Deserialization for the format '.$format.' is not supported');
        }

        $data = $this->decode($data, $format);

        return $this->denormalize($data, $type, $format);
    }

There is no manipulation of the $type variable. Therefore, it must be the same value for both deserialize() and denormalize().

.... and I'm also just now realizing that this is declared as final so we couldn't even override it. We'd either have to spin up our own Serializer class and copy most of the code, or we would have to get it changed upstream. I guess we could technically abuse it, and just pass a non-class string anyway... but that seems likely to cause confusion and bugs.

It seems like we'd have to do a lot of work to couple the Serializer to our Typed Data API. If we want to keep them decoupled, all we have to do is have the REST plugin check whether the deserialized entity is the same entity type as it is expecting. Decoupled and less work seems like a winner to me.

klausi’s picture

Currently the RequestHandler in REST module uses the following code:

<?php
$definition
= $resource->getDefinition();
$class = $definition['serialization_class'];
$unserialized = $serializer->deserialize($received, $class, 'drupal_jsonld');
?>

So now we cannot simply continue with $unserialized, because it could be anything. The serializer can't be trusted, it could return a taxonomy term object although we received the request on a node route and $class contains "\Drupal\Core\Entity\Node" or similar. It feels strange that the serializer just ignores the $class parameter.

What would be the solution to avoid this security issue?

<?php
$definition
= $resource->getDefinition();
$class = $definition['serialization_class'];
$unserialized = $serializer->deserialize($received, $class, 'drupal_jsonld');
if (!(
$unserialized instanceof $class)) {
 
$type = gettype($unserialized);
  throw new
SecurityException("Someone tried to hack your route with a $type object!");
}
?>

Really?

So the next thing I'll do is add a simpletest to REST module that taxonomy terms cannot be created on node routes in #1839346: REST module: POST/create.

linclark’s picture

We could use the class that is passed in. I'm fine with switching from entity_create to new {serialization_class}() if that works.

That is different than what fago is suggesting, though. He is suggesting that we would pass in an entity type string.

Even if we use the class, from what I can gather we still need to get the entity type out of the data. This is because multiple entity types can use the same class. Therefore, we would still need to do the checking in REST module to ensure it is the same entity type.

fago’s picture

Even if we use the class, from what I can gather we still need to get the entity type out of the data. This is because multiple entity types can use the same class. Therefore, we would still need to do the checking in REST module to ensure it is the same entity type.

Yeah. I'd suggest opening a follow-up for checking that or moving to entity types - so it does not hold up things. Right now every entity in core has its own class afaik, but the api will allow for multiple entity types with the same class (ECK use case).

So I'd be happy to see the $class param used in whatever way as long as it determines what the serializer is going to create.

There is no manipulation of the $type variable. Therefore, it must be the same value for both deserialize() and denormalize().

Exactly, that is what strange in upstream and needs clarifications.

It seems like we'd have to do a lot of work to couple the Serializer to our Typed Data API.

It's true that type inheritance checking is nothing the typed data API takes care of now, so you'd have to get an instance and do use the classes to do that. Not too complex, but not too nice either. I see us needing a proper way to denote type inheritance for other stuff also though, so I suppose this will become easier later on.

So keeping TypeData based experiments to #1846000: Add serializer support for JSON and AJAX for now and taking the learnings over to this one (or not) sounds like a reasonable approach to me.

$class = $definition['serialization_class'];

It's not the class used for serialization, is it? So this sounds a bit misnamed to me. I cannot think of a really good name right now though, maybe resource_class ?

Crell’s picture

Catching up in this thread... I am really not OK with binding the serializers to the TypedData system. We already have an implementation of REST that exposes Watchdog records, which are not TypedData. There's no reason for those two systems to have a hard dependency.

Lin: With the lookup table route, how would one populate it in the first place? What automation is there, if any?

linclark’s picture

The first table (which contains the site-schema URIs and their corresponding entity types/bundles) would be populated automatically. Whenever someone modifies a content type, that table would be updated. The vocabularies for both content deployment and external API/syndication would both be listed there.

The second table (the mappings to external types) could potentially be populated automatically for the content deployment vocabulary. If the content staging and live sites are assumed to have the same configuration, then the site admin could just specify the URI of both sites and the mappings could be generated automatically (since the terms are guaranteed to be the same except for base URI... we're not permitting configuration of RDF URI patterns).

If we want to allow mapping for the external API/syndication vocabulary, then that would probably have to be done manually. We expect vocabularies like Schema.org to be used in that case, which requires human intelligence to determine whether two "Article" classes are really the same. We could provide an API/user interface for doing such mappings, or we could leave it for contrib.

Crell’s picture

I understood slightly more than half of what you just said. :-) I think, given that, I'd rather leave in the hooks (metaphorically speaking) to build such mapping tables in contrib. Let core just handle the easy automated ones and let contrib handle anything fancier, including those that would be needed for staging between two sites. We're going to need a deploy module in contrib to actually leverage this stuff for content staging anyway, so may as well make our lives easier for now.

linclark’s picture

Title:Decide how to get entity type/bundle metadata from JSON-LD @type URI» Use cache to get entity type/bundle metadata from JSON-LD @type URI
Status:Active» Needs review
StatusFileSize
new9.08 KB
new24.3 KB
FAILED: [[SimpleTest]]: [MySQL] 34,378 pass(es), 11,343 fail(s), and 16,539 exception(s).
[ View ]

So I have implemented the event-based technique we discussed on the call. I am currently looking for architectural review, as well as feedback on the clarity of variable names/comments.

To use this, deserialize() would do the following:

$mappingManager = drupal_container()->get('rdf.site_schema.mapping_manager');
$typedDataIds = $mappingManager->getTypedDataIds(array('http://d8.l/site-schema/content-staging/node/article'));

The $typedDataIds array would be:

array(
  'entity_type' => 'node',
  'bundle' => 'article',
)

In the background, this fires the RdfMappingEvents::MAP_INPUT_TYPE event. The default listener is a simple URI matcher. It compares an incoming @type URI to the list of site schema type URIs and if it finds a match, returns it. Other modules could provides listeners which have more complex ways of mapping the incoming URI to an internal type URI. For example, Deploy could use this to swap out the staging site URI for the live site URI.

Whenever a new bundle or entity type is added, type URIs need to be updated, so it has the same cycle as the entity_info cache. Because of this, I used a tagged cache to handle the storage of the materialized type URIs.

The for-testing patch includes code from #1831286: Provide machine-readable description of entity/field/property.

Status:Needs review» Needs work

The last submitted patch, 1852812-type-uri-mapper__for-testing.patch, failed testing.

Crell’s picture

Status:Needs work» Needs review
+++ b/core/modules/rdf/lib/Drupal/rdf/MapInputTypesEvent.php
@@ -0,0 +1,90 @@
+class MapInputTypesEvent extends Event {

Do we need full bean methods on this class? When would we be getting/setting the protected variables other than in the constructor?

+++ b/core/modules/rdf/lib/Drupal/rdf/RdfMappingManager.php
@@ -0,0 +1,169 @@
+  public function __construct(EventDispatcherInterface $dispatcher) {
+    $this->dispatcher = $dispatcher;
+    $this->siteSchemas = array(
+      new SiteSchema(SchemaConstants::CONTENT_STAGING),
+      new SiteSchema(SchemaConstants::SYNDICATION),
+    );
+  }

So how would I add more SiteSchemas?

+++ b/core/modules/rdf/lib/Drupal/rdf/RdfMappingManager.php
@@ -0,0 +1,169 @@
+    $cache = cache()->get($bin);

The cache should be injected. We can do that now, can't we? Or is that still blocked somewhere?

+++ b/core/modules/rdf/lib/Drupal/rdf/RdfMappingManager.php
@@ -0,0 +1,169 @@
+      switch ($bin) {
+        case 'rdf:site_schema:types':
+          $data = $this->buildTypeCache();
+          break;
+      }

I have no idea why this switch is here.

+++ b/core/modules/rdf/lib/Drupal/rdf/RdfMappingManager.php
@@ -0,0 +1,169 @@
+    $this->dispatcher->addListener(RdfMappingEvents::MAP_INPUT_TYPES, function (MapInputTypesEvent $event) {
+      $inputUris = $event->getInputUris();
+      $siteSchemaTypes = $event->getSiteSchemaTypes();
+      foreach ($inputUris as $inputUri) {
+        if (isset($siteSchemaTypes[$inputUri])) {
+          $event->setSiteSchemaUri($inputUri);
+          $event->stopPropagation();
+        }
+      }
+    });

While this works, I would strongly recommend against registering ad-hoc listeners. Just make it a subscriber. There's really no performance benefit to doing it inline, but it makes later optimization more difficult. It also makes it impossible for someone else to remove or change the priority of in a compiler pass if necessary.

linclark’s picture

When would we be getting/setting the protected variables other than in the constructor?

I'm not sure whether I understand the question, but I'll try to answer it and you can tell me if I didn't understand it. The variables are:

  • inputUris—This would be accessed by each listener to figure out which RDF type URIs are coming in to the system
  • siteSchemaTypes—This is the list of type URIs from the cache. Listeners might use this, though the default listener is probably the only one that really needs it.
  • siteSchemaUri—This is set by the listener that finds a match.

There might be a better way to accomplish this within the Events system, I'm open to suggestions on that.

So how would I add more SiteSchemas?

The plan is to only have those two SiteSchemas, one which corresponds to vnd.drupal.ld+json and the other which corresponds to ld+json. This is the model of the site. Terms from external vocabularies (like Schema.org) are mapped to that model. I think we want to limit how many models of your site you can have because the idea of modeling in general is confusing to most people. Since vnd.drupal.ld+json is totally fixed in its modeling and can't be altered, users only have to worry about configuring one model.

The list of the two SiteSchemas probably could be registered with the container instead. However, if registering it with the container implies that more SiteSchemas can be added for a site, we should hold off I think.

The cache should be injected. We can do that now, can't we?

I'm not sure on this one, I can look into it. You mean the whole cache object, right, not the return from get()?

I have no idea why this switch is here.

Because we will want to have a property cache in addition to the type cache, but I can take that out for now.

While this works, I would strongly recommend against registering ad-hoc listeners.

Makes sense, I'll switch it.

linclark’s picture

StatusFileSize
new4.13 KB
new10.03 KB
new30.84 KB
PASSED: [[SimpleTest]]: [MySQL] 49,247 pass(es).
[ View ]

I switched from a listener to an event subscriber.

I still need to check on the cache and temporarily remove the switch statement that Crell pointed out.

linclark’s picture

StatusFileSize
new7.76 KB
new16.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1852812-28-type-uri-mapper__for-review.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new37.04 KB
FAILED: [[SimpleTest]]: [MySQL] 49,400 pass(es), 0 fail(s), and 3,886 exception(s).
[ View ]

I've written a test which demonstrates how the content staging module would map a type URI from the staging site to a type URI from the live site's schema.

I'm posting this patch to make it clear for review. I'm currently working on injecting the cache, it's more complicated than I expected.

Status:Needs review» Needs work

The last submitted patch, 1852812-28-type-uri-mapper__for-review.patch, failed testing.

linclark’s picture

Status:Needs work» Needs review
StatusFileSize
new58.25 KB
FAILED: [[SimpleTest]]: [MySQL] 48,507 pass(es), 722 fail(s), and 175 exception(s).
[ View ]

I think this patch is pretty close. I added the injected cache, got rid of use of drupal_container, added exceptions, and made a number of other corrections, so the interdiff is pretty big. It might be better to just review the patch itself.

I also have simply included the patch from #1831286: Provide machine-readable description of entity/field/property in this. I think we should just commit it as one patch and then the additions that need to be made to the site schema can take place in follow up issues.

To summarize, this patch:

  • Creates a SiteSchema system, which generates the machine readable schema and the URIs we need for JSON-LD's @type attribute
  • Creates a site schema type cache, which can be used to get the typed data ids from the @type URI
  • Replaces the temporary URI parsing that klausi put in place so deserialize could go in with proper URI handling
  • Adds a RDF mapping manager, with a MAP_INPUT_TYPE event. This can be used by something like the Deploy module to accept @type URIs from other sites. An example of this is provided in the RdfMappingEventTest and its supporting module, rdf_test_mapping.
linclark’s picture

StatusFileSize
new58.28 KB
FAILED: [[SimpleTest]]: [MySQL] 48,484 pass(es), 725 fail(s), and 176 exception(s).
[ View ]

Actually, this is the patch I meant to attach.

Status:Needs review» Needs work

The last submitted patch, 1852812-31-type-uri-mapper.patch, failed testing.

linclark’s picture

Status:Needs work» Needs review
StatusFileSize
new59.74 KB
PASSED: [[SimpleTest]]: [MySQL] 49,435 pass(es).
[ View ]

Trying the fix for modules enabled/disabled test that katbailey used in #1846582: Restrict data retrieving routes to media type.

Status:Needs review» Needs work
Issue tags:-WSCCI, -json-ld, -Stalking Crell

The last submitted patch, 1852812-33-type-uri-mapper.patch, failed testing.

moshe weitzman’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI, +json-ld, +Stalking Crell

#33: 1852812-33-type-uri-mapper.patch queued for re-testing.

Crell’s picture

I only sort of follow the RDF parts of this, so this review is largely just a general programming tidiness review. Only a few problems, only one of them (abstract class) really significant.

+++ b/core/modules/jsonld/lib/Drupal/jsonld/JsonldEntityNormalizer.php
@@ -38,24 +40,31 @@ public function normalize($entity, $format = NULL) {
+    } catch (RdfMappingException $e) {
+      throw new UnexpectedValueException($e->getMessage());
+    }

If rethrowing an exception like that, you should pass the old exception to the new exception for a more complete backtrace. (New feature of PHP 5.3.)

+++ b/core/modules/jsonld/lib/Drupal/jsonld/JsonldEntityWrapper.php
@@ -72,13 +85,21 @@ public function getId() {
-    $entity_type = $this->entity->entityType();
+    $entityType = $this->entity->entityType();
     $bundle = $this->entity->bundle();
-    return url('site-schema/content-staging/' . $entity_type . '/' . $bundle, array('absolute' => TRUE));
+    switch ($this->format) {
+      case 'drupal_jsonld':
+        $schemaPath = SiteSchema::CONTENT_DEPLOYMENT;
+        break;
+      case 'jsonld':
+        $schemaPath = SiteSchema::SYNDICATION;
+    }
+    $schema = $this->siteSchemaManager->getSchema($schemaPath);
+    return $schema->bundle($entityType, $bundle)->getUri();

Minor point: these are local variables, not object properties, so they should remain $lower_case, not $camelCase.

+++ b/core/modules/rdf/lib/Drupal/rdf/RdfMappingException.php
@@ -0,0 +1,14 @@
+use Exception;
+
+/**
+ * Exception to use when no RDF mapping is found.
+ */
+class RdfMappingException extends Exception { }

Technically coding standards now say a global class should not be use'd and just be referenced with \Exception.

Although this could possibly be a good case for \LogicException or similar, depending on where it gets used.

+++ b/core/modules/rdf/lib/Drupal/rdf/SiteSchema/BundleSchema.php
@@ -0,0 +1,59 @@
+    $path = str_replace(array('{entity_type}', '{bundle}'), array($this->entityType, $this->bundle), self::URI_PATTERN);

It doesn't actually matter here, I think, but technically static::URI_PATTERN would be better than self::URI_PATTERN.

+++ b/core/modules/rdf/lib/Drupal/rdf/SiteSchema/EntitySchema.php
@@ -0,0 +1,68 @@
+    $path = str_replace('{entity_type}', $this->entityType , self::URI_PATTERN);

Ibid.

+++ b/core/modules/rdf/lib/Drupal/rdf/SiteSchema/SchemaTermBase.php
@@ -0,0 +1,69 @@
+abstract class SchemaTermBase {

This ought to have an interface to go with it; abstract classes should only be used as a convenience utility, and never as a specification definition.

linclark’s picture

StatusFileSize
new7.97 KB
new59.27 KB
PASSED: [[SimpleTest]]: [MySQL] 49,377 pass(es).
[ View ]

Thanks for the review, Crell. I made all the changes.

Status:Needs review» Needs work
Issue tags:-WSCCI, -json-ld, -Stalking Crell

The last submitted patch, 1852812-37-type-uri-mapper.patch, failed testing.

linclark’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI, +json-ld, +Stalking Crell

#37: 1852812-37-type-uri-mapper.patch queued for re-testing.

linclark’s picture

StatusFileSize
new10.54 KB
new59.38 KB
PASSED: [[SimpleTest]]: [MySQL] 49,408 pass(es).
[ View ]

I just renamed some functions and fixed some code style.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

OK, we've satisfied the reviewers.

klausi’s picture

+++ b/core/modules/jsonld/lib/Drupal/jsonld/JsonldEntityNormalizer.php
@@ -38,24 +40,31 @@ public function normalize($entity, $format = NULL) {
+    } catch (RdfMappingException $e) {
+      throw new UnexpectedValueException($e->getMessage(), 0, $e);
+    }

"catch" should be on a new line, see http://drupal.org/node/608166

+++ b/core/modules/jsonld/lib/Drupal/jsonld/JsonldRdfSchemaNormalizer.php
@@ -0,0 +1,51 @@
+  /**
+     * The interface or class that this Normalizer supports.
+     *
+     * @var string
+     */
+  protected static $supportedInterfaceOrClass = 'Drupal\rdf\SiteSchema\SchemaTermBase';
+
+  /**
+     * Implements \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize()
+     */
+  public function normalize($data, $format = NULL) {

indentation errors

+++ b/core/modules/jsonld/lib/Drupal/jsonld/JsonldRdfSchemaNormalizer.php
@@ -0,0 +1,51 @@
+    foreach ($graph as $termUri => $properties) {

$termUri is a local variable and should not use camelCase naming. See http://drupal.org/coding-standards#naming

+++ b/core/modules/jsonld/lib/Drupal/jsonld/Tests/JsonldTestSetupHelper.php
@@ -0,0 +1,96 @@
+class JsonldTestSetupHelper {

If an acronym is used in a class or method name, make it all caps (SampleXMLClass, not SampleXmlClass). So this should be JSONLDTestSetupHelper. This would also apply to any other class in the jsonld module, so I guess we leave it as is for now and fix that in a separate issue. See http://drupal.org/node/608152

+++ b/core/modules/jsonld/lib/Drupal/jsonld/Tests/NormalizeDenormalizeTest.php
@@ -25,7 +25,7 @@ class NormalizeDenormalizeTest extends WebTestBase {
-  public static $modules = array('language', 'entity_test');
+  public static $modules = array('language', 'entity_test', 'rdf');

interesting, this test works although it does not even enable the jsonld module ... I guess the lib folder of jsonld must somehow bleed through to the test setup.

+++ b/core/modules/jsonld/lib/Drupal/jsonld/Tests/NormalizeDenormalizeTest.php
@@ -170,17 +167,36 @@ function testDenormalize() {
+    } catch (UnexpectedValueException $e) {
+      $this->pass('Trying to denormalize entity data without @type results in exception.');
+    }

"catch" coding standards again, also elsewhere.

+++ b/core/modules/jsonld/lib/Drupal/jsonld/Tests/RdfSchemaSerializationTest.php
@@ -0,0 +1,55 @@
+    $entityType = $bundle = 'entity_test';

$entityType should be $entity_type

+++ b/core/modules/jsonld/lib/Drupal/jsonld/Tests/RdfSchemaSerializationTest.php
@@ -0,0 +1,55 @@
+    $arrayKeys = array_keys((array) $parsedTerm);

Same here.

+++ b/core/modules/jsonld/lib/Drupal/jsonld/Tests/SupportsSerializationTest.php
@@ -50,13 +46,8 @@ public static function getInfo() {
+    $setupHelper = new JsonldTestSetupHelper();

And here. And a lot of other places.

+++ b/core/modules/rdf/lib/Drupal/rdf/EventSubscriber/MappingSubscriber.php
@@ -0,0 +1,44 @@
+   *
+   * @param \Drupal\rdf\MapTypesFromInputEvent $event
+   */

Every @param directive needs to have a description. See http://drupal.org/node/1354#functions

I also can't say much about the technical correctness of the implementation, so leaving at RTBC as my minor coding standard comments should not hold up this issue either.

linclark’s picture

StatusFileSize
new60.41 KB
PASSED: [[SimpleTest]]: [MySQL] 49,398 pass(es).
[ View ]
new7.02 KB

Thanks for the code style review, I made all the changes except for two. I also added an exception. I don't think this requires setting back to needs review, but if it does feel free to.

I didn't yet change the local variable names, simply because scanning all the files will take a while. Hopefully I'll get a chance to do that before Friday.

I also didn't change file names to JSONLDXx. While code standards are supposed to improve readability, that change would reduce it in my opinion. I would suggest that we want to rethink that standard. Other PHP projects do not follow it. For example, Symfony uses "Json", and JSON-LD libraries use "JsonLd". We can discuss this in a separate issue, though.

linclark’s picture

StatusFileSize
new610 bytes
new60.37 KB
PASSED: [[SimpleTest]]: [MySQL] 49,371 pass(es).
[ View ]

Removed a stray slash from the last edit.

Status:Reviewed & tested by the community» Needs work
Issue tags:-WSCCI, -json-ld, -Stalking Crell

The last submitted patch, 1852812-43-type-uri-mapper.patch, failed testing.

linclark’s picture

Status:Needs work» Needs review
Issue tags:+WSCCI, +json-ld, +Stalking Crell

#44: 1852812-43-type-uri-mapper.patch queued for re-testing.

scor’s picture

Status:Needs review» Needs work

@linclark re naming convention in #43, see #1627350: Patch for: Case of acronyms in class names (SomethingXSSClassName versus SomethingXssClassName) where this decision was made. You might want to rally a few people first if you plan to reopen it for reconsideration...

scor’s picture

Status:Needs work» Needs review

x-post

linclark’s picture

StatusFileSize
new34.56 KB
new61.45 KB
PASSED: [[SimpleTest]]: [MySQL] 49,106 pass(es).
[ View ]

This just fixes the camel case issue.

linclark’s picture

Issue summary:View changes

Crossed off con.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

Only code style changes since last RTBC.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

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

Anonymous’s picture

Issue summary:View changes

Updated post patch.