add standalone jw player adapter and api logic #10

manewc · 2024-09-20T15:35:54Z

src/api/class-jw-player-api.php: This class is to handle the API logic for performing a request to the JWPlayer API and handle the responses.

Note

I had to update the function get_last_modified_date() because $this->last_modified_date was throwing a PHP error (Typed property must not be accessed before initialization) which I believe was due to the option of the last modified date not existing. I thought I would mention as I wasn't sure why this is different than what is in the class-jw-player-7-for-wp.php file.

src/adapters/class-jw-player.php: This is the Adapter used for syncing the posts. This class is instantiated with the JW_Player_API class - this could be changed to be accepting a class that is adhering to an Interface, but it doesn't seem necessary at this beginning stage.

WP_VIDEO_SYNC_VERSION: This constant was added so it may be output as the user agent in the header.

kevinfodness · 2024-09-20T16:00:28Z

src/adapters/class-jw-player.php

+	 */
+	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' );


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.

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

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.

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.

kevinfodness · 2024-09-20T16:15:01Z

src/adapters/class-jw-player.php

+	 * @return ?DateTimeImmutable
+	 */
+	public function get_last_modified_date(): ?DateTimeImmutable {
+		return $this->last_modified_date ?? DateTimeImmutable::createFromFormat('Y-m-d', $this->origin_modified_date );


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

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

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)

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.

kevinfodness · 2024-09-20T16:21:12Z

src/adapters/class-jw-player.php

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


Remove (see comment below)

kevinfodness · 2024-09-20T16:23:18Z

src/adapters/class-jw-player.php

+	 *
+	 * @var ?DateTimeImmutable
+	 */
+	private ?DateTimeImmutable $last_modified_date;


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.

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

See my reply to the other comment

kevinfodness · 2024-09-20T16:25:57Z

src/api/class-jw-player-api.php

+			],
+		];
+
+		return 'vip' === $type


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

kevinfodness · 2024-09-20T16:26:07Z

src/api/class-jw-player-api.php

+
+		return 'vip' === $type
+			? $default_args
+			: array_merge( $default_args, [ 'timeout' => 3 ] );


wp_parse_args instead of array_merge

kevinfodness · 2024-09-20T16:29:53Z

src/api/class-jw-player-api.php

+	 *
+	 * @return array
+	 */
+	public function request_latest_videos( string $updated_after, int $batch_size ): array {


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

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.

kevinfodness · 2024-09-20T16:31:07Z

wp-video-sync.php

@@ -21,4 +21,7 @@
 	exit;
 }

+// Define the plugin version.
+define( 'WP_VIDEO_SYNC_VERSION', '1.7.2' );


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

manewc · 2024-09-25T16:04:08Z

Hi @kevinfodness , I've done some refactoring and addressed your previous comments. Please have a subsequent look when you get a chance. Much appreciated!

NOTE: this is coupled with our theme changes located: https://github.com/alleyinteractive/nypost-go/pull/23678

Morgan

…ethod for defined function

… shape

srtfisher · 2024-09-26T19:09:50Z

src/api/class-request.php

+	/**
+	 * The API requester.
+	 *
+	 * @var API_Requester
+	 */
+	public API_Requester $api_requester;
+
+	/**
+	 * Constructor.
+	 *
+	 * @param API_Requester $api_requester The API requester.
+	 */
+	public function __construct( API_Requester $api_requester ) {
+		$this->api_requester = $api_requester;
+	}


Suggested change

/**

* The API requester.

*

* @var API_Requester

*/

public API_Requester $api_requester;

/**

* Constructor.

*

* @param API_Requester $api_requester The API requester.

*/

public function __construct( API_Requester $api_requester ) {

$this->api_requester = $api_requester;

}

/**

* Constructor.

*

* @param API_Requester $api_requester The API requester.

*/

public function __construct( public API_Requester $api_requester ) {}

manewc · 2024-09-27T18:51:01Z

@kevinfodness FYI, I have some more changes I'd like to make, so I requested a review earlier in haste. I'll let you know when it's all complete from my perspective.

Thanks!

manewc · 2024-10-01T20:45:11Z

@kevinfodness if you would have another look over the latest changes that would be much appreciated. Thanks!

kevinfodness · 2024-09-26T19:04:38Z

src/adapters/class-jw-player.php

+	/**
+	 * The JW Player API.
+	 *
+	 * @var JW_Player_API
+	 */
+	public JW_Player_API $jw_player_api;
+
+	/**
+	 * Constructor.
+	 *
+	 * @param JW_Player_API $api Instance of the JW Player API object.
+	 */
+	public function __construct( JW_Player_API $api ) {
+		$this->jw_player_api = $api;
+	}


Let's make this a modern PHP constructor:

Suggested change

/**

* The JW Player API.

*

* @var JW_Player_API

*/

public JW_Player_API $jw_player_api;

/**

* Constructor.

*

* @param JW_Player_API $api Instance of the JW Player API object.

*/

public function __construct( JW_Player_API $api ) {

$this->jw_player_api = $api;

}

/**

* Constructor.

*

* @param JW_Player_API $jw_player_api Instance of the JW Player API object.

*/

public function __construct( public readonly JW_Player_API $jw_player_api ) {}

kevinfodness · 2024-10-04T16:11:12Z

src/api/class-request.php

+	/**
+	 * The API requester.
+	 *
+	 * @var API_Requester
+	 */
+	public API_Requester $api_requester;
+
+	/**
+	 * Constructor.
+	 *
+	 * @param API_Requester $api_requester The API requester.
+	 */
+	public function __construct( API_Requester $api_requester ) {
+		$this->api_requester = $api_requester;
+	}


kevinfodness · 2024-10-04T16:14:55Z

src/class-last-modified-date.php

+	 * @param string $last_modified_date The date of the last modified video in the batch.
+	 * @return void
+	 */
+	public function set_last_modified_date( string $last_modified_date = '' ): void {


I feel pretty strongly that this should accept a DateTimeImmutable rather than a string. The date formats returned by different APIs won't necessarily be the same, so it's best to trade in a common format, which is DateTimeImmutable. This way, the JW Player API response (which returns an ISO-8601 date compatible with the DATE_W3C format of DateTime) can be converted to a DateTimeImmutable object in the adapter and passed here where it will be set. If another adapter gets dates in a different format (e.g., Y-m-d H:i:s or just Y-m-d or a Unix timestamp) that adapter would be responsible for standardizing on DateTimeImmtuable, etc.

kevinfodness · 2024-10-04T16:16:57Z

src/interfaces/adapter.php

-	 *
-	 * @return ?DateTimeImmutable
-	 */
-	public function get_last_modified_date(): ?DateTimeImmutable;


We shouldn't remove this from the interface. Being able to keep a cursor and fetch updated videos since the last run is core to the mission of this library. By removing this from the interface, it grants permission to adapters to not keep a cursor and do a full sync of all videos on every run, which we don't want.

ah, good point.

kevinfodness · 2024-10-04T16:19:26Z

src/api/class-request.php

+/**
+ * Perform an API request.
+ */
+class Request {


This is interesting - I've always implemented API adapters as self-contained, rather than passing them as dependencies using DI into a generic request class. Is this a pattern that we're using elsewhere? I can see how it would cut down on repeated code, but I can also see how it would introduce some confusion into utilization (you have to set the properties of the API object including request URL, then pass it to the request class for execution, versus calling a method on the API class to get the data). A way to make this a bit more user friendly would be to encapsulate this functionality in the JW Player API class like so:

public function get_videos_after( DateTimeImmutable $updated_after, int $batch_size ): array { $this->set_request_url( $updated_after->format( 'Y-m-d' ), $batch_size ); return ( new Request( $this ) )->get(); }

That code could be moved out of the adapter and into the API wrapper, and then the adapter would call $this->jw_player_api->get_videos_after( $updated_after, $batch_size ) and would otherwise proceed as normal. That makes the way the API class makes its requests an implementation detail, rather than something that the adapter class needs to worry about.

This has been updated in the latest commit: 7397082

…e methods for the jwp api implementation

srtfisher

🫡

srtfisher · 2024-10-08T15:08:47Z

tests/Feature/JWPlayerAdapterTest.php

 			->with_response_code( 200 )
-			->with_body( file_get_contents( __DIR__ . '/../Fixtures/jw-player-api-v2-media.json' ) );
+			->with_file( __DIR__ . '/../Fixtures/jw-player-api-v2-media.json' );


@srtfisher

@srtfisher has approved. further validation may be read here: https://alleyinteractive.slack.com/archives/C8A89JLHX/p1728413714181879?thread_ts=1728313706.141549&cid=C8A89JLHX

add standalone jw player adapter and api logic

3dde99b

kevinfodness requested changes Sep 20, 2024

View reviewed changes

manewc added 6 commits September 20, 2024 14:12

correct and update plugin point version

8c57df8

remove unnecessary origin modified date

31c3aeb

assign allowed null value to type declaration

1bc6198

refactor request arg retrieval for api requests

dcd3f22

refactor logic to make the api calls more abstract

6e57ffb

consolidate repeated logic and moved to a parent class

5b66892

manewc added 5 commits September 26, 2024 08:00

address phpcs sniffs

8d24cc1

address phpstan errors

1cfbed8

check for function existence. PHPStan not checking the parent class m…

4a728e9

…ethod for defined function

strict parameter definition to conform to the wp_remote_get parameter…

400d6c6

… shape

correct formatting

91d61c9

srtfisher reviewed Sep 26, 2024

View reviewed changes

srtfisher and others added 5 commits September 26, 2024 15:13

Testing CI

55c36fc

Enable debug mode

97b5f7b

Fix action

910760d

Remove debug mode

b15d7a2

add unit test for the JW Player Adapter integration

0bb364a

provide base README documentation outlining class methods

020ded3

kevinfodness previously requested changes Oct 4, 2024

View reviewed changes

manewc added 5 commits October 7, 2024 10:19

implement php constructor property promotion

f54af24

ensure get_last_modified_date method is applied within adapter

e7e8ce0

restrict setting last modified date to a DateTimeImmutable object

3ad4dd5

ensure last modified date is a DateTimeImmutable instance, encapsulat…

7397082

…e methods for the jwp api implementation

update unit test to remove dependency class param

31a0135

srtfisher approved these changes Oct 8, 2024

View reviewed changes

manewc merged commit c3481d5 into develop Oct 9, 2024
2 checks passed

manewc deleted the feature/NYP-21439/jwp-adapter branch October 9, 2024 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add standalone jw player adapter and api logic #10

add standalone jw player adapter and api logic #10

manewc commented Sep 20, 2024

kevinfodness Sep 20, 2024

manewc Sep 23, 2024 •

edited

Loading

kevinfodness Sep 23, 2024

manewc Sep 24, 2024

kevinfodness Sep 20, 2024

manewc Sep 23, 2024

kevinfodness Sep 23, 2024

manewc Sep 24, 2024 •

edited

Loading

kevinfodness Sep 20, 2024

kevinfodness Sep 20, 2024

manewc Sep 23, 2024

kevinfodness Sep 23, 2024

kevinfodness Sep 20, 2024

kevinfodness Sep 20, 2024

kevinfodness Sep 20, 2024

manewc Sep 23, 2024

kevinfodness Sep 20, 2024

manewc commented Sep 25, 2024

srtfisher Sep 26, 2024

kevinfodness Oct 4, 2024

manewc commented Sep 27, 2024

manewc commented Oct 1, 2024

kevinfodness Sep 26, 2024

kevinfodness Oct 4, 2024

kevinfodness Oct 4, 2024

kevinfodness Oct 4, 2024

manewc Oct 7, 2024

kevinfodness Oct 4, 2024

manewc Oct 7, 2024

srtfisher left a comment

srtfisher Oct 8, 2024

add standalone jw player adapter and api logic #10

add standalone jw player adapter and api logic #10

Conversation

manewc commented Sep 20, 2024

Choose a reason for hiding this comment

manewc Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manewc Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manewc commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manewc commented Sep 27, 2024

manewc commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srtfisher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manewc Sep 23, 2024 •

edited

Loading

manewc Sep 24, 2024 •

edited

Loading