Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Watch out for failures to parse webhooks #7578

Merged
merged 1 commit into from Aug 17, 2019

Conversation

aspiers
Copy link
Contributor

@aspiers aspiers commented Jun 2, 2019

Description

Log useful messages just in case anything goes wrong with parsing of the webhook payload. In theory this should never happen, but defensive programming just in
case is usually a good idea.

Steps to test this PR:

  1. Load up this PR
  2. Enable debugging if possible
  3. Send an invalid payload to the webhook endpoint
Q A
Bug fix? N
New feature? N
Automated tests included? N
Related user documentation PR URL n/a
Related developer documentation PR URL n/a
Issues addressed (#s or URLs) n/a
BC breaks? none
Deprecations? none

@virgilwashere
Copy link
Contributor

"if [[ ${TRAVIS_PHP_VERSION:0:3} == "7.2" ]]; then bin/php-cs-fixer fix -v --dry-run --diff; fi" exited with 8.
  Exit codes
  ----------

  Exit code is built using following bit flags:

  *  0 OK.
  *  1 General error (or PHP/HHVM minimal requirement not matched).
  *  4 Some files have invalid syntax (only in dry-run mode).
  *  8 Some files need fixing (only in dry-run mode).
  * 16 Configuration error of the application.
  * 32 Configuration error of a Fixer.
  * 64 Exception raised within the application.

In theory this should never happen, but defensive programming just in
case is usually a good idea.
@npracht npracht added the ready-to-test PR's that are ready to test label Jun 25, 2019
Copy link
Contributor

@virgilwashere virgilwashere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have successfully implemented this change on multiple production deployments.

Does what it says on the box.

@kuzmany kuzmany added pending-test-confirmation PR's that require one test before they can be merged enhancement Any improvement to an existing feature or functionality and removed ready-to-test PR's that are ready to test labels Jul 22, 2019
@npracht npracht added this to the 2.16.0 milestone Aug 15, 2019
@npracht npracht added this to Ready to Test (confirmation) in Mautic 2 Aug 15, 2019
Copy link
Contributor

@Woeler Woeler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Mautic 2 automation moved this from Ready to Test (confirmation) to Ready to Test (first time) Aug 17, 2019
@Woeler Woeler removed the pending-test-confirmation PR's that require one test before they can be merged label Aug 17, 2019
@Woeler Woeler merged commit 0617c33 into mautic:staging Aug 17, 2019
Mautic 2 automation moved this from Ready to Test (first time) to Merged Aug 17, 2019
@Woeler Woeler modified the milestones: 2.16.0, 2.15.3 Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any improvement to an existing feature or functionality
Projects
No open projects
Mautic 2
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants