Problem/Motivation

Dreambroker videos have some new parameters that can be used when embedding videos:

  • quality -> for example "1080p" and this will automatically set the video quality
  • lang -> for example "sv" and this will allow the use of subtitles in the video for the provided language code

Steps to reproduce

Just add parameters to the url, like &quality=1080p&lang=sv

Proposed resolution

Allow using those parameters. Currently the module just strips away all the other parameters except "autoplay" that can be set in the module settings.
The optimal solution would be some kind of system that would be more flexible if the parameters change or there will be more of them in the future.

Remaining tasks

Devise a solution and apply a patch.

User interface changes

None, unless something will be added to the module settings because of this.

CommentFileSizeAuthor
#11 interdiff-8-11.txt3.53 KBkekkis
#11 dreambroker-query-parameters-3366926-11.patch7.66 KBkekkis
#8 dreambroker-query-parameters-3366926-8.patch11.05 KBhartsak
#5 interdiff_3-5.txt1.45 KBmrinalini9
#5 dreambroker-query-parameters-3366926-5.patch10.66 KBmrinalini9
#3 dreambroker-query-parameters-3366926-3.patch10.6 KBhartsak
#2 dreambroker_query_parameters_1.patch1.85 KBhartsak

Issue fork media_entity_dreambroker-3366926

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

hartsak created an issue. See original summary.

hartsak’s picture

hartsak commented
StatusFileSize
new dreambroker_query_parameters_1.patch1.85 KB

Here is a quick fix that allows using just those mentioned parameters - "lang" and "quality".
Obviously, this could be made much better, but like mentioned, this is a quick fix :)

hartsak’s picture

hartsak commented
StatusFileSize
new dreambroker-query-parameters-3366926-3.patch10.6 KB

Ok, another try.
The first patch was a failure as it would not allow using DreamBroker videos without lang and quality parameters. That should be fixed now. In addition, I tried to fix deprecated/broken tests. So let's see how this attempt turns out (fingers crossed).

kekkis’s picture

kekkis
he/him
Primary language Finnish
Location Pirkkala
commented
Status: Active » Needs work

Thank you for the contribution! Here are a couple of notes on the patch.

+++ b/src/Plugin/Field/FieldFormatter/DreambrokerEmbedFormatter.php
@@ -53,22 +53,34 @@ class DreambrokerEmbedFormatter extends FormatterBase {
+        $channelid = current($matches['id']['channelid']);

I would like to see variable names conforming better to coding standards, e.g. $channel_id.

+++ b/src/Plugin/Field/FieldFormatter/DreambrokerEmbedFormatter.php
@@ -53,22 +53,34 @@ class DreambrokerEmbedFormatter extends FormatterBase {
+        // Use preg_match_all() to match multiple times (for URL params)

Coding standards mandate ending all inline comments with a full stop.

mrinalini9’s picture

mrinalini9 commented
Status: Needs work » Needs review
StatusFileSize
new dreambroker-query-parameters-3366926-5.patch10.66 KB
new interdiff_3-5.txt1.45 KB

Updated patch #3 by addressing #4, please review it.

Thanks!

hartsak’s picture

hartsak commented

Thanks @mrinalini9! The patch in #5 seems to work.

vsheokeen’s picture

vsheokeen commented
Status: Needs review » Reviewed & tested by the community

add parameters to the url with quality=1080p&lang=sv and it worked well.

Moing it to RTBC.

hartsak’s picture

hartsak commented
StatusFileSize
new dreambroker-query-parameters-3366926-8.patch11.05 KB

Sorry about mixing things up a bit, but I just noticed that the patch in this issue also fixes a lot of Drupal 10 compatibility issues. Because this project doesn't have the Automated D10 patch, I guess we could use this issue to come up with a simple D10 support at the same time?

I checked the patch from #5 with Upgrade Status module and only thing missing from it is the version number. So here is another patch that should add it as well. After that this could be used for Drupal 10 upgrade too?

> -core_version_requirement: ^8.8 || ^9
> +core_version_requirement: ^8.8 || ^9 || ^10
kekkis’s picture

kekkis
he/him
Primary language Finnish
Location Pirkkala
commented

I am choosing a different path with the versions. I created a different issue for pure D10 support (see https://git.drupalcode.org/project/media_entity_dreambroker/-/merge_requ... and #3395467). I will publish that version as 2.0.0 and will add a 2.1.x branch where I'll incorporate these changes. Thanks again for the efforts!

kekkis’s picture

kekkis
he/him
Primary language Finnish
Location Pirkkala
commented
Version: 8.x-1.x-dev » 2.1.x-dev
Assigned: Unassigned » kekkis
Status: Reviewed & tested by the community » Needs review

Updating status and base version to rerun some testing and to create a MR.

kekkis’s picture

kekkis
he/him
Primary language Finnish
Location Pirkkala
commented
Assigned: kekkis » Unassigned
StatusFileSize
new dreambroker-query-parameters-3366926-11.patch7.66 KB
new interdiff-8-11.txt3.53 KB

Uploading new patch which is based on 2.1.x. Also providing interdiff, even if that now contains changes already present in the published version 2.0.0 since they weren't yet present in the patches here.

Please @hartsak if you are able, test the issue fork with a D10 installation to see whether my changes are valid.

kekkis opened merge request !2

hartsak’s picture

hartsak commented

Hello @kekkis!
I finally had time and an opportunity to test the issue fork in my D10 (10.1.5) project. Works as expected: D10 compatible, both quality and lang parameters work. The videos also work without the additional parameters.

Thank you for your efforts in maintaining this!

  • kekkis committed 5655a52c on 2.1.x 
    Issue #3366926 by kekkis, hartsak: Support for quality and lang query...
kekkis’s picture

kekkis
he/him
Primary language Finnish
Location Pirkkala
commented
Status: Needs review » Fixed
kekkis’s picture

kekkis
he/him
Primary language Finnish
Location Pirkkala
commented

Thank you for the contributions here, I assigned credit to everyone involved. Sorry that I didn't think to add this comment in the same status update.

Status: Fixed » Closed (fixed)

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