fix(IRegistrationContext): Use SimpleContainer in registerService factory

[provokateurin]

Using SimpleContainer as the type is important so that psalm and PHPstan will get our correct type annotation for the get method, as the ContainerInterface just returns mixed.

[CarlSchwan]

Just noticed but this is in OC, so using that in OCP is bad :(

[provokateurin]

Let's see if it breaks something, then we can revert it 🙈

[nickvergessen]

It breaks our policy that OCP should not expose OC

[DerDreschner]

Since this PR, we see the following error message in our psalm pipeline for the mail app (https://github.com/nextcloud/mail/actions/runs/21984802061/job/63516233833?pr=12412#step:7:11):

Error: lib/AppInfo/Application.php:107:43: InvalidArgument: Argument 2 of OCP\AppFramework\Bootstrap\IRegistrationContext::registerService expects callable(OC\AppFramework\Utility\SimpleContainer):mixed, but impure-Closure(Psr\Container\ContainerInterface):mixed provided (see https://psalm.dev/004)
Error: lib/AppInfo/Application.php:113:45: InvalidArgument: Argument 2 of OCP\AppFramework\Bootstrap\IRegistrationContext::registerService expects callable(OC\AppFramework\Utility\SimpleContainer):mixed, but impure-Closure(Psr\Container\ContainerInterface):OCA\Mail\Vendor\Favicon\Favicon provided (see https://psalm.dev/004)

Should we switch to SimpleContainer instead of ContainerInterface? Or is this breaking change unexpected? 🤔

[provokateurin]

Good question. I know it's an annoying change, but this helps static analyzers to understand our container properly (which can also be seen in the fixes in this PR), so think it's better for apps to change it as well. What do you think @CarlSchwan ?

[DerDreschner]

Ohh, don't get me wrong @provokateurin: I'm for that change as well! Better static analyzing is always being desired as someone coming from C#. 😄

I just wanted to be sure that it's not being reverted right after I'm doing the change as we would have to use the SimpleContainer from the more-or-less private OC namespace in our app then as well. That feels a bit wrong from an architectural standpoint IMHO.

[provokateurin]

You're right, also see the comment above. I think the best solution is to add a container interface to OCP and use that, then we can have correct type information and don't need to rely on OC. I'll do that next week :)

[nickvergessen]

Or we use PSR Container like we already use PSR Logger

[provokateurin]

Which is what we had before, but the type annotation on it is bad and confuses Psalm and PHPStan, so we need to control the interface ourselves.

[Koc]

@provokateurin can you please provide a guidelines for module maintainers, how they should behave with your changes to make Psalm happy? I had an attempt to add if (class_exists()) but it makes things even worse, see an example

[nickvergessen]

For tables instead of adding a custom interface and then using that:

1. Reference the class directly instead of your interface that is just a wrapper for the same class anyway:
   https://github.com/nextcloud/tables/blob/8c4003412a8cfed19f470e760973e1d044ae4b02/lib/Listener/WhenTableTransferredAuditLogListener.php#L33
2. Or just do https://github.com/nextcloud/tables/blob/8c4003412a8cfed19f470e760973e1d044ae4b02/lib/Service/Support/DefaultAuditLogService.php#L20-L22 directly in all the places?

[provokateurin]

can you please provide a guidelines for module maintainers, how they should behave with your changes to make Psalm happy

See the comments above, I will add a proper interface to OCP next week.

[ChristophWurst]

Could we revert for now to have passing Psalm CI in the apps @provokateurin?

[provokateurin]

I'm working on this right now, so reverting will cause more work than just fixing it properly.

[provokateurin]

#58355