Skip to content

Conversation

@ritoban23
Copy link

Base information

Question Answer
Related to a SourceForge thead / Another PR / Combodo ticket?
Type of change? Bug fix

Symptom (bug) / Objective (enhancement)

  • In src/Core/Notification/Action/_ActionWebhook.php, the code referenced $this->m_aWebrequestErrors instead of the correct $this->aRequestErrors, causing runtime errors.
  • The same file had a syntax error: case WebRequestSender::ENUM_SEND_STATE_ERROR; (semicolon instead of colon).
  • In tests/php-unit-tests/_ActionWebhookTest.php, unused imports (Exception, ReflectionException) were present.

Reproduction procedure (bug)

  1. With module version x.y.z

  2. With PHP x.y.z

  3. Trigger a webhook action that results in an error (e.g., invalid callback signature).

  4. Observe PHP notices about undefined property or syntax errors in logs.
    -->

  5. On iTop 3.x.x

  6. With module version 1.x.x

  7. With PHP 7.4+ or 8.x

  8. Trigger a webhook action that should log errors (e.g., misconfigured callback).

  9. See PHP notice: "Undefined property: ...m_aWebrequestErrors"

  10. See parse error if the switch case is hit.

Cause (bug)

  • Typo in property name: should be $this->aRequestErrors
  • Syntax error in switch statement: should use colon, not semicolon

Proposed solution (bug and enhancement)

  • Corrected property name in _ActionWebhook.php
  • Fixed switch case syntax
  • Removed unused imports in _ActionWebhookTest.php

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested all changes I made on an iTop instance
  • Would a unit test be relevant and have I added it? (N/A, covered by existing tests)
  • Is the PR clear and detailed enough so anyone can understand without digging in the code?

Checklist of things to do before PR is ready to merge

  • Review requested changes (if any)
  • Confirm all tests pass
  • Update documentation if needed

@jf-cbd
Copy link
Member

jf-cbd commented Oct 30, 2025

Hello, thank you for your PR and for correcting errors :)
I just have a question : ReflectionException seems to be used in the PHPDoc of testCheckCallbackSignature, why do you suggest removing it ?

@ritoban23
Copy link
Author

@jf-cbd You're right, thank you for catching that! and Exception is documented too, my bad....ill update the changes, thanks

@ritoban23 ritoban23 force-pushed the fix/webhook-action-bugs branch from 57e37ea to 8cbca0f Compare October 30, 2025 09:51
@jf-cbd jf-cbd moved this from Hacktoberfest to Pending technical review in Combodo PRs dashboard Nov 3, 2025
@jf-cbd
Copy link
Member

jf-cbd commented Nov 13, 2025

Hey @ritoban23, we should approve and merge your PR in the month.
Your PR is a nice catch, and we'd like to send you stickers (about iTop and Hacktoberfest), as a thank you for your contribution. If you want to receive some, please send us your address at community[at]combodo.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Pending technical review

Development

Successfully merging this pull request may close these issues.

3 participants