Wrong return type in UserPermissionsForm::permissionsByProvider()

Added some comments, I think at least we need to update the return description. Maybe add @see \Drupal\user\PermissionHandlerInterface::getPermissions explaining that's what a permission looks like.

Hi, in Drupal core changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies. Thanks.

The MR should be on 11.x as well.

I put up an alternative more advanced solution using @phpstan-type and @phpstan-import-type in MR !13352. Not sure if core is ready for this though so feel free to run with the original if that's desired.

Can we close 1 MR here?

I'd roll with the @phpstan-type solution. It looks good to me. I'm setting this to Needs Work because the proposed resolution section of the issue summary needs to be updated with the newer solution.

I do believe the feedback for this one has been addressed, correct me if I'm wrong.

First of all, I apologize for getting the @return comment wrong in #3216341: Provide a module-specific permissions form.

It looks as though this will be the first use of @phpstan-type (and @phpstan-import-type) in Drupal core, unless some other pending MR gets merged first. That is a good thing! It also means that we should give some thought to coding standards for these constructs. We do not have to spell out the standards in this issue, but we should set a good example.

The MR adds @phpstan-type and @phpstan-import-type in the main doc blocks for the interface/class. That seems like a good place for it. Are there any other reasonable options? We might define/import the type definition closer to where it is used, but that can get messy as functions are added or removed, since more than one function can use the same type.

I made a couple of suggestions on the MR, adding line breaks to keep the lines short. I think the multi-line versions are more readable. I also added a space before the opening { and a trailing comma before the closing }. Back to NR to consider those suggestions.

My only other question is whether it is helpful to define PermissionInfoList instead of using array<string, PermissionInfo> directly. Adding a level of indirection is not always the right thing to do.

Reference: https://phpstan.org/writing-php-code/phpdoc-types#local-type-aliases

Thank you for sharing that @benjifisher @prudloff what do you think about the suggestions? Definitely a fan of the 2nd one as it makes it easier to read.

@benjifisher is the first suggestion because * @phpstan-import-type PermissionInfoList from \Drupal\user\PermissionHandlerInterface is over 80 characters?

is the first suggestion because ... is over 80 characters?

Yes.

Seems both suggestions were accepted yay! Going to mark thanks all!

+1 for RTBC

Committed and pushed 4385ef7c598 to main and f155b8c6855 to 11.x and 0883f86a720 to 11.4.x. Thanks!