The following has been filling up my logs:

Incoming push notification did not contain the proper subdomain key.

Given the way that RecurlyPushListenerController::processPushNotification() was written, there's a missing else clause around the response at the end so the error code is always sent.

CommentFileSizeAuthor
#4 interdiff-2758709-3-4.txt586 bytesmarkdorison
#4 responses_to_webhook-2758709-4.patch9.2 KBmarkdorison
#3 interdiff-2758709-2-3.txt602 bytestommycox
#3 recurly-stop_sending_error_code_if_no_error-2758709-3.patch9.66 KBtommycox
#2 recurly-stop_sending_error_code_if_no_error-2758709-2.patch9.47 KBcolan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

colan created an issue. See original summary.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented
Status: Active » Needs review
FileSize
recurly-stop_sending_error_code_if_no_error-2758709-2.patch9.47 KB

I did the following:

  • Moved the error reporting to the top of the method.
  • It fires if an only if there actually is an error.
  • If the error is fired, the method exits so that code indenting beneath (most of the method) is reduced.
  • Method now returns 200 OK error code at end if there are no problems.
  • Removed the extraneous $notification = NULL; at the top of the method.
  • Added a better comment on checking the subdomain: // Verify that the subdomain matches the configured one if it was specified.
tommycox’s picture

tommycox CreditAttribution: tommycox as a volunteer commented
Status: Needs review » Reviewed & tested by the community
FileSize
recurly-stop_sending_error_code_if_no_error-2758709-3.patch9.66 KB
interdiff-2758709-2-3.txt602 bytes

The above patch fixes the issue and cleans up the code significantly. Thanks @colan! I've simply added a PHPDoc comment for the return argument per coding standards.

markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison at Chromatic commented
Status: Reviewed & tested by the community » Needs review
FileSize
responses_to_webhook-2758709-4.patch9.2 KB
interdiff-2758709-3-4.txt586 bytes

Updated @response documentation to communicate that an HtmlResponse (of some type) is always expected. Does that read OK to everyone?

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented

HtmlResponse is already specified in the @return line so it's somewhat redundant, but I'm fine with keeping it in for emphasis.

tommycox’s picture

tommycox CreditAttribution: tommycox as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. I was hoping someone would update that line in fact. I find often that the additional comment feels redundant but is necessary for code sniffers.

  • markdorison committed 689721a on 8.x-1.x authored by colan 
    Issue #2758709 by Tommy Cox, markdorison, colan: Responses to webhook...
markdorison’s picture

markdorison
he/him
CreditAttribution: markdorison at Chromatic commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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