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

Allow use of remote video media #48

Closed
wants to merge 4 commits into from

Conversation

wgilling
Copy link
Contributor

@wgilling wgilling commented Jul 6, 2021

This addresses the issue: #47

This issue only happens when the media type of "remote_video" is being used. In order to set this up, as admin navigate to "Add media type": http://localhost:8000/admin/structure/media/add -- NOTE: this media type will also need to have the two fields as are configured for all other islandora media types - these are "media_use" (a taxonomy entity reference) and "media_of" (entity reference).

The error was occurring when the riprap code attempted to call getSourceFile on an items' media when that item uses remote_video media type since there is not "file" underlying a remote_video media.

To reproduce the error,

  1. configure Islandora to have a media type of remote_video and add/edit a test item so that it includes a media that is using "Remote video"
  2. observe the error on the item/{nid}/media page

To test the patched code:

  1. follow steps above - but also pull in the code from this branch, clear the cache.
  2. observe no error entry when viewing an item's media page that is using remote_video

@mjordan
Copy link
Owner

mjordan commented Jul 6, 2021

@wgilling thanks for finding this. I will test and merge this evening.

@@ -71,8 +71,13 @@ public function getFileUuid($mid) {
public function getFedoraUrl($mid) {
$media = Media::load($mid);
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason we can't put the following check here, right after loading the media?

if ($media->bundle() == 'remote_video') {
   return False
}

It's a bit simpler to read, and returns before executing the next two lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do -- very soon I will refactor this method to handle the issue I was seeing.

@wgilling wgilling force-pushed the remote_video_media branch from 1b00b47 to 93d6727 Compare July 8, 2021 15:47
@mjordan
Copy link
Owner

mjordan commented Jul 9, 2021

@wgilling I just merged in the issue-46 branch into main which has now caused merge conflicts with this PR. Do you mind pulling in the updates to main and resolving the conflicts? If so I can do it and you can continue from there. Let me know.

@wgilling
Copy link
Contributor Author

wgilling commented Jul 9, 2021

too much changed -- made a new branch remote_video_media_noconflict and will issue a pull request there.

@wgilling wgilling closed this Jul 9, 2021
@mjordan
Copy link
Owner

mjordan commented Jul 9, 2021

OK, thanks. Sorry about that.

@wgilling
Copy link
Contributor Author

wgilling commented Jul 9, 2021

OK, thanks. Sorry about that.

NO WORRIES. My bad -- I didn't realize that it wasn't based off of latest main code (skipped that all-important git pull command!). :)

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