Allow variable substitutions in hidden fields. #6130
Conversation
| for (let i = 0; i < varSubsFields.length; i++) { | ||
| const variable = varSubsFields[i].getAttribute('default-value'); | ||
| varSubPromises.push( | ||
| this.urlReplacement_.expandAsync(variable).then(value => { |
There was a problem hiding this comment.
Careful, UrlReplacements isn't a generic variable substitution library. I added an assert that the "protocol" of the url remains the same (because they were all URLs before now), but that doesn't apply here. So if the default value had something like RANDOM:X:Y, this would break because of the changing "protocol".
We should split UrlReplacements into a core class (VarSubstitutions?), and leave the URL specific code out of it.
There was a problem hiding this comment.
Yeah that makes sense.
I can get started in moving VarSubstitutions to a separate service. My guess is for any call to that service directly (outside of url-replacement-impl.js) shouldn't allow URL replacements then?
There was a problem hiding this comment.
My guess is for any call to that service directly (outside of url-replacement-impl.js) shouldn't allow URL replacements then?
Hm?
There was a problem hiding this comment.
I meant to say that we'd need a way to prevent direct calls to var-sub service with a URL to avoid circumventing the protocol assert.
There was a problem hiding this comment.
We can presubmit and whitelist Forms and UrlReplacements.
| * @private | ||
| */ | ||
| waitOnPromisesOrTimeout_(promises, timeout) { | ||
| return Promise.race( |
There was a problem hiding this comment.
Timer#timeoutPromise takes a second "race" promise.
There was a problem hiding this comment.
That one rejects though when timeout happens, I guess I could call the same callback in both then and catch clauses, or is there a better way to do this?
There was a problem hiding this comment.
Ahh, this is fine then.
| this.renderTemplate_(error.responseJson || {}); | ||
| rethrowAsync('Form submission failed:', error); | ||
| // Wait until all variables have been substituted or 100ms timeout. | ||
| return this.waitOnPromisesOrTimeout_(varSubPromises, 100).then(() => { |
There was a problem hiding this comment.
This changes the method signature from void return. Consider returning null in other cases and updating return type to ?Promise.
There was a problem hiding this comment.
Dropped this return since we don't use it.
| // Wait until all variables have been substituted or 100ms timeout. | ||
| return this.waitOnPromisesOrTimeout_(varSubPromises, 100).then(() => { | ||
| if (isHeadOrGet) { | ||
| xhrUrl = addParamsToUrl(this.xhrAction_, this.getFormAsObject_()); |
There was a problem hiding this comment.
Nit: This would be more readable if xhrUrl was declared closer to its usage and the isHeadOrGet conditional wasn't duped below for body. E.g.
let xhrUrl, body;
if (isHeadOrGet) {
xhrUrl = addParamsToUrl(this.xhrAction_, this.getFormAsObject_());
body = new FormData(this.form_);
} else {
xhrUrl = this.xhrAction_;
}
return this.xhr_.fetchJson(xhrUrl, {
body: body,
| @@ -91,6 +99,7 @@ describe('amp-form', () => { | |||
| sandbox.restore(); | |||
| }); | |||
|
|
|||
| // asd | |||
|
PTAL 👀 |
|
@choumx @jridgewell this is ready for another 👀 |
| @@ -201,47 +210,86 @@ export class AmpForm { | |||
| return; | |||
| } | |||
|
|
|||
| const isVarSubExpOn = isExperimentOn(this.win_, 'amp-form-var-sub'); | |||
| // Fields that support var substitutions. | |||
| const varSubsFields = !isVarSubExpOn ? [] : this.form_.querySelectorAll( | |||
Fix #5654