b-interactive / cloudflare-stream-wordpress Goto Github PK
View Code? Open in Web Editor NEWA fork of the official Cloudflare Stream plugin for WordPress.
License: GNU General Public License v2.0
A fork of the official Cloudflare Stream plugin for WordPress.
License: GNU General Public License v2.0
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:
auto
the appropriate default when preloading is requested?auto
) or entire video (true
) be useful?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.
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
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.
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.
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.
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.
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.
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?
We probably have updates coming that will affect blocks created with the current/old versions of the plugin:
Some ideas to mitigate this:
I'm open to thoughts on how to approach this.
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.
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:
I propose providing these controls in the admin settings with these aims:
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.
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.
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).
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?
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.
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.
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.
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.
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!...
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.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.