Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
The Webform Remote POST handler provides great coverage for processing error messages, but what about success messages? It appears that the success message is only covered by the webform confirmation settings, which appear to always execute regardless of the success/failure of the handler.
Proposed resolution
Expand current functionality to not be "error-only" and also include successful POST scenarios, especially when the handler is the primary processor for the webform.
Remaining tasks
1. Discuss options
2. Determine next steps
3. Add tests and submit for review
User interface changes
Slight changes to config form field labels
API changes
N/A
Data model changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#15 | webform-3134549-15.patch | 12.36 KB | bighappyface |
| |||
#14 | webform-3134549-14.patch | 15.75 KB | bighappyface |
#11 | 3134549-11.patch | 13.72 KB | jrockowitz |
| |||
#11 | interdiff-3134549-10-11.txt | 1.21 KB | jrockowitz |
#10 | interdiff_4-10.txt | 3.84 KB | bighappyface |
Comments
Comment #2
bighappyface CreditAttribution: bighappyface at Rackspace commentedAttached is a quick patch to apply the custom error message by status code handling to successful response codes too. No tests, but I want to get the conversation started.
Comment #3
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented@bighappyface Your request and patch make a lot of sense and it probably just needs test coverage.
Comment #4
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedThe safest approach is to provide support custom 2xx response messages but do not display the default custom error message for 2xx response. My concern is someone might have configured a custom response error message which would now be displayed for all 2xx responses. For most 2xx response the default confirmation message should be fine.
Comment #5
bighappyface CreditAttribution: bighappyface at Rackspace commented@jrockowitz thanks for the feedback! Please see the updated patch.NVM - I forgot to refresh the page before posting this comment/patch.Comment #6
bighappyface CreditAttribution: bighappyface at Rackspace commented+1 for #4
@jrockowitz I applied the patch locally and tested in context for my needs. Works great! What do you need to commit this enhancement?
Comment #7
bighappyface CreditAttribution: bighappyface at Rackspace commentedComment #8
bighappyface CreditAttribution: bighappyface at Rackspace commented@jrockowitz are we good losing the `addError` here with the relocation to `displayCustomResponseMessage` where it becomes `addMessage`?
Everything comes up as a status message. Perhaps we can check for that in `displayCustomResponseMessage` to switch between status/error?
Comment #9
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented2xx response should be status messages and everything else considered an error message.
Comment #10
bighappyface CreditAttribution: bighappyface at Rackspace commented@jrockowitz I have taken a stab at handling the status/error message states. Please see the attached patch and interdiff.
Comment #11
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedI think this is the right solution. Attached is a minor improvement.
Comment #12
bighappyface CreditAttribution: bighappyface at Rackspace commented@jrockowitz #11 LGTM. I tested it out manually, and everything is good to go.
May we merge this?
Thank you so much for your help.
Comment #13
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #14
bighappyface CreditAttribution: bighappyface at Rackspace commented#11 Patch reroll
Comment #15
bighappyface CreditAttribution: bighappyface at Rackspace commented#11 Patch Re-reroll
Comment #16
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented@bighappyface The patch seemed fine to me so I committed it.
Comment #17
bighappyface CreditAttribution: bighappyface at Rackspace commented@jrockowitz sorry to bug - you committed a patch for this issue? I only see the commit for #3135006: Webform Handler Token String Conversion Notice in the commit log on 8.x-5.x
Comment #19
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented@bighappyface You are right I somehow messed up the commit. I just committed your patch. Thanks.
Comment #20
bighappyface CreditAttribution: bighappyface at Rackspace commented