Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ [RTC-Config]: add source_url and title to real-time-config global macros allowlist #38723

Merged
merged 3 commits into from
Jul 23, 2023

Conversation

zshnr
Copy link
Contributor

@zshnr zshnr commented Mar 24, 2023

Implements the changes proposed in #38627 by adding the TITLE and SOURCE_URL global macros to the allowlist for real-time-config

@zshnr
Copy link
Contributor Author

zshnr commented Apr 19, 2023

Hi @calebcordry , could I have some assistance with getting this PR over the line? :)

@erwinmombay erwinmombay requested review from powerivq and removed request for calebcordry July 19, 2023 19:14
@erwinmombay
Copy link
Member

@zshnr apologies for the delay. could you rebase this issue. we'll try and get this over the line

@zshnr
Copy link
Contributor Author

zshnr commented Jul 20, 2023

@erwinmombay Thank you so much! I have rebased off of the main branch, fixed the one conflict, and pushed the changes :)

@zshnr
Copy link
Contributor Author

zshnr commented Jul 20, 2023

@erwinmombay I might need help debugging the unit tests. Not sure how I can log the output of the function called in the failing test to see in either the browser console or command line.

Is there anyone that can assist with this? Thanks!

@powerivq
Copy link
Contributor

I would start with generalizing your regex and see at which point would the unit test passes.

@zshnr
Copy link
Contributor Author

zshnr commented Jul 22, 2023

@powerivq I have updated the test's regex and it passes.

However, I had to generalize the part of the regex that checks that SOURCE_URL is a valid url with either http or https.

old regex had

src=https?:\/\/[\w\.\/]+

generalised this with:

src=[^&]*

Is this testing for the correct behavior of SOURCE_URL substitution or does this point to SOURCE_URL not substituting the correctly formatted url string?

@powerivq
Copy link
Contributor

@zshnr I think escaping might have occur and failed your regex.

@powerivq powerivq merged commit 810596b into ampproject:main Jul 23, 2023
10 checks passed
eszponder pushed a commit to krzysztofequativ/amphtml that referenced this pull request Apr 22, 2024
…acros allowlist (ampproject#38723)

* add source_url and title to real-time-config global macro allowlist

* add back accidently removed regex token

* fix unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants