Skip to content

N°8952 - fix: correct variable naming and syntax errors in webhook actions#28

Open
ritoban23 wants to merge 1 commit into
Combodo:masterfrom
ritoban23:fix/webhook-action-bugs
Open

N°8952 - fix: correct variable naming and syntax errors in webhook actions#28
ritoban23 wants to merge 1 commit into
Combodo:masterfrom
ritoban23:fix/webhook-action-bugs

Conversation

@ritoban23
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@jf-cbd jf-cbd changed the title fix: correct variable naming and syntax errors in webhook actions N°8952 - fix: correct variable naming and syntax errors in webhook actions Nov 27, 2025
@jf-cbd jf-cbd self-requested a review November 27, 2025 13:38
@CombodoApplicationsAccount CombodoApplicationsAccount moved this from Pending technical review to Pending review in Combodo PRs dashboard May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Pending review

Development

Successfully merging this pull request may close these issues.

4 participants