You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
Remove ServiceType::VOICE and the 'voice' entry from AppDispatch::$factoryMap.
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:
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.
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.
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.
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.
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
interface/modules/custom_modules/oe-module-faxsms/src/Controller/VoiceClient.php.interface/modules/custom_modules/oe-module-faxsms/templates/phone_widget.html.twig.ServiceType::VOICEand the'voice'entry fromAppDispatch::$factoryMap.Expected: at least one failure pointing at the broken voice rendering path.
Actual: everything passes. The
getApiService('voice')lookup returnsnullat runtime, theRenderEventlisteners silently skip, and the phone button stops appearing — but only on a running instance withoe_enable_voiceenabled and credentials configured. No test exercises this path.Root causes
Three structural gaps let the wiring stay invisible:
VoiceClientextendsAppDispatch, which forces it to declaresendFax/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 thoughgetCredentials()/getVoiceCredentials()are the real, used methods. This is the read that motivated refactor(faxsms): remove unimplemented VoiceClient #12020.AppDispatch::getApiService()resolves through$factoryMap[$type][$s]— astring => int => Closurearray returningmixed. PHPStan can't follow it;grepcan't trace it. The only thing tyingServiceType::VOICE = 9toVoiceClientis the string'voice'. RemovingVoiceClientproduces no compile-time error.NotificationEventListener::renderPhoneWidgetloads 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_NAVwithoe_enable_voiceon 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
BrowserWidgetCredentialProviderinterface with the methods voice actually implements (getCredentials(),getVoiceCredentials()).MessagingChannelinterface for fax/SMS/email.sendFax/sendSMS/sendEmail/fetchReminderCountstubs fromVoiceClient. A class with no stubs can't be misread as unimplemented.2. Typed service registration
Kernel.$container->get(VoiceCredentialProvider::class)).null.3. Template path as a class constant
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:
Equivalents for fax / SMS / email. Plus a meta-test that enumerates
ServiceTypecases 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::VOICEand on the newBrowserWidgetCredentialProviderinterface 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