Phpinspectionsea: Perhaps this can be replaced with ::class, false positives

[linaori]

I think it was from this plugin (I found it somewhere in a diff). I get a suggestion to use the ::class variant instead.

While the feature is definitely very nice to have, it's giving me some false positives. I don't know if it's possible, but perhaps the usage of the same method could be inspected to see if the arguments are actually supposed to be classes?

class Product {} class Contract {}

$foo->set('Product')->set('Contract'); // suggest
$foo->set('Product')->set('Startdate'); // ignore

[kalessil]

Hi @iltar,

Correct, this suggestion is from the plugin. Methods lookup will not help to identify context - only people are able to do so.

But there is a way to tell the plugin that it's not a class:

    ->set('product', ...);

The plugin takes into account only exact matches between strings and classes, so changing case should resolve the false-positive.

[linaori]

The problem here is that this piece of code is used in an interface, which means I can't make it lower-case. This is just a coincidental usage between a legacy class and the value of something in the html.

Hence I think it's a false-positive that could be resolved by inspecting usages of that method to see if there's always an existing class passed along. But if that's not possible, you can close this issue as it should only be triggered in weird cases.

[kalessil]

I see. I'll keep it open until the next cleanup of the backlog - perhaps some ideas will appear before that time.

[bram123]

Could make it an option to inspect global classes or not. Those with namespaced codebase will not receive these false positives. Those without namespaced codebase have an extra incentive to fix their codebase.
Would hopefully also speedup the inspections.
Option would be default to don't inspect. Because it's the preferred situation.

[linaori]

That's actually a good idea!

[rentalhost]

It's a problem, for instance, in cases where I have a string like "Data" that is confused by a Data::class that exists on my context.

Things that could works:

• Consider as a _String::class_ if it starts with slash \Data;
• Consider as a _String::class_ if it is a namespaced class: Carbon\Date;
• Consider as a _String::class_ is there another class _same context_;

For this last:

// No warning (any class context):
return [ 'Date' => 'Date', 'User' => 'User' ];

// Warning for Date value only.
return [ 'Date' => '{{{Date}}}', 'User' => User::class ];

// Warning for Date key only.
return [ '{{{Date}}}' => 'Date', User::class => 'User' ];

// Warning for both Date strings.
return [ '{{{Date}}}' => '{{{Date}}}', User::class => User::class ];

// Warning for Date value and User value (\User should be declared).
return [ 'Date' => '{{{Date}}}', 'User' => '{{{\Use}}}r' ];
return [ 'Date' => '{{{Date}}}', 'User' => '{{{Namespaced\User}}}' ];

Unfortunatelly there is not a phpdoc type that expects a _String::class_ as argument, then check for methods arguments could not works for now (except if you do a deep analysis, what I think that is a lot of works).

[funivan]

We disable this inspection in our project.

P.S. Latest EAP version contain interesting severity option No highlighting, only fix.

[kalessil]

@bram123, @iltar: the option for reporting root NS (enabled by default) will be implemented.
@funivan: I think once the option has been introduced, you'd give it a try.

I also not sure if the option should be enabled by default. @veewee, @rentalhost, @funivan, @bram123, @iltar, please vote +1/-1 for enabling the option by default.

[linaori]

For the majority of people, it might be nice to have. As long as it's easy to turn it off, no problem to have it on by default :+1:

[veewee]

For me this isn't a case I've encountered yet. I would say +1 for enabling it by default at this point.

[kalessil]

@rentalhost : regarding the cases you mentioned, I reading but can not get a point (I'm pretty slow getting into things today in general ;) )

[rentalhost]

@kalessil No problem!

Basically, you can _accept_ that a string refers to a class if you uses a _backslash_ in it full content. Then a string like \Date (where Date is a mapped class) or Carbon\Carbon (where it too is a mapped class) should be reported to use ::class. But if you declares only "Date" if should not be reported (except if it's a mapped class on current context/namespace).

The last case is if you have an array where you have values and/or keys that have a class name as the previous cases, then any other string could be checked (even without the backslash usage).

Now to organize better:

Case 1: accept zero-index backslash as a class. If you have a string that the first letter is a backslash, and this full string could refers to a real mapped class (mapped here mean "was declared in some place"), then you could suggests that it should be \Date::class instead.

// Info: you could replace it with \Date::class.
$variable = '\Date';

Case 2: accept any-index backslash as a class. I guess that you could generalize this over the first case, because both will use the same way to works. If you have a backslash declared in any index of a string, and this full strings refers to a real mapped class, then you could suggests that it should be Carbon\Carbon::class instead.

// Info: you could replace it with Carbon\Carbon::class.
$variable = 'Carbon\Carbon';

Case 3: accept no-index backslash as a class if it exists on current context. Lets supposes that you are on YourNamespace, and you have a class mapped as \YourNamespace\Date. In this case "Date" string could be reported (but not "date", in lowercase).

namespace YourNamespace;

// Info: you could replace it with Date::class (as \YourNamespace\Date).
$variable = 'Date';

Case 4: accept no-index backslash as a class is an aliases class. For instance: if in your context you have an use that aliases a class to another name, it should be reported. Example will help to understand.

use Date as MyDate;

// Info: you could replace it with MyDate::class.
$variable = 'MyDate';

Case 5: accept no-index backslash as a class if it is imported "as is". For instance: if I do imported Date, then 'Date' string should be reported. It should not be reported if it is a global class (to avoid _false-positives_, but the next case will help with that).

use Carbon\Carbon;
use Date as MyDate;

// Info: you could replace it with Carbon::class.
$variable = 'Carbon';

// Not should be reported anymore: you have aliased Date to MyDate.
// "Date" where doesn't exists or is a global.
$variable = 'Date';

Case 6: accept "complex" global classes. As an extension for case 5, maybe you could accept a global class if it is mapped and have a complex name. With complex I mean: CamelCased or with underline. For instance:

// Info: you could replace it with DateTime::class.
$variable = 'DateTime';

// Info: you could replace it with PHPUnit_Framework_TestCase::class.
$variable = 'PHPUnit_Framework_TestCase';

Case 7: accept no-index case if any array value and/or key accept is a ::class (_only on current dimension_). For instance: if you have an array with two values like [ Date::class, 'Carbon' ], maybe you like to refers that "Carbon" is on reality a Carbon::class (and should imports that) because _one_ of other values is a class ('Carbon' should be a mapped class to be reported too).

It should not report 'Carbon' in cases like [ '\Date', 'Carbon' ], but it will reports '\Date' (like case 1). Then after you accept the autofix (will have autofix, right?), then now 'Carbon' will be reported too.

Note: value and keys should be checked apart. Then, for instance, if I have: [ Date::class => 'Carbon', Carbon::class => 'Carbon' ], in this case, nothing will be reported because only keys are "make to be a class" and not values (reverse way should be truth too).

In case of a multidimensional array, each dimension should be checked apart. Then if on dimension one you have a reportable class, it not mean that an inner array should reports too (example below).

// Not should be reported (only if Date exists on current context).
$array = [ 'Date' ];

// Not should be reported (only if Date or Carbon exists on current context).
$array = [ 'Date', 'Carbon' ];

// Info: you could replace it (value#1) with Carbon::class.
use \Carbon\Carbon;
$array = [ Date::class, 'Carbon' ];

// Info: you could replace it (key#0) with Carbon::class.
// Value here should not be reported (only if Date exists on current context).
use \Carbon\Carbon;
$array = [ 'Carbon' => 'Date' ];

// Info: you could replace it (key#0) with Carbon::class.
// Info: you could replace it (value#0) with Carbon::class.
// Note: both exists on current context because it was imported.
use \Carbon\Carbon;
$array = [ 'Carbon' => 'Carbon' ];

// Multidimensional case: note that inner array is checked apart of main dimension.
$array = [
    // Not reports: all is okay here (except if "Date" or "Carbon" exists on current context, case 5).
    // Important: the values here should be checked apart of next values array.
    \Date::class => [ 'Date', 'Carbon' ],

    // Info: you could replace it (key#1) with Carbon::class.
    'Carbon' => [
        Date::class,
        // Same here: as you have a Date::class, then Carbon should be reported now (in that array only).
        // Info: you could replace it (value#1) with Carbon::class.
        'Carbon'
    ]
];

How it could works: is important you do two loopings on arrays: one to find a preexistent ::class and other to check if siblings should be reported too. For instance:

// Here both Carbon and DateTime should be reported because of Date::class.
$array = [ 'Carbon', Date::class, 'DateTime' ];

Then you should code like that (_warning: pseudo-code_):

foreach (theArray as mixed value) {
    // ex. (isStrictClassType): Date::class or \Date or DateTime (in context)
    if (this.isStrictClassType(value)) { 
        foreach (theArray as mixed value) {
            // ex. (isWeakClassType): \Date or DateTime or Date (in/out context).
            if (this.isWeakClassType(value)) {
                phpStorm.reports(value);
            }
        }
        break;
    }
}

Uff! It seems to be very complex because it is long, but I guess that the major part of that is connected one with other, then I think that will be easy to do that. Case 1-to-6 is related and only case 7 is a special case but seems to be easily to solve too.