Deprecate passing non-strings to UserSession::hasPermission() and User::hasPermission()

I bet it's won't fix because the calling code must be fixed instead of core

Alternatively it could use to add deprecation when UserSession::hasPermission() is called with non-string argument

Kind of it, reopening as bug possible and re-title

will need change record and test and re-parented

Seems to have caused some test failures in DrupalCI.

This has broken my site. It started with upgrade to 10.2.0 and now any user that isn't the root user (1) get's this message when logging on. Ideas?

TypeError: Drupal\Core\Session\PermissionChecker::hasPermission(): Argument #1 ($permission) must be of type string, null given, called in /Users/*****/Sites/public_html/web/core/lib/Drupal/Core/Session/UserSession.php on line 123 in

Drupal\Core\Session\PermissionChecker->hasPermission() (line 23 of core/lib/Drupal/Core/Session/PermissionChecker.php).
Drupal\Core\Session\UserSession->hasPermission(NULL) (Line: 121)
Drupal\Core\Session\AccountProxy->hasPermission(NULL) (Line: 212)
form_mode_control_control_access_form_mode(Array, 'creation', NULL, Object, 'contact_message.contact_sales.', Object, Array) (Line: 40)
form_mode_control_entity_form_display_alter(Object, Array, NULL) (Line: 545)
Drupal\Core\Extension\ModuleHandler->alter('entity_form_display', Object, Array) (Line: 133)
Drupal\Core\Entity\Entity\EntityFormDisplay::collectRenderDisplay(Object, 'default') (Line: 284)
Drupal\Core\Entity\ContentEntityForm->init(Object) (Line: 97)
Drupal\Core\Entity\EntityForm->buildForm(Array, Object)
call_user_func_array(Array, Array) (Line: 536)
Drupal\Core\Form\FormBuilder->retrieveForm('contact_message_contact_sales_form', Object) (Line: 283)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 48)
Drupal\Core\Entity\EntityFormBuilder->getForm(Object) (Line: 86)
Drupal\contact\Controller\ContactController->contactSitePage(Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 627)
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: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
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: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
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: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Test failures in patch #7 look strange

/tmp/build_workspace_04dcabf34eade6f2685220200c46ced7IA9qsD/core/lib/Drupal
/Component/Serialization/vendor does not exist and could not be created:
Failed to create "/tmp/build_workspace_7f381338ba97106e55c4fb8ea08a06c6Eeef
Dz/core/lib/Drupal/Component/Render/vendor/drupal/core-utility": mkdir(): N
o space left on device

Creating a new MR with same changes to see the actual tests that are failing

No failed tests

We probably need a test to confirm the deprecation message. But I am not able to find a 'realistic' scenario where a non string value would be passed as an argument to hasPermission(). The proposed solution is to simply cast $permission to string. The only option to create a failing test for this approach is to call hasPermission() with NULL. If hasPermission is called with an array to create a failing test like hasPermission(['access content']), then deprecation warning will get triggered, but the logic will fail (Array to string conversion).

Moving to needs review for suggestions

Can probably create a kernel test and call the function.

Also trigger_errors shouldn't points to the ticket but a change record.

Added Test and CR https://www.drupal.org/node/3411485

Deprecation looks good. CR reads fine too.

The MR doesn't match the issue title - we're deprecating passing a non-string to ::hasPermission() but not adding the type hint yet.

To add the type hint, we'd need to add it to AccountInterface::hasPermission(), this will require using the mechanism documented at https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... - however, we've only actually done that at most once or twice I think so far.

Given this is a bugfix, I think we should just add the cast here with an inline comment, and then open a follow-up to do the full deprecation and type hinting process.

The documentation for adding the typehint seems wrong? it says:

Or, change the typehint of an argument. For example:

-  public function foo(string $bar);
+  public function foo(/* string|Stringable $bar */);

But shouldn't that be:

-  public function foo(string $bar);
+  public function foo(/* string|Stringable */$bar);

As it stands the documentation seems to want you to temporarily remove the argument by commenting it out.

I see no reason to cast as interface expects string, so added type-hints as deprecation already is here

Looks good to me, but waiting for a second approval because of my assumption that the deprecation docs are slightly incorrect.

#19 mentions a follow up, could that be created, then can mark it.

Created a child issue and tagged it

Sorry @kristiaanvandeneynde for this following off my radar. Marking.

I'm triaging RTBC issues. I read the IS, the comments for this issue and the followup and the MR for this issue. I don't see any unanswered questions.

The proposed resolution is not correct. I have updated it.

What I did find that needs work is the follow up. The issue summary over there is too brief. If I had not read this issue first I would have no idea what is to be done in that issue. I will tag that issue for an issue summary update.

Leaving at RTBC

I'm not sure why we're suggesting Stringable in the future typehint. As far as I can see it'll be wrong.

Yeah that will be wrong, I think the author perhaps got confused because the example we linked to makes mention of Stringable too?

Looks good to me now, seems to have a random test failure.

#29 has been addressed, back to RTBC

Committed and pushed a9f3b75195 to 11.x and 1122c8d387 to 10.3.x. Thanks!

Thank you! send a patch to follow-up #3415189: Tighten typehint in UserSession::hasPermission() and User::hasPermission()

Please close the MR