search

WebDevStudios / Automatic-Featured-Images-from-Videos

If a YouTube or Vimeo video exists in the first few paragraphs of a post, automatically set the post's featured image to that vidoe's thumbnail.
33 stars 24 forks source link

major refactoring to use video provider #54

Closed binarygary closed 3 years ago

binarygary commented 6 years ago

@tw2113 we talked about this a good long while ago...I was feeling crazy tonight so I slapped this together for a quick proof of concept. Looking for feedback/comments on architecture specifically. https://github.com/WebDevStudios/Automatic-Featured-Images-from-Videos/issues/49

It's kind of a weird spot autoloading...wasn't sure where to include the new files. I opted for the main plugin file for now. Obviously, the Providers_Bootstrap will need to be changed a bit to allow for extension.

(I initially was working in the WDS repo but didn't have permission to push a new branch, hence the fork. Happy to work there if it's easier - though you'll have to create the branch apparently.)

tw2113 commented 6 years ago

behold the power of moments of boredom and some initiative. Looking over and will provide feedback. I also need to refresh my memory for what I was thinking when I first started those tickets, so we can stay on course.

tw2113 commented 6 years ago

Definitely a good start, even without me having looked over my issue notes. I'll go through that a bit later and then re-review once more.

I had worry about order of method use, but i finally understood that once the match_content method runs, which is early, the rest would be set up already for each provider.

binarygary commented 6 years ago

Thanks for looking at this. I was throwing something at the wall to see if it was inline with your original suggestion. I like the idea of a plugin using hooks with some OOP concepts. I also like the idea of Provider_Bootstrap as a collection of providers, but it is decidedly different than the structure you suggested.

tw2113 commented 6 years ago

Keep in mind that my comments earlier were without me reviewing original ideas yet. It was all based on code presented :D

binarygary commented 6 years ago

I've no idea what you mean by that... :joy: Hopefully, this advances us in some direction...

tw2113 commented 6 years ago

Thoughts? https://stackoverflow.com/questions/1814821/interface-or-an-abstract-class-which-one-to-use

tw2113 commented 6 years ago

@binarygary Looked over my original notes, and I'm not married to them. I feel what you have is pretty solid and am ready to merge once we collab and decide on the 3 comments from myself earlier tonight. From the abstract class vs interface detail, to the supported PHP versions.

Thoughts?

binarygary commented 6 years ago

I started Video_Provider as an abstract because I thought I'd end up adding more logic there prior to creating the Provider_Bootstrap class. At this point, it's little more than an interface. I'll refactor. I also need to add properties to the Vimeo class. I think the last thing I'll want to do is comment the methods?

My only thought on php versioning...this seems like a pretty safe platform to encourage a newer php version without causing much disruption and/or customer feedback.

binarygary commented 6 years ago

@tw2113 I removed the array shorthand so 5.3 is back in play. I did NOT replace the anonymous functions hooked to wds_featured_images_from_video_providers. What else needs to be sorted before this is merged?

tw2113 commented 6 years ago

Thanks.

Probably just some renewed focus on further developing this before it gets merged in, truth be told.

ahmader commented 5 years ago

Hello @tw2113 is this plugin going to continue from this Pull ? I want to do some enhancement and I want to know which fork to use. Thanks

tw2113 commented 5 years ago

I'd say go with the official release branch(es) for the time being, unless your ideas rely on what this would be doing.