Fixed the way HTML tags are replaced

[t-schroeder]

Fixes #18

[tobiasreischmann]

Hey Tim, thanks for the fix. Can you tell me what you need the reverse_iterator for? Cheers Tobias

[t-schroeder]

Hi Tobias, the DOMNodeList class allows you to iterate over it with foreach. But if you replace a node from that list from withing the foreach, that node gets removed from the list and the remaining nodes to the right of it (imagine the leftmost element being the first element in the list and the rightmost the last) all shift one place to the left. The foreach doesn't know that the elements in the list have shifted so it continues like nothing happened and skips the next video in the text. This problem can be avoided if you go through the videos in reverse order (imagine as right-to-left), because then it makes no difference whether you replaced the current node and the node to the right of it took its place because the node to the left of it is still the same and that's the next node you're going to process. My solution is basically just a fancy way of doing this: https://secure.php.net/manual/en/domnode.replacechild.php#50500 Cheers Tim

[t-schroeder]

Ok, I also added handling for VideoJS videos. Below is a screenshot of how the videos looked for us before that change due to the fact that the <video>...</video> got replaced but there were still two <div>s around it, one of which had a "width: 400px", which made the videos look strange.

[t-schroeder]

Don't merge this yet as it breaks unicode characters.

[t-schroeder]

Ok, fixed it.

[tobiasreischmann]

Hey Tim, thanks for your fixes. However, the changes you made got a bit too oversized from my perspective. I just wanted to have a look at your code and found a much easier way to achieve actually the same:

diff --git a/filter.php b/filter.php
index a1e68c0..8fe2e35 100644
--- a/filter.php
+++ b/filter.php
@@ -109,7 +106,7 @@ class filter_opencast extends moodle_text_filter {
                         $newtext =  $renderer->render_player($mustachedata);

                         // Replace video tag.
-                        $text = preg_replace('/<video.*<\/video>/', $newtext, $text, 1);
+                        $text = preg_replace('/<video.*?<\/video>/', $newtext, $text, 1);
                     }
                 }
             }

The single question mark changes the greedy matching to a non greedy one, which results in two players being displayed.

[t-schroeder]

Hi Tobias, thanks for looking over this. You are right, that solves the greedy matching problem. However, another problem of the original solution was that it would just replace the first video tag if there's an Opencast video anywhere in the $text. That becomes a problem e.g. when you have a YouTube video in the same text as an Opencast video, the YouTube video being above the Opencast video and you have your filters and media players setup in a way where the YouTube-Video is displayed in a video tag when the Opencast filter runs over the $text. In default Moodle installations this would be the case if you embedded the YouTube video via the YouTube repository (not as an iframe), because by default the VideoJS player handles YouTube videos and the multimedia filter would probably be run before the Opencast filter. Thus these YouTube videos would be video tags when the Opencast filter runs over them.

[tobiasreischmann]

You are right. I altered the regex a bit. It is now a bit more complex, but I included the found match in the regex. It looks for a video tag followed by the $match (which is the complete string of the source-tag) and makes sure, that there is no other video-tag in between.

diff --git a/filter.php b/filter.php
index a1e68c0..bd75446 100644
--- a/filter.php
+++ b/filter.php
@@ -109,7 +106,7 @@ class filter_opencast extends moodle_text_filter {
                         $newtext =  $renderer->render_player($mustachedata);

                         // Replace video tag.
-                        $text = preg_replace('/<video.*<\/video>/', $newtext, $text, 1);
+                        $text = preg_replace('/<video(?:(?!<\/video>).)*?' . preg_quote($match, '/') . '.*?<\/video>/', $newtext, $text, 1);
                     }
                 }
             }

Could you check, if this fixes your issue?

[t-schroeder]

Hi Tobias, you're right, that fixes the problem and is simpler than my solution. The only problem I then have is with the VideoJS player:

The Opencast videos all look like this in Moodle (the videos themselves are 16:9 aspect ratio):

That's in part because this filter sets the height to 455px and width to 95% (which itself wouldn't be a problem). The other reason is that this filter doesn't replace these <div>s of the VideoJS player: So the Opencast videos always end up with an approximate height of 455px and width of 95% of 400px.

Of course that could be fixed by running the Opencast filter before the multimedia filter so that VideoJS doesn't get a chance to insert itself. But since you can embed Opencast videos as links, too, not just as videos, you'd want the multimedia filter to run before the Opencast filter so that it converts the Opencast video link into an actual video tag so that this filter can handle it.

And I know this is specific to the VideoJS player, but that's the default video player in Moodle.

With your regex solution we could probably also remove the VideoJS <div>s but I'm not sure whether it would still be more elegant than the solution with the DOM parser.

[t-schroeder]

Continued in PR #23.