🐛 Fix `value_regex` for `amp-tiktok@data-src` to allow repeating digits and a TikTok URL #35329

westonruter · 2021-07-21T03:14:44Z

This fixes a typo introduced in #33999, where the value_regex for data-src was specified to allow only a single digit whereas it should instead allow any number of digits. It's also supposed to allow a TikTok URL as seen here:

amphtml/extensions/amp-tiktok/0.1/test/validator-amp-tiktok.html

Lines 37 to 45 in f0a90fa

    
           <!-- Valid --> 
        
           <amp-tiktok width="500" height="500" data-src="6943753342808034566"> 
        
           </amp-tiktok> 
        
           <!-- Valid --> 
        
           <amp-tiktok 
        
             width="700" 
        
             height="800" 
        
             data-src="https://www.tiktok.com/@scout2015/video/6943753342808034566"> 
        
           </amp-tiktok>

Neither of these examples are currently valid, although they both properly construct as seen in the AMP Playground.

Originally discovered by @pierlon in ampproject/amp-wp#6436 (comment).

Closes #35514.

amp-owners-bot · 2021-07-21T03:14:50Z

Hey @ampproject/wg-caching! These files were changed:

extensions/amp-tiktok/0.1/test/validator-amp-tiktok.out
extensions/amp-tiktok/validator-amp-tiktok.protoascii

westonruter · 2021-07-21T03:20:40Z

I'm missing something here, namely that data-src is also supposed to be a URL:

amphtml/extensions/amp-tiktok/0.1/test/validator-amp-tiktok.html

Lines 40 to 45 in f0a90fa

    
           <!-- Valid --> 
        
           <amp-tiktok 
        
             width="700" 
        
             height="800" 
        
             data-src="https://www.tiktok.com/@scout2015/video/6943753342808034566"> 
        
           </amp-tiktok>

This isn't valid in the validator now either, although the component initializes properly.

UPDATE: This is fixed in ad667c7, although it doesn't constrain that the value is actually a valid TikTok URL.

westonruter · 2021-07-21T03:33:01Z

extensions/amp-tiktok/validator-amp-tiktok.protoascii

@@ -32,7 +32,7 @@ tags: {  # <amp-tiktok>
  attr_lists: "extended-amp-global"
  attrs: {
    name: "data-src";
-    value_regex: "\\d";
+    value_regex: "(.+/)?\\d+/?";


It would seem that a value_url would be better for the case of a TikTok URL value, but given that the amp-tiktok component is only parsing the ID out of the URL, this doesn't seem required.

amphtml/extensions/amp-tiktok/0.1/amp-tiktok.js

Lines 168 to 170 in f0a90fa

// If the user provides a src attribute extract the video id from the src

const videoIdRegex = /^((.+\/)?)(\d+)\/?$/;

this.videoId_ = src.replace(videoIdRegex, '$3');

extensions/amp-tiktok/validator-amp-tiktok.protoascii

honeybadgerdontcare

lgtm after nit below

…segment

…se URL

westonruter · 2021-07-23T22:37:13Z

The validator test is failing but I don't understand why: https://app.circleci.com/pipelines/github/ampproject/amphtml/13593/workflows/b6c599af-9a71-47b9-b05c-4f77aa1837c8/jobs/222119

I've run python3 build.py --update_tests locally and confirmed the amp-tiktok/0.1/test/validator-amp-tiktok.out file is as expected. However, on CircleCI it doesn't seem to have the update to the protoascii, and it thinks that the validator-amp-tiktok.out file should include:

diff --git a/extensions/amp-tiktok/0.1/test/validator-amp-tiktok.out b/extensions/amp-tiktok/0.1/test/validator-amp-tiktok.out
index 3ba71070d..6ceca814a 100644
--- a/extensions/amp-tiktok/0.1/test/validator-amp-tiktok.out
+++ b/extensions/amp-tiktok/0.1/test/validator-amp-tiktok.out
@@ -40,6 +40,8 @@ FAIL
 |    </amp-tiktok>
 |    <!-- Valid -->
 |    <amp-tiktok
+>>   ^~~~~~~~~
+amp-tiktok/0.1/test/validator-amp-tiktok.html:41:2 Tag 'amp-tiktok' must have a minimum of 1 child tags - saw 0 child tags. (see https://amp.dev/documentation/components/amp-tiktok)
 |      width="700"
 |      height="800"
 |      data-src="https://www.tiktok.com/@scout2015/video/6943753342808034566">

For some reason the value_regex is not matching when CircleCI runs the test.

westonruter · 2021-07-26T20:47:08Z

@honeybadgerdontcare Any idea what's wrong?

rbeckthomas · 2021-07-26T21:58:53Z

must have a minimum of 1 child tags - saw 0 child tags.

I suspect this is being caused by an error with how I set up the blockquote child. So for the spec_name: 'AMP_TIKTOK blockquote' has a mandatory_min_num_child_tags of 1, which i think is what is triggering this error.
However it seems like this is being applied across the board, so maybe that needs to be changed?

westonruter · 2021-07-26T23:10:32Z

I suspect this is being caused by an error with how I set up the blockquote child. So for the spec_name: 'AMP_TIKTOK blockquote' has a mandatory_min_num_child_tags of 1, which i think is what is triggering this error.

To me it seems that rule is fine. It's just that that spec rule is being matched because the rule with data-src is failing to match. But I don't see why.

extensions/amp-tiktok/0.1/test/validator-amp-tiktok.out

honeybadgerdontcare · 2021-08-03T21:49:16Z

I updated the regex to escape the . for the domain name. It looks like circleci is unhappy with something else though and maybe the PR needs to be rebased.

#!/bin/bash -eo pipefail
curl -sS https://raw.githubusercontent.com/ampproject/amphtml/main/.circleci/compute_merge_commit.sh | bash
bash: line 1: 404:: command not found

Exited with code exit status 127
CircleCI received exit code 127

I have introduced this change in another PR to move it forward: #35514

…tok-validation * 'main' of github.com:ampproject/amphtml: (72 commits) build: run amp lint --fix to address import order of jixie (ampproject#35513) ✨ [amp-analytics] Add Custom Browser Event Tracker (ampproject#35193) babel: teach amp mode transformer about #core/mode (ampproject#35477) 🚮 Remove experiment `amp-consent-granular-consent` (ampproject#35508) ♻️ Enable auto-sorting+grouping within src/ and 3p/ (ampproject#35454) 🐛 [amp-render] fix root-element stripping from amp-render with amp-bind (ampproject#35449) ✅ [Story interactive] Add Example Story for Detailed Results Component (ampproject#35450) 🐛 Fix error on bento example (ampproject#35490) 🐛 amp-story-grid-layer: Fix AMP invalidation error in documentation (ampproject#35503) 🐛 Fix code formatting (ampproject#35499) ✅ buildDom: add tests for amp-fit-text and amp-layout (ampproject#35494) ♻️ Remove unused imports (ampproject#35435) ✨ amp-connatix-player: iframe domain based on a property (ampproject#35179) Updated document with use cases of remote config (ampproject#35496) AMP.goBack: update documentation (ampproject#29290) 🏗 Allow the bundle-size job to run even if the builds were skipped (ampproject#35492) build-system: improve terser/esbuild integration (ampproject#35466) 🧪 [Story performance] Disable animations on first page to 50% (ampproject#35476) 📖 [Amp story] [Page attachments] Amp.dev Docs for New Page Attachment Features ampproject#34883 (ampproject#35338) 🚀 [Story interactive] Rewrite Image URL to Cached URL for Image Quizzes and Polls (ampproject#35375) ...

westonruter · 2021-08-03T22:22:38Z

@honeybadgerdontcare Updating to the latest from main fixed the build problem.

westonruter added the WG: components label Jul 21, 2021

westonruter requested a review from rbeckthomas July 21, 2021 03:14

This was referenced Jul 21, 2021

Update amphtml spec to ed0504b ampproject/amp-wp#6436

Merged

✨ [amp-tiktok] Add validation rules for the amp-tiktok component. #33999

Merged

westonruter changed the title ~~🐛 Fix value_regex for amp-tiktok@data-src to allow repeating digits~~ 🐛 Fix value_regex for amp-tiktok@data-src to allow repeating digits and a URL Jul 21, 2021

westonruter commented Jul 21, 2021

View reviewed changes

honeybadgerdontcare requested changes Jul 21, 2021

View reviewed changes

extensions/amp-tiktok/validator-amp-tiktok.protoascii Outdated Show resolved Hide resolved

westonruter requested a review from honeybadgerdontcare July 23, 2021 04:36

honeybadgerdontcare reviewed Jul 23, 2021

View reviewed changes

extensions/amp-tiktok/validator-amp-tiktok.protoascii Outdated Show resolved Hide resolved

honeybadgerdontcare approved these changes Jul 23, 2021

View reviewed changes

rbeckthomas approved these changes Jul 23, 2021

View reviewed changes

westonruter enabled auto-merge (squash) July 23, 2021 18:29

westonruter changed the title ~~🐛 Fix value_regex for amp-tiktok@data-src to allow repeating digits and a URL~~ 🐛 Fix value_regex for amp-tiktok@data-src to allow repeating digits and a TikTok URL Jul 23, 2021

westonruter added a commit to ampproject/amp-wp that referenced this pull request Jul 23, 2021

Update value_regex for amp-tiktok from ampproject/amphtml#35329

f8e0a3e

westonruter added 3 commits July 23, 2021 15:18

Fix value_regex for amp-tiktok@data-src to allow repeating digits

8248e82

Expand value_regex to allow a URL containing digits in its last path …

f3bf0bb

…segment

Update value_regex to allow digits optionally preceded by a TikTok ba…

c7cb81d

…se URL

westonruter force-pushed the fix/amp-tiktok-validation branch from de808bf to c7cb81d Compare July 23, 2021 22:19

twifkak added the WG: caching label Jul 27, 2021

Gregable requested changes Jul 28, 2021

View reviewed changes

extensions/amp-tiktok/0.1/test/validator-amp-tiktok.out Show resolved Hide resolved

Update escapes

9c28f10

westonruter requested a review from Gregable August 3, 2021 22:25

Gregable approved these changes Aug 3, 2021

View reviewed changes

westonruter merged commit 4c7d5d7 into ampproject:main Aug 3, 2021

westonruter deleted the fix/amp-tiktok-validation branch August 3, 2021 22:30

westonruter added a commit to ampproject/amp-wp that referenced this pull request Aug 4, 2021

Update value_regex for amp-tiktok from ampproject/amphtml#35329

d910128

ampprojectbot added the PR use: In Beta / Experimental label Aug 11, 2021

pierlon pushed a commit to pierlon/amp-wp that referenced this pull request Aug 15, 2021

Update value_regex for amp-tiktok from ampproject/amphtml#35329

1aabf00

ampprojectbot added the PR use: In Stable label Aug 17, 2021

ampprojectbot added the PR use: In LTS label Sep 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix `value_regex` for `amp-tiktok@data-src` to allow repeating digits and a TikTok URL #35329

🐛 Fix `value_regex` for `amp-tiktok@data-src` to allow repeating digits and a TikTok URL #35329

westonruter commented Jul 21, 2021 •

edited

Loading

amp-owners-bot bot commented Jul 21, 2021 •

edited by ampprojectbot

Loading

westonruter commented Jul 21, 2021 •

edited

Loading

westonruter Jul 21, 2021 •

edited

Loading

honeybadgerdontcare left a comment

westonruter commented Jul 23, 2021

westonruter commented Jul 26, 2021

rbeckthomas commented Jul 26, 2021

westonruter commented Jul 26, 2021

honeybadgerdontcare commented Aug 3, 2021

westonruter commented Aug 3, 2021

	<!-- Valid -->
	<amp-tiktok width="500" height="500" data-src="6943753342808034566">
	</amp-tiktok>
	<!-- Valid -->
	<amp-tiktok
	width="700"
	height="800"
	data-src="https://www.tiktok.com/@scout2015/video/6943753342808034566">
	</amp-tiktok>

	// If the user provides a src attribute extract the video id from the src
	const videoIdRegex = /^((.+\/)?)(\d+)\/?$/;
	this.videoId_ = src.replace(videoIdRegex, '$3');

🐛 Fix value_regex for amp-tiktok@data-src to allow repeating digits and a TikTok URL #35329

🐛 Fix value_regex for amp-tiktok@data-src to allow repeating digits and a TikTok URL #35329

Conversation

westonruter commented Jul 21, 2021 • edited Loading

amp-owners-bot bot commented Jul 21, 2021 • edited by ampprojectbot Loading

westonruter commented Jul 21, 2021 • edited Loading

westonruter Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

honeybadgerdontcare left a comment

Choose a reason for hiding this comment

westonruter commented Jul 23, 2021

westonruter commented Jul 26, 2021

rbeckthomas commented Jul 26, 2021

westonruter commented Jul 26, 2021

honeybadgerdontcare commented Aug 3, 2021

westonruter commented Aug 3, 2021

🐛 Fix `value_regex` for `amp-tiktok@data-src` to allow repeating digits and a TikTok URL #35329

🐛 Fix `value_regex` for `amp-tiktok@data-src` to allow repeating digits and a TikTok URL #35329

westonruter commented Jul 21, 2021 •

edited

Loading

amp-owners-bot bot commented Jul 21, 2021 •

edited by ampprojectbot

Loading

westonruter commented Jul 21, 2021 •

edited

Loading

westonruter Jul 21, 2021 •

edited

Loading