b-interactive/cloudflare-stream-wordpress

A fork of the official Cloudflare Stream plugin for WordPress.

cloudflare-stream-wordpress's Issues

Removal of analytics

I wanted to confirm my impression of the decision to remove analytics (0502915). From what was being logged to Heap, I assume from what was being logged that these are for Cloudfront's usage and don't affect analytics on videos (i.e., the view for account holders, not Cloudflare corporate). Does that sound accurate?

Admin CSS enqueued in wrong class.

I misused /src/init.php for enqueing scripts/styles not related to blocks, specifically styles for use in the admin area. I've now moved the section in question to the /src/inc/class-cloudflare-stream-settings.php file with commit 8014c0c.

/src/init.php is used exclusively for blocks, which isn't obvious from the filename. In my prior plugin projects, the comparable file would be used for broader plugin initialisation, hence the mix-up.

Option for account specific sub-domains

Perhaps to address issues with some countries blocking Cloudflare Stream access, Cloudflare has announced support for account specific sub-domain access to Cloudflare Stream resources.

The subdomain consists of an account specific code. I've not yet probed or tested this, but hopefully the code is obtainable via token based access to the API. If so, a plugin user could choose to activate use of the subdomain with a checkbox option.

I believe it needs to remain optional, because it could conflict with any pre-existing content security policies active on the web host (effectively blocking user access to content). In those cases, an admin needs to first update their web hosts content security policies to ensure the subdomain is permitted.

Support constants for api keys

Allow for constants to be used to override the option to set the api key and account id via settings api.

This would allow sites that use environment variables or secrets managers, so set the constants via env and remove the data from being stored directly in the database, as plain text.

Shotcode sizing issues

Hello.

I am trying to use your plugin. It works great where initial integration and setup is concerned. However, I am running into issues regarding "shortcode" usage for video rendering.
The issue is that the video player size is unreasonable. I attach the screenshot.

Regular player looks good:

Maybe you could take a look?

Not all URL parameters work in iframe embed player

It appears that the iframe player treats boolean parameters that are false by default as "true" if they are present at all in the querystring (even if they are set to false).

Affected parameters I've identified are muted, loop, and autoplay. (The behavior is documented for autoplay.)

For example, the following iframe src results in a video that does autoplay, start muted, and loop. (But does not show controls.)

https://iframe.cloudflarestream.com/uid123456?muted=false&preload=false&loop=false&autoplay=false&controls=false

I found this updating the block, but it affects shortcodes as well. For example, the following shortcode generates a URL like the one above which treats autoplay, loop, and muted as effectively true.

[cloudflare_stream uid="[uid]" controls="false" autoplay="false" loop="false" preload="false" muted="false"]

I've raised the issue of at least documenting the behavior, if not allowing false values.

I think the best solution is to add some logic to where querystring parameters are added in Cloudflare_Stream_API::get_video_embed to simply not add parameters that are falsy or "false".

I'm working on a PR now.

Preload behaviour for video embed

Touched on here, the Cloudflare Stream Player's preload attribute has a little more to it than simply true or false. The attribute is considered a hint for browsers on how to handle preloading, so the behaviour may vary. If I understand, the hints mean the following:

• preload=none / preload=metadata / not specifying anything
  Any of the above will just load the metadata needed to start video playback when requested.

• preload=auto
  Will preload the beginning of the video.

• preload=true
  Will preload the entire video.

As of 1cb2038, preload=auto is used when preload is requested by a shortcode. I considered this a safer option, as preload=true could result in bill shock, and a poor end-user experience, if the implications aren't fully understood.

Open to discussion:

1. Is auto the appropriate default when preloading is requested?
2. Might the ability for the user to specify preload behaviour of beginning (auto) or entire video (true) be useful?

Block editor doesn't preview videos requiring signed URLs

The block editor needs to use signed URLs when they're enabled in settings. I'll work on a PR for this.

@B-Interactive what would you think about moving the iframe URL without the querystring into a separate function? Then the editor could get it via AJAX without needing to duplicate the logic.

Unify rendering of block and shortcode

The methods responsible for rendering the block and rendering the shortcode share a lot in common, perhaps intentionally.

Unifying these into a single method that serves both, might be something to consider.

Add support for posters (cover photo) for signed url videos

Working on a yoga platform (see attached screenshot) that uses signed url videos when a user is signed in. However, I am unable to set a custom cover photo for each video when working with signed urls and using shortcodes. I would like to set a custom poster, cover photo, for every video because if you can tell from the screenshot all the videos have the same poster photo (light orange background) which makes it look dull

Deprecation of plugins use of zones API

Features related specifically to the zones API have been flagged as deprecated with bfad540 and 6195cc8, for removal when appropriate.

Upload button is missing

Looks like I broke the upload button didn't come in with my block fix. Probably because I based my solution on previous work that didn't incorporate the API token improvement, and the block uses API credentials to directly upload files via tus.

Working on a fix to send the token/zone instead of the key/account/email to the block script and change the base URL appropriately within the block.

Playback defaults in admin settings

Currently playback options can be specified on a per-video basis. The code for this at the moment, is specific to shortcodes. The available options, with their current defaults are:

• controls = true
• autoplay = false
• loop = false
• preload = false
• muted = false

I propose providing these controls in the admin settings with these aims:

1. The user can specify and modify default playback options universally for all videos.
2. Videos can still override these defaults on a per-video basis.
3. Use of these defaults is no longer shortcode specific, but applies to blocks also.

This proposal is contingent on how blocks will be implemented, and whether they can dynamically access and optionally override these defaults.

The autoplay/muted options have the caveat that when autoplay is enabled, browsers will tend to force mute the video. Browsers such as Chrome will apparently adapt based on user behaviour, so it's not a definite rule, but it should be expected.

Update the block player to the new iframe player

The block needs to be updated to use the iframe player (probably in both the edit and save functions). I have something for this working locally that is almost ready for a PR.

Currently my code uses the second option from the docs (just using their iframe template). It's probably a little more robust going forwards to use the API endpoint like the shortcode does, however.

Localization updates

As I was working on updating the block to the latest recommendations, I noticed the plugin was missing a text domain and i18n functions were used inconsistently. I'm doing a quick pass to add those in.

Build scripts

I'm working on some updates and fixes to the official plugin (first up: getting the block fixed and updated), and saw you had a fork that looks more actively maintained. I love what you've done (API tokens!) and I'm all for contributing to an existing fork rather than starting yet another, but I don't see build scripts there, however. Do you have a build step for blocks in place (I know that hasn't been the focus of this fork)? If not, are you actively managing this fork and open to PRs for things like that?

Switch from zones to accounts API

I'm opening this for contemplation. I'm currently not fully convinced a change is needed, but I realise I may also be looking at it wrong.

I've used the /zones API for API calls. This is as opposed to using the standard /accounts API.

Feedback in the Cloudflare forum prompted me to consider this again. At the very least, it looks like I need to update README.md, because referencing the /zones API as "more secure" doesn't appear to apply to Stream, based on that feedback.

The reason I originally used the /zones API, is because it, in conjunction with an API token with only Account.Stream:Edit permission, could access some general account data, that is otherwise unavailable using the /accounts API.

For example (using /zones):

curl -X GET "https://api.cloudflare.com/client/v4/zones/{ZONE ID}" -H "Authorization: Bearer {TOKEN}" -H "Content-Type:application/json"

Returns data that includes the account ID and account email (among other things).

Using the /accounts equivalent on the other hand, produces a permission error, unless additional permissions beyond Account.Stream:Edit are granted (I'm not sure which specific one is required):

curl -X GET "https://api.cloudflare.com/client/v4/accounts/{ACCOUNT ID}" -H "Authorization: Bearer {TOKEN}" -H "Content-Type:application/json"

{"success":false,"errors":[{"code":9109,"message":"Unauthorized to access requested resource"}],"messages":[],"result":null}

Currently, the /zones API is not being used to pull these additional details in this plugin. In an earlier (non-public) version of my own plugin, I had pulled the account email and ID and simply presented it in the plugin's settings.

However, potentially the same /zones API request could be made to grab the account specific subdomain. This is something I might raise with Cloudflare.

Plugin name / permalink slug currently conflicts with Cloudflare Stream

In preparation for plugin submission to WordPress for review, the current plugin name (which determines its permalink slug) conflicts with the legacy plugin.

 * Plugin Name: Cloudflare Stream

Which upon submission would attempt to resolve to the legacy plugin's permalink, and conflict:
https://wordpress.org/plugins/cloudflare-stream/

WordPress - Plugin Developer FAQ - What will my plugin permalink (slug) be?

This is populated based on the value of Plugin Name in your main plugin file...

If you set yours as Plugin Name: Boaty McBoatface then your URL will be wordpress.org/plugins/boaty-mcboatface and your slug will be boaty-mcboatface

You can’t have two plugins with the same permalink so you need to pick a new one.

A new name is therefore required. Requesting ideas!...

Update readme.txt

Before submitting the plugin for WordPress review, the readme.txt needs to be updated to reflect its nature as a fork.

At a minimum, I suspect this will include the fields for:

• Plugin Name? (#24)
• Description (refine and perhaps note fork?)
• Author (add @B-Interactive and @davidmpurdy and remove Cloudflare who'll maintain a mention under Contributors)
• Author URI (perhaps points to same as Plugin URI)
• Tags (refine, and not sure wpengine is still necesarry)
• Tested up to (update)

...
Refinements to body copy.

Refactor block to use functional components

As the tendency of React and WordPress is to move towards functional components code, I suggest that the block is refactored to use this.

At the same time jQuery could be removed from the block code and instead rely on fetch and a couple of 3rd party libraries for TUS and Stream.

I have already started work on this, though I mistakenly deleted it all, so I started doing the work again.

I should also help make the code more structured and easier to maintain, in the long run, plus it should make it easier to implement testing, further down the line.

I would also be nice to use some of the built in components that Gutenberg supply, such as the MediaPlaceholder component, for giving a unified look to the block.

Upgrade path for existing blocks

We probably have updates coming that will affect blocks created with the current/old versions of the plugin:

• Using the new iframe player. #6
• Adding signed URLs (setting videos to require signed URLs would break existing blocks that don't have that capability). #3

Some ideas to mitigate this:

1. Add a render callback for old blocks to intercept the current markup and output something new. Feels like it's probably the easiest to implement but to keep it working we have to keep supporting that existing block markup indefinitely.
2. Write an upgrade process to search through existing posts containing the current block and update them in each post's source. I've written background routines before and it's always been kind of a pain. This might be the cleanest going forwards, but it feels like a lot of work to implement well.
3. Make a new major version that just breaks existing blocks if signed URLs are enforced for a video or the old player ever stops working. This is kind of the default of what would happen to existing blocks anyway. This fits my personal use case, but if we want this to serve a wider user base might not be the friendliest if there are folx with a ton of existing videos.

I'm open to thoughts on how to approach this.

WordPress plugin adoption

I wasn't previously aware of this option.

WordPress - Take Over an Existing Plugin

It's by no means a guaranteed process, and there are some benefits to a break from legacy. There are still hundreds of active installations however, that could benefit.

Use signed URLs for videos inserted with the block

I'm not 100% sure how to approach this. Just making it a dynamic block looks to me like it would bring it into parity with the shortcode implementation. However, I have concerns about caching (e.g., where the page output is cached longer than the token expiration time).

Maybe the block should include a script that uses AJAX to get the token and set up the player? That would let you freely cache pages with videos while still retaining the benefits of signed URLs (e.g., videos could be restricted to logged-in users only but could still use page caching for those logged in users since the restriction would take place in the AJAX callback).

Any thoughts on changing the architecture in this way? I'm pretty sure that no matter what we do with the block (even just updating to the new iframe player) it will likely break editing for existing blocks. Thoughts on that and whether we would need to try and implement an update to existing blocks or if we could just make it a breaking change? Since blocks currently have the full (but old) script embedded, I'm thinking we could just leave them as-is (meaning they should keep chugging along but they couldn't be edited).