add hook_jsonld_alter_field_mappings

[seth-shaw-unlv]

GitHub Issue: Issue 916

What does this Pull Request do?

Creates a new hook, hook_jsonld_alter_field_mappings, allowing modules to add mappings for custom fieldtypes.

What's new?

A hook was added and the default fieldtype mappings were moved into the module's own implementation of the hook.

How should this be tested?

• The module tests should run without errors. (Because I moved the fieldtype mappings to a hook implementation, the testJsonldcontextResponseIsValid test completing successfully shows that it works.)
• To really give it a run through:
  • Create a new module with a custom fieldtype
  • implement the new hook mapping the fieldtype
  • Add the field to a content type, give it an RDF mapping, and enable the json-ld serialization for that type
  • Create a new node/taxonomy_term using the new field
  • View the JSON-LD serialization for that node and make sure the mapping set by the hook is there.

Additional Notes:

This doesn't actually resolve issue 916, which is a bit more complex. (I have a bit more testing to do before I issue the controlled_access_terms PR to fully resolve it.) However, supporting the serialization of custom fieldtypes via this method is still a good idea.

Interested parties

@Islandora-CLAW/committers (esp @whikloj)

[DiegoPino]

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.

[whikloj]

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.

[seth-shaw-unlv]

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.

[DiegoPino]

@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

[seth-shaw-unlv]

@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?

[whikloj]

@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

[seth-shaw-unlv]

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.

[whikloj]

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.

[seth-shaw-unlv]

@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.)

[DiegoPino]

@seth-shaw-unlv I'm still a bit confused by this pull. Where does $default_mapping come from/assigned? Also, to be honest, i would really appreciate if the hook ends in _alter, even when hooks are "slooowly" moving away from Drupal8 (but still on D9) the common semantic of other module´s naming strategy for hooks is pretty well defined in the collective consciousness. Having the _alter as a suffix instead of in the middle can help read them better and even de-confuse use cases where your module is named alter somehow. (e.g stateofmind_alter) but would allow also other similar, but not "altering" hooks to be defined under the same prefix, module name, etc, structure.

[seth-shaw-unlv]

@DiegoPino

1. $default_mapping is in the pre-existing version of the getTermContextField function. It is there to say "if you asked for a field type mapping and we don't have it, map the type to xsd:string".
2. Okay. I'll issue an update with the change in a minute.

[DiegoPino]

@seth-shaw-unlv @whikloj sorry for keeping this open, but i still think i have some questions.
So the alter hook (thanks for chaning the name!) is right now a blind hook. It is not aware of any other mappings altered or provided by other hooks, and it does not know about the field type if needs to map. Not sure why that is? Again, would it not be desirable that the mapping alter hooks have access to any previous array and at least the fieldtype? Lets say i want to have alter hook for a custom field, that conditionally on the format of the value (your EDTF!) and the type of the field, can decide on what mapping makes more sense?
I feel right now, it does solve your use case (simply changing the mapping for a well known field type ) but it lacks the ability to implement any conditional logic. Any other @Islandora-CLAW/committers feels the same or am i understanding (totally possible) the use case and this pull incorrectly?

[seth-shaw-unlv]

@DiegoPino I modeled the hook on the RDF module's hook_rdf_namespaces() which works this way. I figured if it was a good enough strategy for that it was good enough for this.

A module is perfectly capable of creating some logic to determine how it wants to setup a field type mapping before returning it. However, as a module author I wouldn't want to base my logic on a mapping passed to me because I couldn't be sure of what other modules may have done to it already or what other modules may do to it later. I struggle to see what a mapping of other field types would be useful for (especially since there is a chance it would still be empty, if your implementation is the first to run). As a module author I would be more inclined to perform conditional-based mapping updates in a hook_jsonld_alter_normalized_array implementation (the current plan for EDTF) where I have a lot more information available to me, including field values.

Also, passing the existing mapping implies that they will return the whole mapping plus their additions, which could complicate the merging logic. (The current logic would throw a host of superfluous warnings in that case.)

So, in short, I'm not convinced it is worth a rewrite. That stated, I will do it if you still insist.

[DiegoPino]

@seth-shaw-unlvI get that. I think I would need other people's opinion, probably my uses cases are not even valid in this context. let me wait for a few hours if no feedback I will assume everyone is Ok with the current state of this. In your use case (can you share the code) what argument you pass to the hook if in API definition the function signature does not include any arguments? Are you using the class/object context?

[seth-shaw-unlv]

@DiegoPino sounds like a plan.

As for EDTF, I've gone through a number of iterations. Initially we were going to simply use

function controlled_access_terms_jsonld_field_mappings_alter() {
  return [
    "edtf" => [
      "@type" => "xsd:datetime",
    ],
  ];
}

and then in the RDF mapping use the datatype_callback setting to have a converter transform the EDTF into a valid xs:datetime.

However, the specific EDTF usecase has evolved since then. Because I need the additional logic based on the actual EDTF value to select between types (date, gYear, and gYearMonth) I'm now going to do it in controlled_access_terms_jsonld_alter_normalized_array. I wouldn't have been able to do it in hook_jsonld_field_mappings_alter even if I had received the other mappings, because it doesn't have field values to base the logic on.

Even though I don't need the hook for EDTF anymore, I figured it would still be useful for others.

[dannylamb]

@seth-shaw-unlv My take on this is it's a hook implemented as an alter, which looks a little odd. I tend to relate alters with passing by reference. What I think we need right now is just a hook. One that takes no arguments, and the functions return arrays that get combined as you're already doing. Modules can use that to build up the list of field mappings. Then later, when someone needs to do some surgery, we can add an alter and pass the array around.

[dannylamb]

In other words, drop the alter from the function name and I'm 👍 on this.

[seth-shaw-unlv]

@dannylamb, I see what you mean. @DiegoPino, if you are fine with that I'll make the update to change hook_jsonld_field_mappings_alter to hook_jsonld_field_mappings.

[DiegoPino]

Yeah, go for it El El mar, 18 de dic. de 2018 a las 17:03, Seth Shaw < notifications@github.com> escribió:

@dannylamb <https://github.com/dannylamb>, I see what you mean. @DiegoPino <https://github.com/DiegoPino>, if you are fine with that I'll make the update to change hook_jsonld_field_mappings_alter to hook_jsonld_field_mappings. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGn85z2qdu0yTsCMnx9H0VlpgTCgCIe8ks5u6WYngaJpZM4ZR7Y1> .

-- Diego Pino Navarro Digital Repositories Developer Metropolitan New York Library Council (METRO)

[DiegoPino]

ups, you missed one- Should be const FIELD_TYPE_HOOK ?

[seth-shaw-unlv]

Ha, yeah. Oops. I was so focused on the actual hook name that I neglected the const. Just a moment. Doing too many things at once.

[DiegoPino]

No worries, no rush, will approve and merge once that gets in. Thanks

Changes now comply with the actual use case

[DiegoPino]

Ok, new changes inform the use case. I'm OK with this!

[DiegoPino]

@seth-shaw-unlv good. @Islandora-CLAW/committers do we have 24 hours waiting time here?

[dannylamb]

Nope. Merge away @DiegoPino