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

add standalone jw player adapter and api logic #10

Merged
merged 23 commits into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
3dde99b
add standalone jw player adapter and api logic
manewc Sep 20, 2024
8c57df8
correct and update plugin point version
manewc Sep 20, 2024
31c3aeb
remove unnecessary origin modified date
manewc Sep 23, 2024
1bc6198
assign allowed null value to type declaration
manewc Sep 23, 2024
dcd3f22
refactor request arg retrieval for api requests
manewc Sep 23, 2024
6e57ffb
refactor logic to make the api calls more abstract
manewc Sep 23, 2024
5b66892
consolidate repeated logic and moved to a parent class
manewc Sep 24, 2024
8d24cc1
address phpcs sniffs
manewc Sep 26, 2024
1cfbed8
address phpstan errors
manewc Sep 26, 2024
4a728e9
check for function existence. PHPStan not checking the parent class m…
manewc Sep 26, 2024
400d6c6
strict parameter definition to conform to the wp_remote_get parameter…
manewc Sep 26, 2024
91d61c9
correct formatting
manewc Sep 26, 2024
55c36fc
Testing CI
srtfisher Sep 26, 2024
97b5f7b
Enable debug mode
srtfisher Sep 26, 2024
910760d
Fix action
srtfisher Sep 26, 2024
b15d7a2
Remove debug mode
srtfisher Sep 26, 2024
0bb364a
add unit test for the JW Player Adapter integration
manewc Sep 27, 2024
020ded3
provide base README documentation outlining class methods
manewc Oct 1, 2024
f54af24
implement php constructor property promotion
manewc Oct 7, 2024
e7e8ce0
ensure get_last_modified_date method is applied within adapter
manewc Oct 7, 2024
3ad4dd5
restrict setting last modified date to a DateTimeImmutable object
manewc Oct 7, 2024
7397082
ensure last modified date is a DateTimeImmutable instance, encapsulat…
manewc Oct 7, 2024
31a0135
update unit test to remove dependency class param
manewc Oct 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 106 additions & 0 deletions src/adapters/class-jw-player.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
<?php
/**
* WP Video Sync: JW Player Adapter.
*
* @package wp-video-sync
*/

namespace Alley\WP\WP_Video_Sync\Adapters;

use Alley\WP\WP_Video_Sync\API\JW_Player_API;
use Alley\WP\WP_Video_Sync\Interfaces\Adapter;
use DateTimeImmutable;
use stdClass;

/**
* JW Player Adapter.
*/
class JW_Player implements Adapter {

/**
* The date of the last modification to the last batch of videos.
*
* @var ?DateTimeImmutable
*/
private ?DateTimeImmutable $last_modified_date;
Copy link
Member

Choose a reason for hiding this comment

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

I can't figure out how this value would be read before it's initialized, as you indicated in your comment, based on my read of the code, but you could initialize it to null so it has a value which is compatible with the declared type to avoid this if it's still a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try null here, the reason was due to #10 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

See my reply to the other comment


/**
* The JW Player API.
*
* @var JW_Player_API
*/
public JW_Player_API $jw_player_api;

/**
* The start date if the `last_modified_date` does not exist.
*
* @var string
*/
public string $origin_modified_date;
Copy link
Member

Choose a reason for hiding this comment

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

Remove (see comment below)


/**
* Constructor.
*
* @param JW_Player_API $api Instance of the JW Player API object.
* @param string $origin_modified_date The date of the used if the `last_modified_date` does not exist.
*/
public function __construct( JW_Player_API $api, string $origin_modified_date = '' ) {
$this->jw_player_api = $api;
$this->origin_modified_date = ! empty( $origin_modified_date ) ? $origin_modified_date : date( 'Y-m-d' );
Copy link
Member

Choose a reason for hiding this comment

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

The plugin keeps track of the last modified date via an option, so it shouldn't also need to be kept track of by another property here. If what you want to do is to set a minimum date to override the plugin default of 1/1/1970, then you should update the option with the user-provided value if it isn't already set (see https://github.com/alleyinteractive/wp-video-sync/blob/develop/src/class-sync-manager.php#L97).

The purpose of the get_last_modified_date() function is to return the value of the last modified date after a batch of videos has been imported, because where that value comes from is going to be different from adapter to adapter - it shouldn't need to be read until after it's set by the get_videos function.

Copy link
Contributor Author

@manewc manewc Sep 23, 2024

Choose a reason for hiding this comment

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

So this update was to correct this value from not being initialized because it was typed. It seems like it it is coming from a case where the wp_video_sync_last_sync option doesn't exist, possibly coupled with the API not returning any videos. I believe what is happening is that when the option does not exist, this https://github.com/alleyinteractive/wp-video-sync/blob/develop/src/class-sync-manager.php#L99 is not true, and this method https://github.com/alleyinteractive/wp-video-sync/blob/develop/src/class-sync-manager.php#L116 is throwing the fatal:

Fatal error: Uncaught Error: Typed property Alley\WP\WP_Video_Sync\Adapters\JW_Player_7_For_WP::$last_modified_date must not be accessed before initialization in /var/www/nyp/wp-content/plugins/wp-video-sync/src/adapters/class-jw-player-7-for-wp.php:31

(I was testing the JW_Player_7_For_WP adapter to rule out if there was an issue with my adapter, but that didn't seem to be the case here)

I like the idea of setting the date, so I'll look further in to the best solution fit for this

Copy link
Member

Choose a reason for hiding this comment

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

Something else is going on here - the get_option call uses the second parameter to provide a default, which is a string, so if the option doesn't exist, it will default to the string for the Unix epoch in ISO-8601 format. If that's failing, it likely means that there is an option saved to the options table with that key, and its value is not a string.

For line 116, it will only ever be called after the get_videos method is called, which sets that property value, so there should never be a case where it gets to that part of the code and fails unless the get_videos method is failing to set that value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to track. I believe get_videos() wasn't returning any results, resulting in the the option not being set. So then when get_last_modified_date() was called, the error was flagged due to $this->last_modified_date not being assigned.

I am thinking this is a unique case due to my local testing and just uploading one batch of videos. If it makes sense to wrap https://github.com/alleyinteractive/wp-video-sync/blob/develop/src/class-sync-manager.php#L115-L119 within an! empty( $videos ) condition then I can handle that. Although I did default assign the $last_modified_date to null, which also works, and this portion may be moved "up" to a parent class shared amongst the JWP adapters.

}

/**
* Fetches the date of the last modification to the last batch of videos.
*
* @return ?DateTimeImmutable
*/
public function get_last_modified_date(): ?DateTimeImmutable {
return $this->last_modified_date ?? DateTimeImmutable::createFromFormat('Y-m-d', $this->origin_modified_date );
Copy link
Member

Choose a reason for hiding this comment

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

With advice above this becomes a simple return of the class property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this - is this a case where these jwp classes should extend a parent class?

Copy link
Member

Choose a reason for hiding this comment

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

It could, yeah, especially if your API class and the JW Player 7 class are both handling data in the same format (which they should be - the JW Player 7 class is using the JW Player 7 plugin's API helper methods and credentials and otherwise returning what it gets)

Copy link
Contributor Author

@manewc manewc Sep 24, 2024

Choose a reason for hiding this comment

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

I did rewrite this which may be seen in this commit: 5b66892. I do wonder now if this is an anti-pattern since the get_last_modified_date() function is removed from adapter.php . I just wonder as I see the chaining in this line: https://github.com/alleyinteractive/wp-video-sync/blob/develop/src/class-sync-manager.php#L116.

Let me know what you think on this and I'll move forward with either approach.

}

/**
* Sets the date of the last modification to the latest batch of videos.
*
* @param array $videos An array of videos and associated data.
* @return void
*/
public function set_last_modified_date( array $videos ): void {
if (
! empty( $videos )
&& isset( $videos[ count( $videos ) - 1 ]->last_modified )
) {
$last_modified_date = DateTimeImmutable::createFromFormat( DATE_W3C, $videos[ count( $videos ) - 1 ]->last_modified );

if ( $last_modified_date instanceof DateTimeImmutable ) {
$this->last_modified_date = $last_modified_date;
}
}
}

/**
* Fetches videos from JW Player that were modified after the provided DateTime.
*
* @param DateTimeImmutable $updated_after Return videos modified after this date.
* @param int $batch_size The number of videos to fetch in each batch.
*
* @return stdClass[] An array of video data.
*/
public function get_videos( DateTimeImmutable $updated_after, int $batch_size ): array {
// Get the latest videos from JW Player.
$videos = $this->jw_player_api->request_latest_videos(
$updated_after->format( 'Y-m-d' ),
$batch_size
);

// Check for an API error.
if ( ! empty( $videos['error'] ) ) {
return [];
}

// Attempt to set the last modified date.
$this->set_last_modified_date( $videos );

// Return the videos.
return ! empty( $videos ) ? $videos : [];
}
}
161 changes: 161 additions & 0 deletions src/api/class-jw-player-api.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
<?php
/**
* WP Video Sync: JW Player API integration.
*
* @package wp-video-sync
*/

namespace Alley\WP\WP_Video_Sync\API;

class JW_Player_API {

/**
* The API URL.
*
* @var string
*/
public string $api_url = 'https://api.jwplayer.com/v2/sites';

/**
* The API public key.
*
* @var string
*/
public string $api_key;

/**
* The API v2 secret key.
*
* @var string
*/
public string $api_secret;

/**
* Constructor.
*/
public function __construct( string $api_key, string $api_secret ) {
$this->api_key = $api_key;
$this->api_secret = $api_secret;
}

/**
* Set the user agent in the request headers for identification purposes.
*
* @return string
*/
public function user_agent(): string {
global $wp_version;

return 'WordPress/' . $wp_version . ' WPVideoSync/' . WP_VIDEO_SYNC_VERSION . ' PHP/' . phpversion();
}

/**
* Generate the request URL.
*
* @param string $last_modified_date The date of the last modification to the last batch of videos.
* @param int $batch_size The number of videos to fetch in each batch.
*
* @return string
*/
public function request_url( string $last_modified_date, int $batch_size ): string {
$request_url = $this->api_url . '/' . $this->api_key . '/media/';

return add_query_arg(
[
'q' => 'last_modified:[' . $last_modified_date . ' TO *]',
'page' => 1,
'sort' => 'last_modified:asc',
'page_length' => $batch_size,
],
$request_url
);
}

/**
* Generate the request arguments.
*
* @param string $type The type of request to make.
*
* @return array
*/
public function request_args( string $type = '' ): array {
$default_args = [
'user-agent' => $this->user_agent(),
'headers' => [
'Authorization' => 'Bearer ' . $this->api_secret,
'Content-Type' => 'application/json',
],
];

return 'vip' === $type
Copy link
Member

Choose a reason for hiding this comment

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

Rather than having type be a string that could be vip either allow the user to specify the timeout value directly or let them specify an array of args for the remote get function

? $default_args
: array_merge( $default_args, [ 'timeout' => 3 ] );
Copy link
Member

Choose a reason for hiding this comment

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

wp_parse_args instead of array_merge

}

/**
* Parse the API response.
*
* @param mixed $api_response The API response.
*
* @return array
*/
public function parse_response( mixed $api_response ): array {
// Failed request expressed as a WP_Error.
if ( is_wp_error( $api_response ) ) {
return [];
}

// Condition for when the response body is empty.
$response_body = wp_remote_retrieve_body( $api_response );

if ( empty( $response_body ) ) {
return [];
}

// Assign the response object for further evaluation.
$response_object = json_decode( $response_body );

if ( 200 !== wp_remote_retrieve_response_code( $api_response ) ) {
return isset( $response_object->errors[0]->description )
? [ 'error' => $response_object->errors[0]->description ]
: [];
}

return is_array( $response_object->media ) && ! empty( $response_object->media )
? $response_object->media
: [];
}

/**
* Make the API request.
*
* @param string $updated_after The date of the last modification to the last batch of videos.
* @param int $batch_size The number of videos to fetch in each batch.
*
* @return array
*/
public function request_latest_videos( string $updated_after, int $batch_size ): array {
Copy link
Member

Choose a reason for hiding this comment

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

When I write API adapters I typically establish a few layers of abstraction to make it easier to work with - I have a basic request method that accepts an HTTP verb with a URL and parameters, then helper functions for get and post etc, then specific functions that do specific things, so that way you can have your logic for "how to fetch a response from the API" be generic and reusable - see, for instance, https://github.com/alleyinteractive/apple-news/blob/develop/includes/apple-push-api/request/class-request.php

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about doing that initially so an API request may be made from other platforms than JW Player. I opted not to because this was just one basic call, but I can look in to breaking this out and mimic what we have in our theme's custom video player plugin.

$request_url = $this->request_url(
$updated_after,
$batch_size
);

if ( function_exists( 'vip_safe_wp_remote_get' ) ) {
$api_request = vip_safe_wp_remote_get(
$request_url,
'',
3,
5,
3,
$this->request_args( 'vip' )
);
} else {
$api_request = wp_remote_get(
$request_url,
$this->request_args()
);
}

return $this->parse_response( $api_request );
}
}
3 changes: 3 additions & 0 deletions wp-video-sync.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,7 @@
exit;
}

// Define the plugin version.
define( 'WP_VIDEO_SYNC_VERSION', '1.7.2' );
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong - it's 0.1.0 currently, this would be an 0.2.0 release


require_once __DIR__ . '/src/autoload.php';
Loading