N°8952 - Correct variable naming and syntax errors in webhook actions#28
Conversation
|
Hello, thank you for your PR and for correcting errors :) |
|
@jf-cbd You're right, thank you for catching that! and Exception is documented too, my bad....ill update the changes, thanks |
57e37ea to
8cbca0f
Compare
|
Hey @ritoban23, we should approve and merge your PR in the month. |
There was a problem hiding this comment.
Pull request overview
This PR fixes two runtime-breaking issues in the webhook action execution path by correcting an invalid property reference used for error reporting and by fixing a PHP switch syntax error. These changes improve reliability of webhook notifications and prevent notices/parse errors during execution.
Changes:
- Replace references to non-existent
$this->m_aWebrequestErrorswith the existing$this->aRequestErrors. - Fix
switchsyntax by changingcase ...;tocase ...:for the error sender state branch.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Base information
Symptom (bug) / Objective (enhancement)
src/Core/Notification/Action/_ActionWebhook.php, the code referenced$this->m_aWebrequestErrorsinstead of the correct$this->aRequestErrors, causing runtime errors.case WebRequestSender::ENUM_SEND_STATE_ERROR;(semicolon instead of colon).tests/php-unit-tests/_ActionWebhookTest.php, unused imports (Exception,ReflectionException) were present.Reproduction procedure (bug)
With module version x.y.z
With PHP x.y.z
Trigger a webhook action that results in an error (e.g., invalid callback signature).
Observe PHP notices about undefined property or syntax errors in logs.
-->
On iTop 3.x.x
With module version 1.x.x
With PHP 7.4+ or 8.x
Trigger a webhook action that should log errors (e.g., misconfigured callback).
See PHP notice: "Undefined property: ...m_aWebrequestErrors"
See parse error if the switch case is hit.
Cause (bug)
$this->aRequestErrorsProposed solution (bug and enhancement)
_ActionWebhook.php_ActionWebhookTest.phpChecklist before requesting a review
Checklist of things to do before PR is ready to merge