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.

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 optional pass_target_id argument to #68

Merged
merged 3 commits into from
Nov 24, 2022
Merged

Add optional pass_target_id argument to #68

merged 3 commits into from
Nov 24, 2022

Conversation

seth-shaw-asu
Copy link
Member

GitHub Issue: asulibraries/islandora-repo#549

What does this Pull Request do?

Adds an optional 'pass_target_id' parameter to the EntityReferenceConverter::linkFieldPassthrough that allows us to pass through the target ID OR an empty string if it couldn't load the target ID's taxonomy term. This prevents errors from popping up in the watchdog for rest_oai_pmh and JSON-LD when an invalid target shows up.

What's new?

  • Adds an optional 'pass_target_id' parameter to the EntityReferenceConverter::linkFieldPassthrough that allows us to pass through the target ID OR an empty string if it couldn't load the target ID's taxonomy term.
  • Does this change require documentation to be updated? No
  • Does this change add any new dependencies? No
  • Does this change require any other modifications to be made to the repository
    (i.e. Regeneration activity, etc.)? No
  • Could this change impact execution of existing code? No

How should this be tested?

A description of what steps someone could take to:

  • Make or reuse a taxonomy with URIs
  • Update the RDF mapping to use the linkFieldPassthrough converter, e.g.
  field_standard_rights:
    properties:
      - 'dcterms:rights'
    datatype_callback:
      callable: 'Drupal\jsonld\EntityReferenceConverter::linkFieldPassthrough'
      arguments:
        link_field: field_source
  • Make a node with an entity reference for this field that is invalid. This is easiest to do using drush php:cli to load an existing node and update the field, e.g.:
$node = \Drupal::entityTypeManager()->getStorage('node')->load(3);
$node->field_standard_rights[] = ['target_id'=> 0];
$node->save();
  • View the JSON-LD or OAI-PMH endpoint
  • Now check your logs and you should see an error complaining about 'target_id'.
  • Apply the patch
  • Clear cache
  • Reload either the JSON-LD or OAI-PMH feed.
  • Check the logs, there shouldn't be any new errors.
  • Even further: update your RDF mapping to pass the target_id, e.g.:
  field_standard_rights:
    properties:
      - 'dcterms:rights'
    datatype_callback:
      callable: 'Drupal\jsonld\EntityReferenceConverter::linkFieldPassthrough'
      arguments:
        link_field: field_source
        pass_target_id: true
  • Clear cache and view the item again. Now the invalid target_id will show up as the value for the configured field.

Interested parties

@Islandora/8-x-committers

@whikloj
Copy link
Member

whikloj commented Aug 15, 2022

Is there anywhere we are expecting the entity returned if it can't be converted?

@seth-shaw-asu
Copy link
Member Author

Is there anywhere we are expecting the entity returned if it can't be converted?

Not that I'm aware of.

@whikloj
Copy link
Member

whikloj commented Nov 23, 2022

@seth-shaw-asu how exactly does this error show up without defining the linkFieldPassThrough in the RDF mapping? I tried to use the instructions before adding that section and I do not see an error appearing. Just trying to see the original issue.

@seth-shaw-asu
Copy link
Member Author

The error appears due to a bug with linkFieldPassThrough, so if you don't have it the error won't appear. This fix is adding the additional parameter to return a valid value if the original linkFieldPassThrough code fails.

@whikloj
Copy link
Member

whikloj commented Nov 23, 2022

🤦 for some reason I saw that as new code and not a change, mea culpa.

@whikloj
Copy link
Member

whikloj commented Nov 23, 2022

I'm using the playbook and field_standard_rights does not exist on the Repository Item. Is this custom or is it new and I have to use ISLE to test this?

@seth-shaw-asu
Copy link
Member Author

It was custom. It doesn't really matter what taxonomy and reference field you use, as long as the target taxonomy has a Link/URI field.

For example, you could test this with Media using the Media Use field; you would just configure the RDF mapping for the media to use the passthrough plugin on the Media Use field and add the field_external_uri as the field parameter:

  field_media_use:
    properties:
      - 'pcdm:use'
    datatype_callback:
      callable: 'Drupal\jsonld\EntityReferenceConverter::linkFieldPassthrough'
      arguments:
        link_field: field_external_uri

Then you would make a Media with an invalid field_media_of reference:

$media = \Drupal::entityTypeManager()->getStorage('media')->load(1); # Or whichever media you are testing with...
$media->field_media_use[] = ['target_id'=> 0];
$media->save();

Then check the Media's JSON-LD. It should throw errors before the patch and work afterward.

Note: I didn't test this example, but all the same principles apply.

@whikloj
Copy link
Member

whikloj commented Nov 23, 2022

Ok, this is not causing errors for me. I added an EntityReference field to islandora_object called 'field_standard_rights'. I linked it to a taxonomy that has a field_external_uri. I added a new object and added a "right" linked to a taxonomy term.

I altered the RDF mapping to add this section

field_standard_rights:
    properties:
      - 'dcterms:rights'
    datatype_callback:
      callable: 'Drupal\jsonld\EntityReferenceConverter::linkFieldPassthrough'
      arguments:
        link_field: field_external_uri
    mapping_type: rel

When I go into drush php:cli:

$node = \Drupal::entityTypeManager()->getStorage('node')->load(1);
=> Drupal\node\Entity\Node {#8454
     +in_preview: null,
     #nid: Drupal\Core\Field\FieldItemList {#8015},
 ...
     #field_weight: Drupal\Core\Field\FieldItemList {#8278},
   }
>>> $node->field_standard_rights[0]->getValue();
=> [
     "target_id" => "36",
   ]

I then altered it

>>> $node->field_standard_rights[0]->setValue(0);
=> null
>>> $node->save();
<warning>PHP Deprecated:  Calling the Symfony\Component\EventDispatcher\EventDispatcherInterface::dispatch() method with a string event name as the first argument is deprecated in drupal:9.1.0, an Event object will be required instead in drupal:10.0.0. See https://www.drupal.org/node/3154407 in /var/www/html/drupal/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php on line 116</warning>
<warning>PHP Deprecated:  Calling the Symfony\Component\EventDispatcher\EventDispatcherInterface::dispatch() method with a string event name as the first argument is deprecated in drupal:9.1.0, an Event object will be required instead in drupal:10.0.0. See https://www.drupal.org/node/3154407 in /var/www/html/drupal/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php on line 116</warning>
<warning>PHP Deprecated:  Calling the Symfony\Component\EventDispatcher\EventDispatcherInterface::dispatch() method with a string event name as the first argument is deprecated in drupal:9.1.0, an Event object will be required instead in drupal:10.0.0. See https://www.drupal.org/node/3154407 in /var/www/html/drupal/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php on line 116</warning>
<warning>PHP Deprecated:  Calling the Symfony\Component\EventDispatcher\EventDispatcherInterface::dispatch() method with a string event name as the first argument is deprecated in drupal:9.1.0, an Event object will be required instead in drupal:10.0.0. See https://www.drupal.org/node/3154407 in /var/www/html/drupal/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php on line 116</warning>
=> 2

But I still don't see any error in my log when I view the node as JSON-LD. 🤷 I can change it back to target_id = 36 and then I see the taxonomy term in the normal View and the URI in the JSON-LD view.

@whikloj
Copy link
Member

whikloj commented Nov 23, 2022

I just tried

>>> $node = \Drupal::entityTypeManager()->getStorage('node')->load(1);
=> Drupal\node\Entity\Node {#5738
     +in_preview: null,
     #nid: Drupal\Core\Field\FieldItemList {#5691},
...
     #field_weight: Drupal\Core\Field\FieldItemList {#8275},
   }
>>> $node->field_standard_rights[0]->getValue();
=> [
     "target_id" => "36",
   ]
>>> $node->field_standard_rights[0]->setValue(["target_id" => "0"]);
=> null
>>> $node->save();
<warning>PHP Deprecated:  Calling the Symfony\Component\EventDispatcher\EventDispatcherInterface::dispatch() method with a string event name as the first argument is deprecated in drupal:9.1.0, an Event object will be required instead in drupal:10.0.0. See https://www.drupal.org/node/3154407 in /var/www/html/drupal/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php on line 116</warning>
<warning>PHP Deprecated:  Calling the Symfony\Component\EventDispatcher\EventDispatcherInterface::dispatch() method with a string event name as the first argument is deprecated in drupal:9.1.0, an Event object will be required instead in drupal:10.0.0. See https://www.drupal.org/node/3154407 in /var/www/html/drupal/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php on line 116</warning>
<warning>PHP Deprecated:  Calling the Symfony\Component\EventDispatcher\EventDispatcherInterface::dispatch() method with a string event name as the first argument is deprecated in drupal:9.1.0, an Event object will be required instead in drupal:10.0.0. See https://www.drupal.org/node/3154407 in /var/www/html/drupal/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php on line 116</warning>
<warning>PHP Deprecated:  Calling the Symfony\Component\EventDispatcher\EventDispatcherInterface::dispatch() method with a string event name as the first argument is deprecated in drupal:9.1.0, an Event object will be required instead in drupal:10.0.0. See https://www.drupal.org/node/3154407 in /var/www/html/drupal/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php on line 116</warning>
=> 2

But that also doesn't seem to cause any errors. I'm definitely missing something, but I don't know what it is.

@seth-shaw-asu
Copy link
Member Author

seth-shaw-asu commented Nov 23, 2022

I went back and tried to replicate the error w/ JSON-LD and I can't either. 😕 I threw a few statements in and discovered that these invalid values aren't being sent to the data_callback at all anymore, although it isn't clear what changed since I encountered this error on our servers. However, the other bit this patch does for JSON-LD is fall back to the referenced entity's label if the URI doesn't exist. To test, reference a term that doesn't have a URI value and you will see the label there instead.

That said, the rest_oai_pmh plugin does still throw errors without this patch when you try to view the record using the RDF Mapping plugin.

Copy link
Member

@whikloj whikloj left a comment

Choose a reason for hiding this comment

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

Works as described on OAI-PMH requests. 👍

@whikloj whikloj merged commit 5db3cd5 into Islandora:2.x Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants