major refactoring to use video provider #54
Conversation
|
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. |
| $video_thumbnail_url = ''; | ||
| $providers = new Provider_Bootstrap(); | ||
| $providers->add_provider( new Youtube() ); | ||
| $providers->add_provider( new Vimeo() ); |
There was a problem hiding this comment.
You have a WP_Error return possible for this method, but don't do anything with that potential return value.
There was a problem hiding this comment.
Yeah...not sure what to do with that. We don't usually raise exceptions in WP? Do you have thoughts, preferences?
includes/Provider_Bootstrap.php
Outdated
| * @param Video_Provider $value An object that is of a class that extends Video_Provider. | ||
| */ | ||
| return apply_filters( 'wds_video_providers', $this->providers ); | ||
| } |
There was a problem hiding this comment.
Feels like we have a slight disconnect with how to add providers. We get to do so with the add_provider method, everyone else would need to use the filter. Is there a way we could work things to be the same routine for everyone? Perhaps everyone uses the filter version.
There was a problem hiding this comment.
Totally fair point. I've refactored based on this note.
Given we have an add_provider method, I've added a remove_provider method. I've also updated the provider classes as appropriate.
|
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. |
|
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. |
|
Keep in mind that my comments earlier were without me reviewing original ideas yet. It was all based on code presented :D |
|
I've no idea what you mean by that... 😂 |
|
|
||
| add_action( 'wds_featured_images_from_video_providers', function( $providers ) { | ||
| $providers->add_provider( 'vimeo', new Vimeo() ); | ||
| }, 9, 1 ); |
There was a problem hiding this comment.
I find it tempting to do this and make the minimum PHP version become 5.3. Very tempting.
There was a problem hiding this comment.
Yeah, I'll leave the php versioning decision up to you. One of the ongoing concerns with anonymous functions and actions/filters is that they can't be unhooked.
In this case, I like the approach given we have a remove_provider method available so there is no need to unhook.
includes/Provider_Bootstrap.php
Outdated
|
|
||
| class Provider_Bootstrap { | ||
|
|
||
| protected $providers = []; |
There was a problem hiding this comment.
Definitely willing to drop 5.2, but this array shorthand would necessitate dropping 5.3 as well. Also tempting, especially with this plugin not being one of our more popular ones, even with useful utility.
There was a problem hiding this comment.
Just a habit...let me know where you want to go with versioning and I'll adjust accordingly.
|
@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? |
|
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. |
|
@tw2113 I removed the array shorthand so 5.3 is back in play. |
|
Thanks. Probably just some renewed focus on further developing this before it gets merged in, truth be told. |
|
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. |
|
I'd say go with the official release branch(es) for the time being, unless your ideas rely on what this would be doing. |
@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.
#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.)