Problem/Motivation

In Webform issue #2898424: Improve Remote Post the RemotePostWebformHandler class was improved and method names and functionality was changed. Other improvements have come since.

The webform handler class SalesforceWebToLeadPostWebformHandler is no longer compatible with the latest version of `RemotePostWebformHandler` and must be refactored to work with the latest versions of webform.

Proposed resolution

  1. Refactor SalesforceWebToLeadPostWebformHandler to get back to desired operation
  2. Add some tests to help with regression form changes in webform

Remaining tasks

  1. Refactor SalesforceWebToLeadPostWebformHandler to get back to desired operation
  2. Add some tests to help with regression form changes in webform

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#7 interdiff-2930325-4-7.txt909 bytesjohn cook
#7 refactor-webform-handler-2930325-7.patch14.4 KBjohn cook
#5 refactor-webform-handler-2930325-4.patch14.38 KBbighappyface
#5 interdiff-2930325-3-4.patch7.89 KBbighappyface
#3 refactor-webform-handler-2930325-3.patch14 KBbighappyface
#2 refactor-webform-handler-2930325-2.patch6.73 KBbighappyface

Comments

bighappyface created an issue. See original summary.

bighappyface’s picture

bighappyface commented
StatusFileSize
new refactor-webform-handler-2930325-2.patch6.73 KB

Attached is a patch refactoring SalesforceWebToLeadPostWebformHandler to get things working with the latest version of webform (as of this post) make better use of the superclass RemotePostWebformHandler and the functionality it provides.

I will follow-up with some tests to satisfy the second item listed in the description.

bighappyface’s picture

bighappyface commented
StatusFileSize
new refactor-webform-handler-2930325-3.patch14 KB

Attached is a fresh patch with the introduction of protected method setStateUrl and new kernel test `SalesforceWebToLeadPostWebformHandlerTest`.

The `getRequestData` and `setStateUrl` methods are tested and we have a path for testing other methods in the future.

Note: these changes fix related issues #2896481: Error with latest Webform version (5.0 Beta 15) and #2923161: TypeError: Argument 7 passed to ... debug() must implement interface as the annotation namespaces are updated and the debug method type error issue is resolved by removing the call and deferring to RemotePostWebformHandler::remotePost.

WidgetsBurritos’s picture

WidgetsBurritos commented
Status: Active » Needs work

This all looks good, but minor nitpicks:

  1. +++ b/tests/src/Kernel/Plugin/WebformHandler/SalesforceWebToLeadPostWebformHandlerTest.php
@@ -0,0 +1,185 @@
+    // Get contructor args and mocks.

    constructor* 

  2. +++ b/tests/src/Kernel/Plugin/WebformHandler/SalesforceWebToLeadPostWebformHandlerTest.php
@@ -0,0 +1,185 @@
+    // Mock toekn manager and replace method.

    token* 

  3. +++ b/tests/src/Kernel/Plugin/WebformHandler/SalesforceWebToLeadPostWebformHandlerTest.php
@@ -0,0 +1,185 @@
+    return $this->getMockBuilder('Drupal\Core\Config\ConfigFactoryInterface')->getMock();

    This is inconsistent with the namespace style used above. I'd check all of these below.

bighappyface’s picture

bighappyface commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2930325-3-4.patch7.89 KB
new refactor-webform-handler-2930325-4.patch14.38 KB

Attached is a cleaned up patch with PHPCS violations addressed along with the feedback from @widgetsburritos.

WidgetsBurritos’s picture

WidgetsBurritos commented
Status: Needs review » Reviewed & tested by the community

+1 RTBC

john cook’s picture

john cook commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new refactor-webform-handler-2930325-7.patch14.4 KB
new interdiff-2930325-4-7.txt909 bytes

I've had a look at the patch from @bighappyface and it looks good.

I've made a slight tweak in SalesforceWebToLeadPostWebformHandler::getRequestData(). As count($salesforce_mapping) cannot be greater than count($data), I've swapped the loop so it goes though the smaller number of elements.

bighappyface’s picture

bighappyface commented
Status: Needs review » Reviewed & tested by the community

+1 for #7

Tests are still good and the functionality is unaffected. Good adjustment, @john-cook!

  • aaronbauman committed ff7de40 on 8.x-5.x authored by John Cook 
    Issue #2930325 by bighappyface, John Cook: Refactor Webform Handler per...
aaronbauman’s picture

aaronbauman
he/him
commented
Status: Reviewed & tested by the community » Fixed

Committed - thanks for the patch.
Will push an alpha release soon.

bighappyface’s picture

bighappyface commented

Thanks, @aaronbauman. I am happy to help with reviews on other tasks.

aaronbauman’s picture

aaronbauman
he/him
commented

Cool thanks for the offer.
In case you didn't see, i updated this issue #2878133: 8.x release with links to two others (for now) that should get addressed before a 5.0 stable.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.