Services\SessionLimit should type-hint dependencies using interfaces

Indeed, it is easier because of the changes in 9.4.

IMO this should be changed in the Session Limit itself, type hinting to an interface rather than implementation is better, also there are other projects that decorate the module handler.

Upgrading to 3.3 from 3.2 is also giving me errors. I fixed some by patching facets (https://www.drupal.org/project/facets/issues/3308343). This replaces ModuleHandler with ModuleHandlerInterface.

The website encountered an unexpected error. Please try again later.
TypeError: Argument 2 passed to Drupal\openid_connect\OpenIDConnectClaims::__construct() must be an instance of Drupal\Core\Extension\ModuleHandler, instance of Drupal\hook_event_dispatcher\HookEventDispatcherModuleHandler given, called in /var/www/ipbes/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 262 in Drupal\openid_connect\OpenIDConnectClaims->__construct() (line 56 of modules/contrib/openid_connect/src/OpenIDConnectClaims.php).
Drupal\openid_connect\OpenIDConnectClaims->__construct(Object, Object) (Line: 262)
Drupal\Component\DependencyInjection\Container->createService(Array, 'openid_connect.claims') (Line: 176)
Drupal\Component\DependencyInjection\Container->get('openid_connect.claims') (Line: 68)
Drupal\openid_connect\Form\OpenIDConnectLoginForm::create(Object) (Line: 28)
Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition('Drupal\openid_connect\Form\OpenIDConnectLoginForm') (Line: 193)
Drupal\Core\Form\FormBuilder->getFormId('Drupal\openid_connect\Form\OpenIDConnectLoginForm', Object) (Line: 227)
Drupal\Core\Form\FormBuilder->buildForm('Drupal\openid_connect\Form\OpenIDConnectLoginForm', Object) (Line: 219)
Drupal\Core\Form\FormBuilder->getForm('Drupal\openid_connect\Form\OpenIDConnectLoginForm') (Line: 185)
openid_connect_form_user_login_form_alter(Array, Object, 'user_login_form') (Line: 562)
Drupal\Core\Extension\ModuleHandler->alter('form', Array, Object, 'user_login_form') (Line: 114)
Drupal\hook_event_dispatcher\HookEventDispatcherModuleHandler->alter(Array, Array, Object, 'user_login_form') (Line: 835)
Drupal\Core\Form\FormBuilder->prepareForm('user_login_form', Array, Object) (Line: 279)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 158)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 191)
Drupal\page_cache\StackMiddleware\PageCache->fetch(Object, 1, 1) (Line: 128)
Drupal\page_cache\StackMiddleware\PageCache->lookup(Object, 1, 1) (Line: 82)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 709)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Same problem occurs in combination with the easy_breadcrumb module.
Patch to fix this in that module: https://www.drupal.org/project/easy_breadcrumb/issues/3307333

Crosslinking a variety of issues mentioned here.

This is a problem with Session Limit. The project should be typehinting the interface, not the concrete implementation.

This module should extend the core class to avoid breaking such expectations.

This would defeat one of the reasons for having service interfaces. There should not be a need to extend any Core service, I recommend mentally assuming most core services are marked final in order to avoid this kind of issue.

---

Moving to the appropriate queue and reformatting IS.

Confirming that the patch in #15 (MR 5) fixes the problem.

Thanks, @Ambient.Impact!

Environment:
- Drupal 9.4.5
- session_limit 2.0.0-beta2

Here is a patch for fixing the issue. Please review it.

Using
- hook_event_dispatcher 4.0.0-beta1
- Drupal core 9.5.9
- session_limit 2.0.0-beta3
I get the following error:
Argument #6 ($moduleHandler) must be of type Drupal\Core\Extension\ModuleHandler, Drupal\hook_event_dispatcher\HookEventDispatcherModuleHandler given, called in /var/www/html/docroot/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259

I've applied the patch from #17 and the error is fixed.

Updated patch to change all the dependencies to use interfaces. Problems will occur eventually with the others, makes sense to fix them all in one go.

Confirming the latest patch #19 works for us.

Confirming the latest patch #19 works for us.

Reroll of #19

I applied the #22 and it worked fine with Hook Event Dispatcher.
Moving it to RTBC.

Looks like partial duplicate of #3361326: Fix the issues reported by phpcs and phpstan with no proper MR.

Re-opening with updated patch. https://www.drupal.org/project/session_limit/issues/3361326 missed a few things from this issue.

Please use merge request for easier review process. Thanks.

I have opened the MR.

Thank you! Looks good. 🍻
RTBC

Confirmed the MR fixes the issue for me as well.

Session limit 2.0.1 is getting conflicted with hook_event_subscriber module. This patch resolves the conflict.

The MR already covers the replacing of ModuleHandler to ModuleHandlerInterface, and more. Hiding this patch to avoid confusion.

+1 RTBC

Doneski! Thank you! 🥂