Skip to content

fix: validate dynamic method calls in ProcessHandler and TaskHandler (#153)#174

Merged
s2x merged 2 commits intomasterfrom
fix/validate-dynamic-method-calls-153
Apr 30, 2026
Merged

fix: validate dynamic method calls in ProcessHandler and TaskHandler (#153)#174
s2x merged 2 commits intomasterfrom
fix/validate-dynamic-method-calls-153

Conversation

@s2x
Copy link
Copy Markdown
Collaborator

@s2x s2x commented Apr 30, 2026

Summary

Fix #153 — ProcessHandler and TaskHandler now validate the service method format and check method existence before dynamic invocation, preventing worker crashes with clear error messages.

Changes

Shared helper (src/Util/ServiceMethodHelper.php)

  • New ServiceMethodHelper::split() validates the serviceId::methodName format and returns [serviceId, method]
  • Throws InvalidArgumentException on malformed input (missing ::, empty parts)

ProcessHandler & TaskHandler

  • Format validation stays outside try block (fail-fast on config errors)
  • Method existence check (method_exists()) moved inside try block — caught by catch (Throwable) and dispatched as error event instead of crashing the worker
  • Runtime exceptions from the service method continue to be caught gracefully

Tests added (22 total)

  • ServiceMethodHelperTest: valid formats, invalid formats (5 variants)
  • ProcessHandlerTest: success, missing method, runtime exception, invalid format
  • TaskHandlerTest: success, missing method, runtime exception, invalid format

@s2x
Copy link
Copy Markdown
Collaborator Author

s2x commented Apr 30, 2026

Code Review

1. Validation exceptions thrown outside try-catch (Medium)

The new validation logic in both ProcessHandler and TaskHandler throws \InvalidArgumentException before the existing try/catch block. This means:

  • Invalid format (serviceMethod without ::) or non-existent method → exception propagates up and can still crash the worker
  • Method throws exception at runtime → caught by catch (\Throwable $e) and error event dispatched (existing behavior)

The goal of #153 is to prevent worker crashes, but this implementation may still crash the worker — just with a different exception type.

Two possible fixes:

  • If the intent is graceful handling: move the validation inside the try block so errors are caught and dispatched via the error event
  • If the intent is fail-fast on config errors: this is acceptable, but the PR description claiming \Error is "uncatchable" is misleading — catch (\Throwable $e) does catch \Error. Can you clarify what actual error was observed and why it wasn't caught?

2. Missing tests (Medium)

New validation paths lack test coverage for:

  • Malformed format (missing ::, empty service ID, empty method name)
  • Non-existent method on resolved service
  • Valid method calls still work correctly

3. Duplicated validation logic (Low)

The same explode + validation logic is duplicated between ProcessHandler and TaskHandler. Consider extracting to a shared helper to avoid drift.

Extract shared validation into ServiceMethodHelper and move method
existence check inside try-catch for graceful error handling.

ProcessHandler and TaskHandler now:
- Validate service method format (serviceId::methodName) via shared helper
- Check method_exists() inside try-catch, dispatching error events
  instead of crashing the worker
- Gracefully handle runtime exceptions from the service method

Adds unit tests for:
- ServiceMethodHelper (valid/invalid formats)
- ProcessHandler (success, missing method, runtime error, invalid format)
- TaskHandler (success, missing method, runtime error, invalid format)

Closes #153
@s2x s2x force-pushed the fix/validate-dynamic-method-calls-153 branch from 87fb693 to 5b95e15 Compare April 30, 2026 22:04
@s2x
Copy link
Copy Markdown
Collaborator Author

s2x commented Apr 30, 2026

@s2x Dzięki za review! Wszystkie 3 uwagi są poprawione:

1. Walidacja w try-catch

method_exists() przeniesiony do środka try bloku. Jeśli metoda nie istnieje, InvalidArgumentException jest łapany przez catch (\Throwable $e) i dispatchowany jako event błędu — zamiast crashować workera.

Walidacja formatu (explode) została na zewnątrz — to config error, fail-fast jest tam właściwy.

2. Testy

Dodane 22 testy:

  • ServiceMethodHelperTest: 7 testów (poprawne formaty + błędne)
  • ProcessHandlerTest: 5 testów (sukces, brak metody, runtime error, zły format)
  • TaskHandlerTest: 5 testów (analogicznie)

3. Wydzielona logika

Utworzony src/Util/ServiceMethodHelper::split() — statyczna metoda walidująca format serviceId::methodName, używana przez oba handlery.

@s2x s2x merged commit 02e3ef0 into master Apr 30, 2026
22 checks passed
@s2x s2x deleted the fix/validate-dynamic-method-calls-153 branch April 30, 2026 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ProcessHandler and TaskHandler call dynamic method without validation

1 participant