Move the media source from cloudflare_stream_hosted_video to cloudflare_stream

Sounds very reasonable. I'm willing to review any merge request with the changes. I'll wait to do a proper 3.x release so we can add this to 3.x so we don't need to later jump another major because of the changes done here.

I raised a MR which:

• Move the media source from the cloudflare_stream_hosted_video submodule to the main cloudflare_stream module
• Rename the media source from HostedVideo to Cloudflare Stream
• Rename the submodule cloudflare_stream_hosted_video to cloudflare_stream_bundle
• Adapt the code in cloudflare_stream_bundle to use the new bundle name and media source name
• Adapt the code in cloudflare_stream_sync

I adapted the cloudflare_stream_sync module as-is but I think it should be adapted to be more generic and not only check the cloudflare_stream media bundle provided by cloudflare_stream_bundle but instead should check for all the bundles using the cloudflare_stream media source. We can probably have a checkbox added to cloudflare_stream media source when cloudflare_sync module is enable to decide if the bundle should be synced or not.

Please note that I have not been able to test my change (other than enabling the module and checking the possibility to create a new media type, etc) as the Cloudflare test account I was using for my PoC expired.

This MR looks fine on a read-through. I'll make time to test the modified code against our Stream instance very soon. We would be a new user, but I would think it would be important to have a post-update hook those moving from 2.x who have the submodule installed.

Fixed typo in title.

@vbouchet, you can contact me to get access to my account.

I've tested almost everything and it works like expected.

Like @FatherShawn point out, we will need an upgrade path to allow to update from 2.x to 3.x
So setting this to needs work as we will need an upgrade path to continue.

Tried thinking about an upgrade path, but not as easy as it should be.

* Update media type
* Update form displays
* Update view displays
* Update field config

Also the module name switch will need to have the old module still available to be able to uninstall it.
But installing the new module is maybe not needed as the field names won't match...
Also the Sync command will need an option to chose which destination entity it should be?

A lot to think about. Lets discuss the things here...

I'm exploring simplifying this solution:

2. Still move the media source plugin to the main module and rename it.
3. Leave the existing sub-module in place but adjust as needed for the plugin changes.

If we think that we should rename the sub-module, I'd recommend that we do that in a later release. We can notify people in the release notes to simply move the existing submodule into their custom modules.

Here are two merge requests. MR-30 marks the hosted video module and source as deprecated in the 2.x branch. In #2928256: Users shouldn't be able to change the media source plugin after the media type is created → changing of media sources was blocked in core. The source property on Media types is a protected property. There may be ways to circumvent that but it is only intended to be changed before the type is saved. This was permitted in #2928851: Allow the media source of a media type to be changed when creating a new media type → .

MR-31 creates a new source in the main module. I've adjusted the sync commands and the form for selection of the target type. I've also moved the required core version to 10.3 so that the shift from Annotations to Attributes for plugin metadata can be accommodated.

Thanks @FatherShawn for looking into this and pinging me on Slack and also thanks for the nice call we had a few days ago. I'm looking forward to get this landed.

As you mentioned, we should mark the cloudflare_stream_hosted_video as obsolete and provide guidance on how to upgrade to 3.0.x. We could add this as a Change Record to this module.

2.x changes:

When adding the changes from MR 30 for 2.x, the site throws an error:

Drupal\Core\Extension\InfoParserException: Extension Cloudflare Stream - Hosted video (modules/contrib/cloudflare_stream/modules/cloudflare_stream_hosted_video/cloudflare_stream_hosted_video.info.yml) has 'lifecycle: obsolete' but is missing a 'lifecycle_link' entry. in Drupal\Core\Extension\InfoParserDynamic->parse() (line 95 of core/lib/Drupal/Core/Extension/InfoParserDynamic.php).

For deprecated and obsolete lifecycle states, a lifecycle_link is required which specifies a URL to the documentation to display to users. This helps orient site builders as to what do. See https://www.drupal.org/node/3215042 →

We could use the Change Record for the lifecycle_link.

3.0.x changes:

When adding the changes from MR 31 for 3.0.x, I'm missing functionality:

• The template is not used anymore so the description is not shown.

For the rest I'm not having any functional problems. I'll look deeper into the code later.

Oh for the 3.0.x changes I tested the Drush command, and I got a straight error.

AssertionError: Cannot load the "media_type" entity with NULL ID. in /var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php on line 261 #0 /var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(261): assert(false, 'Cannot load the...')

Maybe we should give a nicer error?

Thanks for the review @tim-diels!

1. I added the change record link to the info file and the deprecation comment tag on the hosted video source plugin.
2. The missing description was because the description field is not enabled by default:


Here's the resulting markup:

<div id="0e31b8bd-41f3-4a71-9b09-b21836628c30">

  <!-- THEME DEBUG -->
  <!-- THEME HOOK: 'cloudflare_video' -->
  <!-- BEGIN OUTPUT from 'modules/cloudflare_stream/templates/cloudflare-video.html.twig' -->

  <stream src="redacted-id-string" preload="" style="width: 854px; height: 480px;">
    <div style="position: relative; height: 0px; width: 100%; padding-top: 56.3388%;">
      <iframe style="height: 100%; width: 100%; position: absolute; top: 0px; left: 0px; border: medium;"
              src="https://embed.videodelivery.net/embed/iframe.fla9.3b8bfec.html?videoId=redacted-id-string"
              allowfullscreen="true" allow="autoplay; picture-in-picture"></iframe>
    </div>
  </stream>
  <script data-cfasync="false" defer="" type="text/javascript"
          src="https://embed.videodelivery.net/embed/r4xu.fla9.latest.js?video=redacted-id-string"></script>
  <p class="description">The captain's chair</p>

  <!-- END OUTPUT from 'modules/cloudflare_stream/templates/cloudflare-video.html.twig' -->
</div>

3. I added a targeted exception at the case in which no media type id is passed to the drush command and none is set in sync settings. This now results in the CLI in that case:
   [error] ValueError: A media type ID was not provided and none is stored in cloudflare_stream_sync.settings in Drupal\cloudflare_stream_sync\SyncVideos->syncVideos()

Resolved all warnings from the code linters in the GitLab pipelines.

The 2.x MR 30 looks good. Tested this and works as expected with a nice message on the status page. Great work

tim-diels → closed merge request !27

tim-diels → changed the visibility of the branch 3.0.x to hidden.

Sorry did something wrong. Removed a file accidently and changed status.

@fathershawn

No problem at all.

1. Works as expected. Thanks.

2. For some reason this didn't work in the first place but does now with new tests. This should probably be a test in the future so we don't need to manually test this.

3. Looks much better and is descriptive enough to understand what is missing when trying to run the command.

Some things I changed or added:

• Adjusted some comments in the drush command so they were not defaults and use the ones if they existed.
• Changed some variable names as I think they fit better.
• Added config schema for cloudflare_stream_sync.settings

I think we're finished then.

Could you please test the changes I did? If all is fine, then we can commit this.

@tim-diels I tested syncing with your changes locally and we had a Cloudflare Stream video with a name that had characters that threw an error when used as a file name. That revealed a bug in the drush error logging and the filename method of Video Sync. The most recent commit fixes that. All of your work checks out.

@fathershawn

Looks like we've completed this. I'm going to merge it to dev.

We'll need to update the Change Record with the correct info. Is this something you would like to help with?

Yes - I'll write something on Monday in the Change Record :)

Great thanks 🙏

Change record draft is updated.

Thanks @fathershawn

I've published the Change Record → .

Automatically closed - issue fixed for 2 weeks with no activity.