sirbrillig / phpcs-import-detection

A set of phpcs sniffs to look for unused or unimported symbols.
Other
31 stars 7 forks source link

Allow using array for ignoreUnimportedSymbols #35

Open pokemonhan opened 4 years ago

pokemonhan commented 4 years ago

Many of laravel helper functions like app(), request() e.g. $credentials = request(['username', 'password']); and build in throw new Exception(), and some of self-created function etc.s are showing warning with Found unimported symbol 'Exception'. Found unimported symbol 'request'. , So i need to add these function to inside of the rule config.

For now the writing format is like so

<rule ref="ImportDetection.Imports.RequireImports">
        <properties>
            <property name="ignoreUnimportedSymbols" value="/^(wp_parse_args|OBJECT\S*|request|msgOut|configure|app|Exception|ARRAY_\S+|is_wp_error|__|esc_html__|get_blog_\S+)$/" />
        </properties>
    </rule>

can it be able write as below or some other feature to be able to cover laravel helper functions and Exceptions functions etc;

<rule ref="ImportDetection.Imports.RequireImports">
        <properties>
            <property name="ignoreUnimportedSymbols" type="array">
                <element value="OBJECT\S*"/>
                <element value="wp_parse_args" />
                <element value="request" />
                .....
            </property>
        </properties>
    </rule>
sirbrillig commented 4 years ago

Thanks for bringing this up! I haven't personally used Laravel, but if I can get a list of all their global functions, I can easily add a built-in exception list in the same way I have for WordPress functions. Do you happen to have a list of them or know where I could find them?

That said, I believe your main question is about supporting arrays in the ignoreUnimportedSymbols configuration option, which is a very good idea and one that I've had on my mind for some time. I'll see if I can get that working soon.

Lastly, you mentioned seeing "Found unimported symbol 'Exception'." In general it's a good idea to specify a full path to the Exception class, because if your class uses a namespace, then throw new Exception() will actually cause a fatal error. Worse than that, it will fail only when there's an exception, which is often a code path that's not well tested. If you replace your code with throw new \Exception(), then everything should work fine.

On the other hand, if you'd prefer to ignore that warning when you're already in the global namespace, you can use the ignoreGlobalsWhenInGlobalScope configuration option to do that.

challgren commented 4 years ago

I'm pretty new to Laravel but from my digging around the list can be found at https://github.com/laravel/framework/blob/7.x/src/Illuminate/Foundation/helpers.php

adrianthedev commented 4 years ago

I'm not sure that ignoreGlobalsWhenInGlobalScope works though. I have this under my config and I still get notified about the laravel methods.

    <rule ref="ImportDetection"/>
    <rule ref="ImportDetection.Imports.RequireImports">
        <properties>
            <property name="ignoreGlobalsWhenInGlobalScope" value="true"/>
            <property name="ignoreUnimportedSymbols" value="/^(now|collect\S+)$/"/>
        </properties>
    </rule>

I also forced now and collect using ignoreUnimportedSymbols and still get those highlights.

sirbrillig commented 4 years ago

I'm not sure that ignoreGlobalsWhenInGlobalScope works though.

Sorry for the confusion, when I said "if you'd prefer to ignore that warning when you're already in the global namespace, you can use the ignoreGlobalsWhenInGlobalScope configuration option to do that", I meant that that option would allow you to avoid the warning about Exception without the namespace if you are in the global scope.

I also forced now and collect using ignoreUnimportedSymbols and still get those highlights.

What's an example of the warnings you're seeing? The regular expression you have there should prevent warnings for now() and collectFoobar().

adrianthedev commented 4 years ago

I see what you mean with ignoreGlobalsWhenInGlobalScope. My bad. I read that wrong.

This is how I see the warnings. Apologies for the screenshot. I use phpcs inside VSCode. I don't really know how to run it in the terminal.

image

sirbrillig commented 4 years ago

Thanks for the screenshot! Your regular expression is /^(now|collect\S+)$/ which says, "ignore a symbol named now or a symbol that starts with collect but is followed by one or more non-space characters". That means that will ignore collectFoobar, but not collect, since collect is not followed by any characters.

Try changing it to /^(now|collect\S*)$/ and see if that fixes your issue (or just /^(now|collect)$/ if you only want to match now and collect).

adrianthedev commented 4 years ago

Yes. It works now. I haven't taken the time to test the regex. I just assumed. My bad for copy/paste.

Thank you for this! Keep up the good work!

djibarian commented 5 months ago

I started using Laravel and found the same problem. Global helpers raise an ImportDetection.Imports.RequireImports.Symbol - Found unimported symbol 'xxx'. Are you still planning to manually add the list as for Wordpress? List is here: https://laravel.com/docs/11.x/helpers