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

Include video durations? #74

Open
bstein-clever opened this issue Jul 10, 2018 · 8 comments
Open

Include video durations? #74

bstein-clever opened this issue Jul 10, 2018 · 8 comments

Comments

@bstein-clever
Copy link

bstein-clever commented Jul 10, 2018

I'm a huge fan of your neo plugin, and just discovered this one, which looks just like something we were needing for youtube videos.

How hard would it be to add a duration value for the cached video data?
I'm assuming (but haven't tested) that it would be possible to update cache info by doing a cache update, but doing so for an individual entry would be pretty cool.

One extra wrinkle: our site is still on Craft 2 (we use neo heavily), so I know we're not on the lastest version of the plugin.

@beneklisefski
Copy link

+1 for retrieving video duration

@johndwells
Copy link
Contributor

+1

@johndwells
Copy link
Contributor

@ttempleton Would you be interested in a PR for this? I could definitely add support for Vimeo and Youtube. Vimeo returns duration in the oembed response so it would be easy to grab. Youtube's duration is deep within the html provider response, so that could be scraped out. I'm not familiar with other video platforms though (pbs video for example).

@ttempleton
Copy link
Contributor

@johndwells We only really support YouTube and Vimeo for the other video-specific embedded asset methods, so it'd be fine if video durations only supported those too. But if you'd like to work on a PR for adding video durations, then we'd definitely merge that.

@johndwells
Copy link
Contributor

johndwells commented Oct 19, 2020 via email

@ttempleton
Copy link
Contributor

I don't necessarily have a problem with adding functionality that isn't directly provided by Embed. Having thought about it a bit more, though, I do agree with the slippery slope comment in that we shouldn't keep adding embedded asset functionality that's only relevant for certain types of embedded assets or for certain providers. If there were event hooks to allow extending to include video durations or other video-specific information, that seems like a good solution to that situation.

@johndwells
Copy link
Contributor

@ttempleton I've had a play with creating an event which allows me to append arbitrary additional information (in this case, duration) to the array returned by the adapter. But then encountered 2 hurdles:

  1. Any attributes not supported on the EmbeddedAsset model causes the entire model to not be instantiated (https://github.com/spicywebau/craft-embedded-assets/blob/master/src/Service.php#L223-L225).

  2. What is stored on the filesystem is a serialized json object representing the EmbeddedAsset model, not the array created from the adapter response and used to instantiate the model.

This means that even if #1 is addressed to quietly discard/skip any non-supported properties, the custom array values are entirely lost once the EmbeddedAsset is serialized and stored.

I can think of one way to work around this, but it would be significant: allow plugins to provide their own EmbeddedAsset model. This would entail making an EmbeddedAssetInterface, allowing plugins to register their own, etc... quite convoluted, possibly riddled with pitfalls, though it would be the most flexible down the road.

A simpler solution might be to introduce an "extras" attribute on the EmbeddedAsset model and allow that to hold arbitrary information that may be added by 3rd party plugins.

Interested to hear your thoughts. To be honest, I may end up having to bail on this and resort fo my own oembed fetches, since I'm doing far more with the duration than merely displaying it. So even getting Embedded Assets to store the duration only solves one part of my puzzle. So what I suppose I'm saying is, if you feel like this ticket is just generally going down a road you don't want to travel, that's OK. I'll find another route to solving my particular issues.

Thanks!

@ttempleton
Copy link
Contributor

@johndwells I think the extras attribute is an interesting idea and would be a good solution that we could implement in the shorter term, I'll have a look at that in the next couple of weeks.

I do like the idea of providing an interface, and in addition to allowing plugins to provide their own models for their own requirements, I could see us then providing an Embedded Video model which could have the specialist functionality like video ID/duration/etc. I definitely want to work on the extras attribute first, though, but it's an idea that I'd like to try out in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants