Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add hook_jsonld_alter_field_mappings #31

Merged
merged 8 commits into from
Dec 19, 2018
Merged

add hook_jsonld_alter_field_mappings #31

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ff388e7
add hook_jsonld_alter_field_mappings
seth-shaw-unlv Dec 12, 2018
6f6fbec
coding standards
seth-shaw-unlv Dec 13, 2018
45f4ff6
implement hook_jsonld_alter_field_mappings
seth-shaw-unlv Dec 13, 2018
27ee621
cache field mappings and issue warnings for duplicates
seth-shaw-unlv Dec 13, 2018
2324e44
trying to make phpcpd happy
seth-shaw-unlv Dec 13, 2018
aecc0e8
revised field mappings hook name
seth-shaw-unlv Dec 17, 2018
73a87d0
revised field mappings hook name (again)
seth-shaw-unlv Dec 19, 2018
22260da
update const to match
seth-shaw-unlv Dec 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 102 additions & 0 deletions jsonld.api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,105 @@ function hook_jsonld_alter_normalized_array(EntityInterface $entity, array &$nor
}
}
}

/**
* Hook to alter the field type mappings
*
* Be aware that drupal field definitions can be complex.
* e.g text_with_summary has a text, a summary, a number of lines, etc
* we are only dealing with the resulting ->value() of all this separate
* pieces and mapping only that as a whole.
*
* @return string[]
* An associative array of field type mappings where the key is the field type
* and the value is the type mapping.
*/
function hook_jsonld_alter_field_mappings(){
return [
"comment" => [
"@type" => "xsd:string",
],
"datetime" => [
"@type" => "xsd:dateTime",
],
"file" => [
"@type" => "@id",
],
"image" => [
"@type" => "@id",
],
"link" => [
"@type" => "xsd:anyURI",
],
"list_float" => [
"@type" => "xsd:float",
"@container" => "@list",
],
"list_integer" => [
"@type" => "xsd:int",
"@container" => "@list",
],
"list_string" => [
"@type" => "xsd:string",
"@container" => "@list",
],
"path" => [
"@type" => "xsd:anyURI",
],
"text" => [
"@type" => "xsd:string",
],
"text_with_summary" => [
"@type" => "xsd:string",
],
"text_long" => [
"@type" => "xsd:string",
],
"uuid" => [
"@type" => "xsd:string",
],
"uri" => [
"@type" => "xsd:anyURI",
],
"language" => [
"@type" => "xsd:language",
],
"string_long" => [
"@type" => "xsd:string",
],
"changed" => [
"@type" => "xsd:dateTime",
],
"map" => "xsd:",
"boolean" => [
"@type" => "xsd:boolean",
],
"email" => [
"@type" => "xsd:string",
],
"integer" => [
"@type" => "xsd:int",
],
"decimal" => [
"@type" => "xsd:decimal",
],
"created" => [
"@type" => "xsd:dateTime",
],
"float" => [
"@type" => "xsd:float",
],
"entity_reference" => [
"@type" => "@id",
],
"timestamp" => [
"@type" => "xsd:dateTime",
],
"string" => [
"@type" => "xsd:string",
],
"password" => [
"@type" => "xsd:string",
],
];
}
93 changes: 6 additions & 87 deletions src/ContextGenerator/JsonldContextGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ class JsonldContextGenerator implements JsonldContextGeneratorInterface {
*/
const CACHE_BASE_CID = 'jsonld:context';

/**
* Constant hook alter name
*/
const FIELD_TYPE_ALTER_HOOK = 'jsonld_alter_field_mappings';


/**
* Injected EntityFieldManager.
Expand Down Expand Up @@ -317,93 +322,7 @@ protected function getTermContextFromField($field_type) {
"@type" => "xsd:string",
];

$field_mappings = [
"comment" => [
"@type" => "xsd:string",
],
"datetime" => [
"@type" => "xsd:dateTime",
],
"file" => [
"@type" => "@id",
],
"image" => [
"@type" => "@id",
],
"link" => [
"@type" => "xsd:anyURI",
],
"list_float" => [
"@type" => "xsd:float",
"@container" => "@list",
],
"list_integer" => [
"@type" => "xsd:int",
"@container" => "@list",
],
"list_string" => [
"@type" => "xsd:string",
"@container" => "@list",
],
"path" => [
"@type" => "xsd:anyURI",
],
"text" => [
"@type" => "xsd:string",
],
"text_with_summary" => [
"@type" => "xsd:string",
],
"text_long" => [
"@type" => "xsd:string",
],
"uuid" => [
"@type" => "xsd:string",
],
"uri" => [
"@type" => "xsd:anyURI",
],
"language" => [
"@type" => "xsd:language",
],
"string_long" => [
"@type" => "xsd:string",
],
"changed" => [
"@type" => "xsd:dateTime",
],
"map" => "xsd:",
"boolean" => [
"@type" => "xsd:boolean",
],
"email" => [
"@type" => "xsd:string",
],
"integer" => [
"@type" => "xsd:int",
],
"decimal" => [
"@type" => "xsd:decimal",
],
"created" => [
"@type" => "xsd:dateTime",
],
"float" => [
"@type" => "xsd:float",
],
"entity_reference" => [
"@type" => "@id",
],
"timestamp" => [
"@type" => "xsd:dateTime",
],
"string" => [
"@type" => "xsd:string",
],
"password" => [
"@type" => "xsd:string",
],
];
$field_mappings = \Drupal::moduleHandler()->invokeAll('jsonld_alter_field_mappings', []);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few tiny comments here, could the hook be named jsonld_field_mappings_alter instead? Its a bit more D8 expected that way.
Also, your hook has no defined argument for the function. Is that intended? I would assume that it should take at least a previous mapping to allow some conditional logic to happen.

Lastly, maybe for a new pull. If altering the mapping is something people will need, then maybe what is passed around should be the actual field definition, and not only the type: reasons are many, but imagine your field has many properties, or its definition is of DataType Map or List, then maybe the contributed hooks would like to look inside deeper and fetch each properties datatype.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree with @DiegoPino that you need this module to invoke its own hook with the old defaults, much like you did in the jsonld.api.php but that is not invoked so you need to add it to jsonld.module for example.

Also you'll need a way to handle array merges if two modules invoke this hook. What do you end up with, this could be as simple as a straight overwrite (last one wins) or some weighting to choose order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not wedded to the hook name, I chose it because it paralleled the existing hook (hook_jsonld_alter_normalized_array). But if ya'll insist, I'm happy to change it.

Yes, it was intended not to take any arguments. It is patterned after the rdf module's hook_rdf_namespaces which also doesn't take any arguments.

Passing around field definitions is certainly an interesting idea, so I'm open to a larger conversation about it, but if we want to do it, it should be done now. I would rather not declare a new hook in the API now if we plan to change it later.

That stated, I'm not sure how passing the definition instead of a mapping would be helpful. We make the mapping in the module because (I presumed) the existing definitions aren' sufficient to the task of setting the corresponding value's @type. We could use the field definition to set and get a mapping configuration, but we would still need a default mapping in another form somewhere to set them on install. If a developer wants to do something with field definitions it would be better to do it in hook_jsonld_alter_normalized_array where they can easily pull the field definitions they want there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seth-shaw-unlv the Symfony Typed data implementation allows for some complex data types that don't have a 1:1 mapping, as stated in my function comment (the one you copied over to the hook) (look inside drupal/core/TypedData). Right now we are only dealing with native types (mostly.. we do go into entity references.. at least a bit) but if you are planning on full D8 integration, those other ones need to be addressed (Complex, map, list). I agree there are some gray points there since we are only mapping::getValue() value so not even sure with current implementation how what I propose even makes sense, we are not traversing into the field's properties, true. But in many fields, you will see that what can be indexed in Solr will totally differ from the JSON-LD/mapped representation. That said, I'm ok with not going that route, for now, to be honest, its difficult to assume this won't change anymore, that is a lot to ask for! But you do need to pass the mapping array around and establish a basic priority strategy as correctly @whikloj stated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whikloj:

  1. Ah, I missed that, sorry. Commit coming.
  2. the invokeAll has array merging built in: "If modules return arrays from their implementations, those are merged into one array recursively... using array_merge_recursive". Although we could do something more complex that issues warnings if conflicts are found.
  3. What do you think of @DiegoPino's idea of passing around field definitions instead of the mappings via arrays?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seth-shaw-unlv I'm not against @DiegoPino idea, I'm not sure I fully understand it either. As for merging the array recursively, that should work (we'll have to test) such that new types just get added. We might however want to do some deduplication so if multiple modules say "this is a xsd:dateTime" we don't end up with 12 @types of xsd:dateTime

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what the benefits of passing around the mapping array would be, compared to this existing strategy of requesting updates from the other modules and handling the merging internally. The only reason I can think of to do that would be if you wanted to remove existing mappings (rather than update) which seems to me a bad idea we don't want to enable.

Even updating the mappings is suspect because you can't control the order in which implementations are invoked, so prioritization strategies based on the order of hook invocation (either prioritizing first of last) will give unpredictable results.

@whikloj, the array_merge_recursive should take care of that deduplication (the structure only allows a 1:1 mapping since the @type key is mapped to a string rather than an array), but I am inclined to update the module to issue warnings (or at least notices) as I suggested earlier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a simple array_merge_recursive test with PHP 7.1.20 and it left duplicates in and changed the @type to reference an array with multiple strings inside.

  'datetime' =>
  array(1) {
    '@type' =>
    array(2) {
      [0] =>
      string(12) "xsd:dateTime"
      [1] =>
      string(12) "xsd:dateTime"
    }
  }

Perhaps Drupal has a deduplication feature, either way we should try it and see what it looks like in json-ld format.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whikloj, ah, I see you are right. This next commit that ignores duplicates and adds warnings fixes that. (I'm going to test it a bit first.)


return array_key_exists($field_type, $field_mappings) ? $field_mappings[$field_type] : $default_mapping;

Expand Down