fix: disallow duplicate PMIs when creating request

w3c / payment-request

Payment Request API

https://www.w3.org/TR/payment-request/

Other

489 stars 135 forks source link

fix: disallow duplicate PMIs when creating request #908

Closed marcoscaceres closed 4 years ago

marcoscaceres commented 4 years ago

closes #905

The following tasks have been completed:

[X] Confirmed there are no ReSpec errors/warnings.
[X] Modified Web platform tests
[X] Modified MDN Docs - edge case, let's not bother.
[X] Has undergone security/privacy review - part of #905

Implementation commitment:

[x] Safari
[x] Chrome
[x] Firefox
[ ] Edge (public signal)

Optional, impact on Payment Handler spec?

None.

Preview | Diff

marcoscaceres commented 4 years ago

We have a patch ready to go on the Firefox side... @edenchuang was also wondering what we should do about duplicate payment method modifiers for the same PMI... we have a last one wins algo there, the maybe we should start throwing for those too (we can do that as a followup).

danyao commented 4 years ago

We have a patch ready to go on the Firefox side... @edenchuang was also wondering what we should do about duplicate payment method modifiers for the same PMI... we have a last one wins algo there, the maybe we should start throwing for those too (we can do that as a followup).

I went to look for why we decided on "last one wins" before and found these two threads: https://github.com/w3c/payment-request/issues/684 https://github.com/w3c/payment-request/pull/581

IIUC, the main thrust for supporting multiple modifiers for the same PMI is because of basic-card. A merchant may want to specify a different modifier for different supportedNetworks of basic-card.

While I think it'll be conceptually cleaner if the spec treats method data and method modifier consistently, the basic-card use case seems legitimate. So I think we should keep the current "last one wins" algorithm and not throw.

marcoscaceres commented 4 years ago

This is now implemented in Gecko and we added a Web Platform test for it: https://github.com/web-platform-tests/wpt/commit/8bdfc7451d7ad917ebabefc3bedab5f49e0eee5d

@danyao or @rsolomakhin, would you mind providing a tracking chrome bug? I think then we are good to merge this.

marcoscaceres commented 4 years ago

While I think it'll be conceptually cleaner if the spec treats method data and method modifier consistently, the basic-card use case seems legitimate. So I think we should keep the current "last one wins" algorithm and not throw.

Agree, with respect to modifiers.

rsolomakhin commented 4 years ago

https://crbug.com/1085712

marcoscaceres commented 4 years ago

Awesome! @danyao, do you want to give this a last review? I need a ✅ to merge.

marcoscaceres commented 3 years ago

Was fixed in WebKit by https://bugs.webkit.org/show_bug.cgi?id=220824

ianbjacobs commented 3 years ago

Thanks!!