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

Rework plugin to use filters/hooks that enable easy addition of new video providers #49

Open
tw2113 opened this issue Mar 3, 2018 · 10 comments

Comments

@tw2113
Copy link
Member

tw2113 commented Mar 3, 2018

Would need to have a way to provide the following:

Registration of the video service.
Location of video URL
Regex provider to parse out URL specifics like video ID, etc.
Interface/methods to take the found ID/data and make API HTTP requests to provider to return an image, upload, and attach.
Methods to set the various meta data that we will have by the time this feature is started and completed.

The idea is to make it as simple as possible to provide a new video service, and separate out logic/concerns as best possible into individual classes.

@binarygary
Copy link
Contributor

You've got my attention. 👀
I can't tell based on your notes ^ if the intent is for an interface in wp-admin for users to provide this data or a programmatic way (filters that add a new item to a providers array?)
Great concept.

@tw2113
Copy link
Member Author

tw2113 commented Mar 3, 2018

the latter regarding interface. Think how Woo/EDD/etc all allow for easy addition of payment gateways.

@tw2113
Copy link
Member Author

tw2113 commented Mar 3, 2018

@binarygary
Copy link
Contributor

binarygary commented Mar 3, 2018

So we're on the same page, here is my rough understanding.
This is not name-spaced, so in includes we add a contracts dir containing a prefixed interface.
In addition we add 2 classes implementing the contract (youtube, vimeo). These should conform to the standard WP plugin requirements (header, etc) because they should be examples for how to write a stand alone plugin that extends this plugin.
We will create a new filter to add the video provider concrete class to an array of providers.
In both of the previoulsy mentioned concretes we can add the class with the filter...something like:

// Conceivably one could unhook the providers they don't want.
function youtube_video_provider( $providers ) {
    $providers[] = Youtube_Class_Name::class;
    return $providers;
}

That will allow us to check each provider prior to executing the logic. Something along the lines of:

// We get all the providers.
$providers = apply_filters( 'wds_video_providers', array() );

// We iterate over them.
foreach ( $providers as $provider ) {
    $video_interface = new $provider;
    // We verify the provider is actually an instance of the interface.
    if ( ! $video_interface instanceof InterfaceName ) {
        return new WP_Error( 'bad_video_interface', __( sprintf( '%s does not implement InterfaceName', $provider' ), 'wds' ) );
    }

   // Do the things we expect based on the interface. 
}

Do you want to break it down into smaller tasks? I don't know how much time I can put into it, but I'm really excited by the approach.

@tw2113
Copy link
Member Author

tw2113 commented Mar 3, 2018

I'm assuming I'll be doing a lot of the initial legwork, and I definitely need to break it all down into smaller details to help anyone looking better comprehend.

I also wager this will be a bigger overhaul than what has gone on in 1.1.0's changes, which have been mostly minor details, outside of the bulk processing.

@tw2113
Copy link
Member Author

tw2113 commented Mar 4, 2018

I want/would like this as planned out, discussed, debated, and documented ahead of time as we can manage, before any code gets written. That way we can cover all the nooks and crannies and details.

Bit more detail about what provides what:

Base class that video provider extends, interface that provider also implements.

base class: get_video_url() // we can handle. Once we get the url from the content, pass to the appropriate methods.
base class: get_providers() // Filtered list of available providers to iterate over.
base class: process_discovery() // executes everything. Saves meta, uploads to library, attaches as post thumbnail. We will be able to call each appropriate method due to interface.

interface/extended class: video url property // value passed in to constructor.
interface/extended class: get_regex() // just return the regex to use to find in $content
interface/extended class: get_video_id() // use the regex, return the found video id by url passed in to class.
interface/extended class: get_image_url() // receives the video id, returns appropriate image url based on best available.

In the process discovery method, it would probably be a good idea to stop the process once we've found something. Say we have 3 urls all within that character limit. Should we stop after the 1st one? Or let it do all 3 and just have the 3rd one "win". Potential to muck up media libraries with items not used.

@binarygary
Copy link
Contributor

interface/extended class: video url property // value passed in to constructor.
I feel like that can simply be a wrapper to return self::class given that it will already be a unique name. Further, because we'll need to instantiate the class during traversal of the providers the value of the provider class is necessary anyway.

My vote is for stopping the process after the first match for the reasons you mentioned.

I'm a little confused as to architecture based on the first sentence though? Do you have a rough sketch or even directory structure?

@tw2113
Copy link
Member Author

tw2113 commented Mar 6, 2018

I could see myself going either way for a static property vs a getter/setter setup, if not just simply assigning during instantiation.

Which first sentence? The one about planning everything ahead of time? or just the base class part?

My thoughts were that we handle certain parts, the providers handle other parts. Specifically, we handle stitching it all together as much as possible, the providers primary goal is to provide the values/threads, to go with this example. Extending would make sure the instances have a single, consistent method that we control in the parent, while the interface makes sure they've provided the methods for site-specific information.

@tw2113 tw2113 added this to the 1.2.0 milestone Mar 9, 2018
@binarygary
Copy link
Contributor

Which first sentence? The one about planning everything ahead of time? or just the base class part?

Sorry for the ambiguity. Can you perhaps layout the dir/file structure as you see it?
I'm wondering if perhaps we create a branch with the express purpose of throwing away as a place to visualize this architecture discussion?

@tw2113
Copy link
Member Author

tw2113 commented Mar 13, 2018

I'll brain on that and try to get back to this soon.

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

2 participants