DisallowedClass now introduces 2 errors instead of 1 that has count: 2

[ruudk]

I just tried to use the new disallowedClass alias for disallowedNamespace which looks great.

But it now produces 2 errors instead of 1 in the baseline:

+		-
+			message: "#^Class Sentry\\\\State\\\\HubInterface is forbidden, use \\<fg\\=yellow\\>Tracker</\\> instead\\.$#"
+			count: 1
+			path: src/File.php
+
 		-
 			message: "#^Namespace Sentry\\\\State\\\\HubInterface is forbidden, use \\<fg\\=yellow\\>Tracker</\\> instead\\.$#"
-			count: 2
+			count: 1
 			path: src/File.php

This is because src/File.php does an import use Sentry\State\HubInterface; and something like __construct(HubInterface $sentry).

After thinking about this, I don't care if people import a class. I just don't want them to use it. PHP-CS-Fixer will take care of unused imports anyway.

[spaze]

That was introduced in #114 where "Class" is now used instead of "Namespace" in the error message if the usage is more "class-y" than "namespace-y". Imports with use are "namespace-y", while the type hint is "class-y": https://github.com/spaze/phpstan-disallowed-calls/blob/80945f8fa346b1d8b50d6922cde56b309fb6008f/src/Usages/NamespaceUsages.php#L60-L64 Before everything was described as a "namespace".

I'd like to keep this change especially because of the new disallowedClasses alias but not only because of that. I'd also like to keep the use detection, not everyone uses an autofixer or an unused imports detection.

Honestly, I'm not sure how to fix this (and frankly, if there's anything to fix ☺️) but I'm sorry for breaking the baseline for you.

[spaze]

I have now updated the release notes to mention you may see multiple different error messages now.

[ruudk]

It would be sweet if the use detection could be disabled 🤔 Default enabled, but for advanced use cases disabled.

[spaze]

I see. Seems like maybe a new config disallowedImports can be introduced and the UseUse node detection moved over from the namespace detection code here: https://github.com/spaze/phpstan-disallowed-calls/blob/80945f8fa346b1d8b50d6922cde56b309fb6008f/src/Usages/NamespaceUsages.php#L63-L64

I'd rather avoid a config switch (like detectImports, allowUses or something) used only for disallowedNamespaces.

What do you think? Mind if I rename the issue?

[ruudk]

That would mean that anyone who wants to limit imports needs to change it. What if we add a allowImports under disallowedNamespaces ? Then you can toggle it per disallowedNamespace.

Rename issue = OK

[spaze]

Namespaces/classes are detected in multiple places, adding a switch to re-allow one of them would soon require adding allowTraits, allowClassConstants etc. or making it a list like allowUsage: [trait, use]. That would make namespaces even more special than they already are and I don't want to go this way. I'd rather have the various disallowed types have the same config as much as possible.

If imports are so special they need to be separately re-allowed then maybe they should have a separate category ☺️

What benefits would allowing imports of otherwise disallowed classes bring anyway? In your case such imports would be removed eventually, so having PHPStan detect them should be fine but maybe I'm missing something?

[ruudk]

If imports are so special they need to be separately re-allowed then maybe they should have a separate category ☺️

:+1:

You're right. In most cases, its just additional error noise. The plugin already catches all the places where it's used.

[spaze]

👍 So do you think the current behavior is fine, or would disallowedImports make it more useful? I think in most cases it would only duplicate the disallowedClasses config so if we can avoid adding it, I'd be happier.

[ruudk]

I think disallowedImports is not needed. I wouldn't use it. I don't care for imports of forbidden stuff. I only care about usage of forbidden things.

[spaze]

Got it, thanks for the input 👍