🐛Fix data-amp-replace comparison logic
#19069
Conversation
|
Working on writing test now, but wanted to get input on the fix. Looks like this test might have caught it but was skipped :( amphtml/test/functional/test-url-replacements.js Line 1311 in 3a29c54 |
src/service/url-replacements-impl.js
Outdated
| const varNames = Object.keys(vars); | ||
| const varNames = Object.keys(vars) | ||
| // Strip arguments from collect_vars to match whitelist api. | ||
| .map(key => key.substring(0, key.indexOf('('))); |
There was a problem hiding this comment.
I agree. A larger refactor is needed to be less so. Wasn't sure if we should patch and clean up after.
There was a problem hiding this comment.
indeed. is it possible to have spaces before (?
chatted offline, a better fix would redefine this collectUnwhitelistedVarsSync method, and move whitelist logic to the caller.
I'm fine with a quick fix like this to patch prod.
There was a problem hiding this comment.
Should not be possible to have spaces before arguments.
There was a problem hiding this comment.
I thought we allow /path/CLIENT_ID (xxx) ? or did it get normalized somewhere down the path?
There was a problem hiding this comment.
We construct these in maybeCollectVars_ with the macro name coming from the macro definition, not the url.
* remove args * add test * no str.includes
* remove args * add test * no str.includes
Fixes #19061
The new expander had inadvertently introduced a new API that
UrlReplacements.collectUnwhitelistedVarsSync_had become dependent on. When the API was changed back in #18554 it broke the intended functionality of this method.This removes the arguments collected by the
collectVarsobject before comparing them to the givendata-amp-replaceoptions.