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

[FEATURE] Improve handling of requests where Media lacks an associated File #1059

Open
xurizaemon opened this issue Oct 7, 2024 · 2 comments

Comments

@xurizaemon
Copy link

xurizaemon commented Oct 7, 2024

What steps does it take to reproduce the issue?

  • Create a Media entity which has no File attached
  • Request /media/123456?_format=jsonld for that file
  • Witness error below
The website encountered an unexpected error. Try again later.

TypeError: Drupal\islandora\IslandoraUtils::getDownloadUrl(): Argument #1 ($file) must be of type Drupal\file\FileInterface, bool given, called in /app/web/modules/contrib/islandora/src/Plugin/ContextReaction/JsonldSelfReferenceReaction.php on line 94 in <em class="placeholder">Drupal\islandora\IslandoraUtils-&gt;getDownloadUrl()</em> (line <em class="placeholder">641</em> of <em class="placeholder">modules/contrib/islandora/src/IslandoraUtils.php</em>). <pre class="backtrace">Drupal\islandora\Plugin\ContextReaction\JsonldSelfReferenceReaction-&gt;execute(Object, Array, Array) (Line: 254)
islandora_jsonld_alter_normalized_array(Object, Array, Array)
call_user_func_array(Object, Array) (Line: 409)
Drupal\Core\Extension\ModuleHandler-&gt;Drupal\Core\Extension\{closure}(Object, &#039;islandora&#039;) (Line: 388)
Drupal\Core\Extension\ModuleHandler-&gt;invokeAllWith(&#039;jsonld_alter_normalized_array&#039;, Object) (Line: 416)
Drupal\Core\Extension\ModuleHandler-&gt;invokeAll(&#039;jsonld_alter_normalized_array&#039;, Array) (Line: 189)
Drupal\jsonld\Normalizer\ContentEntityNormalizer-&gt;normalize(Object, &#039;jsonld&#039;, Array) (Line: 159)
Symfony\Component\Serializer\Serializer-&gt;normalize(Object, &#039;jsonld&#039;, Array) (Line: 138)
Symfony\Component\Serializer\Serializer-&gt;serialize(Object, &#039;jsonld&#039;, Array) (Line: 159)
Drupal\rest\EventSubscriber\ResourceResponseSubscriber-&gt;renderResponseBody(Object, Object, Object, &#039;jsonld&#039;) (Line: 75)
Drupal\rest\EventSubscriber\ResourceResponseSubscriber-&gt;onResponse(Object, &#039;kernel.response&#039;, Object)
call_user_func(Array, Object, &#039;kernel.response&#039;, Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher-&gt;dispatch(Object, &#039;kernel.response&#039;) (Line: 214)
Symfony\Component\HttpKernel\HttpKernel-&gt;filterResponse(Object, Object, 1) (Line: 202)
Symfony\Component\HttpKernel\HttpKernel-&gt;handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel-&gt;handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session-&gt;handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle-&gt;handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength-&gt;handle(Object, 1, 1) (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength-&gt;handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache-&gt;pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache-&gt;handle(Object, 1, 1) (Line: 50)
Drupal\ban\BanMiddleware-&gt;handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware-&gt;handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware-&gt;handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState-&gt;handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel-&gt;handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel-&gt;handle(Object) (Line: 19)
  • When does this issue occur?

Occurs when content is in an invalid state, so this is a feature request.

  • Which page does it occur on?

/media/%mid?_format=jsonld

  • What happens?

Error is output. This URL is requested internally from Drupal via islandora_jsonld_alter_normalized_array() so the error is typically not visible to end users. The error is visible in Drupal's logs if you look for it.

  • To whom does it occur (anonymous visitor, editor, administrator)?

Any user whose browsing triggers the internal request to the /media/%mid?_format=jsonld request.

  • What did you expect to happen?

Suggest either of:

  1. Throw a NotFoundHttpException from \Drupal\islandora\MediaSource\MediaSourceService::getSourceFile if no file is returned from $source_list = $media->get($source_field). This function already throws such an error if the field is not present (a configuration error), and could throw a similar error for this content error.
  2. Check the result of $this->mediaSource->getSourceFile($entity); in \Drupal\islandora\Plugin\ContextReaction\JsonldSelfReferenceReaction::execute and either omit the file data from the JSON response, or throw an error for missing required data.

Which version of Islandora are you using?

composer show drupal/islandora reports 2.12.3

Any related open or closed issues to this bug report?

These look related:

Screenshots:

@xurizaemon
Copy link
Author

xurizaemon commented Oct 7, 2024

::getSourceFile() says it returns \Drupal\file\FileInterface|\Drupal\Core\Entity\EntityInterface|false|null

The first source entity if there is one, generally expected to be of \Drupal\file\FileInterface. Boolean FALSE if there was no such entity. NULL if the source field does not refer to Drupal entities (as in, the field is not a \Drupal\Core\Field\EntityReferenceFieldItemListInterface implementation).

Which is a lot, and based on that I'd probably be inclined to raise the error from the calling method, the context execute function, when the "file" fetched is NULL or FALSE.

Would it be valid to omit @id from the JSON response, or use the media URL, if a file URL cannot be obtained?

// Swap media and file urls.
if ($entity instanceof MediaInterface) {
$file = $this->mediaSource->getSourceFile($entity);
$graph['@id'] = $this->utils->getDownloadUrl($file);
}

could become:

            // Swap media and file urls.
            if ($entity instanceof MediaInterface) {
              $file = $this->mediaSource->getSourceFile($entity);
              if ($file) {
                $graph['@id'] = $this->utils->getDownloadUrl($file);
              }
            }

With this change, we end up with the source Media (not File) URL in the same place.

I am happy to PR such a change, and go looking for similar in other context reactions, but I'm opening this issue to get a steer on how to approach first.

@seth-shaw-asu
Copy link
Member

@xurizaemon, that seems like a reasonable approach. Please go ahead and submit a PR.

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

No branches or pull requests

2 participants