Skip to content

faxsms: voice channel wiring is invisible to static analysis and tests #12230

@kojiromike

Description

@kojiromike

Bug

The faxsms module's voice channel can be silently removed without any static-analysis or test failure, even though it's a working sponsored feature in production. PR #12020 did exactly that, and nothing in CI caught it. PR #12229 restores the code, but the underlying defect — the wiring is invisible — is still present and will allow the same mistake to recur for voice or any future browser-side channel.

Reproduction

  1. Delete interface/modules/custom_modules/oe-module-faxsms/src/Controller/VoiceClient.php.
  2. Delete interface/modules/custom_modules/oe-module-faxsms/templates/phone_widget.html.twig.
  3. Remove ServiceType::VOICE and the 'voice' entry from AppDispatch::$factoryMap.
  4. Run the full test suite and PHPStan.

Expected: at least one failure pointing at the broken voice rendering path.
Actual: everything passes. The getApiService('voice') lookup returns null at runtime, the RenderEvent listeners silently skip, and the phone button stops appearing — but only on a running instance with oe_enable_voice enabled and credentials configured. No test exercises this path.

Root causes

Three structural gaps let the wiring stay invisible:

  1. VoiceClient extends AppDispatch, which forces it to declare sendFax/sendSMS/sendEmail/fetchReminderCount. Voice can't implement those (it runs in the browser), so it returns '' from all four. The stubs make the class look unimplemented even though getCredentials() / getVoiceCredentials() are the real, used methods. This is the read that motivated refactor(faxsms): remove unimplemented VoiceClient #12020.
  2. AppDispatch::getApiService() resolves through $factoryMap[$type][$s] — a string => int => Closure array returning mixed. PHPStan can't follow it; grep can't trace it. The only thing tying ServiceType::VOICE = 9 to VoiceClient is the string 'voice'. Removing VoiceClient produces no compile-time error.
  3. NotificationEventListener::renderPhoneWidget loads the template via a string literal ($this->twig->render('phone_widget.html.twig', ...)). Deleting the template file produces no error until a real request reaches the listener with the right globals.

There is also no end-to-end test that dispatches RenderEvent::EVENT_BODY_RENDER_NAV with oe_enable_voice on and asserts the button HTML renders, so even the runtime behavior is unobserved by CI.

Fix

Each item below closes one of the gaps. They can land as separate PRs.

1. Split the channel contracts

  • Define a BrowserWidgetCredentialProvider interface with the methods voice actually implements (getCredentials(), getVoiceCredentials()).
  • Define a MessagingChannel interface for fax/SMS/email.
  • Make each client implement only the interface it satisfies. Delete the sendFax/sendSMS/sendEmail/fetchReminderCount stubs from VoiceClient. A class with no stubs can't be misread as unimplemented.

2. Typed service registration

  • Register each client in the Symfony container the module already accesses via Kernel.
  • Resolve via typed accessors (e.g., $container->get(VoiceCredentialProvider::class)).
  • Removing the class then produces a compile-time / container-build failure instead of a silent null.

3. Template path as a class constant

final class VoiceClient {
    public const WIDGET_TEMPLATE = 'phone_widget.html.twig';
}
// listener:
$this->twig->render(VoiceClient::WIDGET_TEMPLATE, $ctx);

Plus a tiny test asserting the file exists. Deleting the template then fails CI.

4. One render smoke test per channel

The single test that would have failed #12020:

Given oe_enable_voice = 9 and seeded credentials, dispatch RenderEvent::EVENT_BODY_RENDER_NAV and assert the output contains the rc-toggle-exe button id.

Equivalents for fax / SMS / email. Plus a meta-test that enumerates ServiceType cases and asserts each has (a) a registered service in the container, (b) a setup page route, (c) a render smoke test. Catches half-wired channels in both directions.

5. Document the architecture

Docblock on ServiceType::VOICE and on the new BrowserWidgetCredentialProvider interface stating: voice runs in the browser via RingCentral Embeddable; the server only mints JWT credentials. Cheap insurance on top of the structural fixes — not a substitute.

References

Metadata

Metadata

Assignees

No one assigned

    Labels

    Module SupportUse if affects modulestestingcreating, fixing and enhancing tests and the testing process

    Type

    No fields configured for Bug.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions