From 6f8fc9c13d652a7bc8cd8412274db0f7b248360c Mon Sep 17 00:00:00 2001 From: Arit Amana <32520970+msarit@users.noreply.github.com> Date: Wed, 8 Dec 2021 14:21:51 -0500 Subject: [PATCH] Implement Unified Embed for Vimeo URLs (#15699) * complete implementation; add/fix specs; fix some straggling YoutubeTag bugs * fix small regex typo * make YT tag more robust; DRY strip_tags method * Refactor level 1 * Refactor level 2 * Refactor level 3 * array to constant * fix failing vimeo specs - tweak vimeo regexp * PR feedback refactor --- app/liquid_tags/liquid_tag_base.rb | 4 ++ app/liquid_tags/unified_embed_tag.rb | 2 +- app/liquid_tags/vimeo_tag.rb | 20 +++++---- app/liquid_tags/youtube_tag.rb | 56 ++++++++++++-------------- spec/liquid_tags/unified_embed_spec.rb | 27 +++++++++++-- spec/liquid_tags/vimeo_tag_spec.rb | 11 +---- spec/liquid_tags/youtube_tag_spec.rb | 12 +++--- 7 files changed, 75 insertions(+), 57 deletions(-) diff --git a/app/liquid_tags/liquid_tag_base.rb b/app/liquid_tags/liquid_tag_base.rb index ab75d1d81..fc59a0982 100644 --- a/app/liquid_tags/liquid_tag_base.rb +++ b/app/liquid_tags/liquid_tag_base.rb @@ -15,6 +15,10 @@ class LiquidTagBase < Liquid::Tag ) end + def strip_tags(string) + ActionController::Base.helpers.strip_tags(string).strip + end + private def validate_contexts diff --git a/app/liquid_tags/unified_embed_tag.rb b/app/liquid_tags/unified_embed_tag.rb index 56f3af4da..37a975bf8 100644 --- a/app/liquid_tags/unified_embed_tag.rb +++ b/app/liquid_tags/unified_embed_tag.rb @@ -34,7 +34,7 @@ class UnifiedEmbedTag < LiquidTagBase end def render(_context) - link, _options = ActionController::Base.helpers.strip_tags(@markup).split + link, _options = strip_tags(@markup) %(#{link}) end end diff --git a/app/liquid_tags/vimeo_tag.rb b/app/liquid_tags/vimeo_tag.rb index 2304b9249..f9961e23b 100644 --- a/app/liquid_tags/vimeo_tag.rb +++ b/app/liquid_tags/vimeo_tag.rb @@ -1,9 +1,13 @@ class VimeoTag < LiquidTagBase PARTIAL = "liquids/vimeo".freeze + # rubocop:disable Layout/LineLength + REGISTRY_REGEXP = %r{(?:https?://)?(?:player\.|www\.)?vimeo\.com/(?:video/|embed/|watch)?(?:ondemand/\w+/)?(?\d*)} + # rubocop:enable Layout/LineLength def initialize(_tag_name, token, _parse_context) super - @id = id_for(token) + input = strip_tags(token) + @id = get_id(input) @width = 710 @height = 399 end @@ -21,14 +25,14 @@ class VimeoTag < LiquidTagBase private - def id_for(input) - # This was the original plan: - # require "uri" - # File.basename URI(input).path - # But the markdown turns the link into html. This is simple enough, - # works for all the use cases and isn't exploitable. - input.to_s.scan(/\d+/).max_by(&:length) + def get_id(input) + match = input.match(REGISTRY_REGEXP) + # binding.pry + match ? match[:video_id] : input end end Liquid::Template.register_tag("vimeo", VimeoTag) + +# NOTE: this does not process Vimeo Showcase IDs; add to documentation +UnifiedEmbed.register(VimeoTag, regexp: VimeoTag::REGISTRY_REGEXP) diff --git a/app/liquid_tags/youtube_tag.rb b/app/liquid_tags/youtube_tag.rb index cfcd14244..8218c958f 100644 --- a/app/liquid_tags/youtube_tag.rb +++ b/app/liquid_tags/youtube_tag.rb @@ -1,6 +1,11 @@ class YoutubeTag < LiquidTagBase PARTIAL = "liquids/youtube".freeze - REGISTRY_REGEXP = %r{https?://(www\.)?youtube\.(com|be)/(embed|watch)?(\?v=)?(/)?[a-zA-Z0-9_-]{11}((\?t=)?(\d{1,})?)?} + # rubocop:disable Layout/LineLength + REGISTRY_REGEXP = %r{https?://(?:www\.)?(?:youtube\.com|youtu\.be)/(?:embed/|watch\?v=)?(?[a-zA-Z0-9_-]{11})(?:\?|&)?(?:t=|start=)?(?(?:\d{1,}h?)?(?:\d{1,2}m)?(?:\d{1,2}s)?{5,11})?} + VALID_ID_REGEXP = /\A(?[a-zA-Z0-9_-]{11})(?:\?|&)?(?:t=|start=)?(?(?:\d{1,}h?)?(?:\d{1,2}m)?(?:\d{1,2}s)?{5,11})?\Z/ + # rubocop:enable Layout/LineLength + REGEXP_OPTIONS = [REGISTRY_REGEXP, VALID_ID_REGEXP].freeze + MARKER_TO_SECONDS_MAP = { "h" => 60 * 60, "m" => 60, @@ -10,11 +15,9 @@ class YoutubeTag < LiquidTagBase def initialize(_tag_name, id, _parse_context) super - # for if id is an unstripped URL; doesn't appear to affect bare youtube ids - input = ActionController::Base.helpers.strip_tags(id).strip - - @id = parse_id_or_url(input) - @width = 710 + input = CGI.unescape_html(strip_tags(id)) + @id = parse_id_or_url(input) + @width = 710 @height = 399 end @@ -32,42 +35,35 @@ class YoutubeTag < LiquidTagBase private def parse_id_or_url(input) - if (input =~ REGISTRY_REGEXP)&.zero? - extract_youtube_id(input) - else - input_no_space = input.delete(" ") - raise StandardError, "Invalid YouTube ID" unless valid_id?(input_no_space) - return translate_start_time(input_no_space) if input_no_space.include?("?t=") + match = pattern_match_for(input) + raise StandardError, "Invalid YouTube ID" unless match - input_no_space - end + video_id = match[:video_id] + time_parameter = match[:time_parameter] + + return video_id if time_parameter.blank? + + translate_start_time(video_id, time_parameter) end - def extract_youtube_id(url) - url = url.gsub(/(>|<)/, "").split(%r{(vi/|v=|/v/|youtu\.be/|/embed/)}) - raise StandardError, "Invalid YouTube URL" if url[2].nil? - - id = url[2].split(/[^a-zA-Z0-9_-]/i) # tweak this to allow for time, fix youtube_tag_spec - id[0] + def pattern_match_for(input) + REGEXP_OPTIONS + .filter_map { |regex| input.match(regex) } + .first end - def valid_id?(id) - id.match?(/\A[a-zA-Z0-9_-]{11}((\?t=)?(\d{1,}h?)?(\d{1,2}m)?(\d{1,2}s)?){5,11}?\Z/) - end + def translate_start_time(video_id, time_parameter) + return "#{video_id}?start=#{time_parameter}" if time_parameter.match?(/\A\d+\Z/) - def translate_start_time(id) - time = id.split("?t=")[-1] - return "#{id.split('?t=')[0]}?start=#{time}" if time.match?(/\A\d+\Z/) - - time_elements = time.split(/[a-z]/) - time_markers = time.split(/\d+/)[1..] + time_elements = time_parameter.split(/[a-z]/) + time_markers = time_parameter.split(/\d+/)[1..] seconds = 0 time_markers.each_with_index do |m, i| seconds += MARKER_TO_SECONDS_MAP.fetch(m, 0) * time_elements[i].to_i end - "#{id.split('?t=')[0]}?start=#{seconds}" + "#{video_id}?start=#{seconds}" end end diff --git a/spec/liquid_tags/unified_embed_spec.rb b/spec/liquid_tags/unified_embed_spec.rb index 8ce3b99f9..a88ef53f2 100644 --- a/spec/liquid_tags/unified_embed_spec.rb +++ b/spec/liquid_tags/unified_embed_spec.rb @@ -4,6 +4,18 @@ RSpec.describe UnifiedEmbed do subject(:unified_embed) { described_class } describe ".find_liquid_tag_for" do + valid_youtube_url_formats = [ + "https://www.youtube.com/embed/dQw4w9WgXcQ", + "https://www.youtube.com/watch?v=rc5AyncB_Xw&t=18s", + "https://youtu.be/rc5AyncB_Xw", + ] + + valid_vimeo_url_formats = [ + "https://player.vimeo.com/video/652446985?h=a68f6ed1f5", + "https://vimeo.com/ondemand/withchude/647355334", + "https://vimeo.com/636725488", + ] + it "returns GistTag for a gist url" do expect(described_class.find_liquid_tag_for(link: "https://gist.github.com/jeremyf/662585f5c4d22184a6ae133a71bf891a")) .to eq(GistTag) @@ -49,9 +61,18 @@ RSpec.describe UnifiedEmbed do .to eq(WikipediaTag) end - it "returns YoutubeTag for a youtube url" do - expect(described_class.find_liquid_tag_for(link: "https://www.youtube.com/embed/dQw4w9WgXcQ")) - .to eq(YoutubeTag) + valid_youtube_url_formats.each do |url| + it "returns YoutubeTag for a valid youtube url format" do + expect(described_class.find_liquid_tag_for(link: url)) + .to eq(YoutubeTag) + end + end + + valid_vimeo_url_formats.each do |url| + it "returns VimeoTag for a valid vimeo url format" do + expect(described_class.find_liquid_tag_for(link: url)) + .to eq(VimeoTag) + end end end end diff --git a/spec/liquid_tags/vimeo_tag_spec.rb b/spec/liquid_tags/vimeo_tag_spec.rb index 0bf48c792..5fb43b213 100644 --- a/spec/liquid_tags/vimeo_tag_spec.rb +++ b/spec/liquid_tags/vimeo_tag_spec.rb @@ -31,14 +31,7 @@ RSpec.describe VimeoTag, type: :liquid_tag do assert_parses id, "ps://player.vimeo.com/video/#{id}" end - # NOTE: This is kinda dumb. It seems like the right answer is that - # either it should run liquid before markdown, or markdown shouldn't - # mess with the liquid tags (there is a fn to escape them, but it doesn't - # seem to escape the url here) - # https://github.com/thepracticaldev/dev.to/blob/master/app/labor/markdown_parser.rb#L73-L92 - # My test suite isn't entirely passing, and I've spent longer on this than I - # wanted to, io Instead of looking into those, I'm going to just make this work ¯\_(ツ)_/¯ - it "accepts urls that were over-eagerly turned into links by markdown" do - assert_parses id, "https://vimeo.com/192819855 " + it "accepts urls that were turned into links by markdown" do + assert_parses id, "https://vimeo.com/#{id} " end end diff --git a/spec/liquid_tags/youtube_tag_spec.rb b/spec/liquid_tags/youtube_tag_spec.rb index 10ac89c54..14e7018f1 100644 --- a/spec/liquid_tags/youtube_tag_spec.rb +++ b/spec/liquid_tags/youtube_tag_spec.rb @@ -2,8 +2,8 @@ require "rails_helper" RSpec.describe YoutubeTag, type: :liquid_tag do describe "#id" do - let(:valid_id_no_time) { "dQw4w9WgXcQ" } - let(:valid_id_with_time) { "QASbw8_0meM?t=8h12m26s" } + let(:valid_id_no_time) { "fhH5xX_yW6U" } + let(:valid_id_with_time) { "fhH5xX_yW6U?t=0h5m0s" } let(:invalid_id) { Faker::Lorem.characters(number: rand(12..100)) } def generate_new_liquid(id) @@ -16,28 +16,28 @@ RSpec.describe YoutubeTag, type: :liquid_tag do liquid = generate_new_liquid(valid_id_no_time).render expect(liquid).to include('