Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow using array for ignoreUnimportedSymbols #35

Open
pokemonhan opened this issue Dec 15, 2019 · 8 comments
Open

Allow using array for ignoreUnimportedSymbols #35

pokemonhan opened this issue Dec 15, 2019 · 8 comments
Labels
enhancement New feature or request

Comments

@pokemonhan
Copy link

pokemonhan commented Dec 15, 2019

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
Copy link
Owner

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.

@sirbrillig sirbrillig added the enhancement New feature or request label Dec 15, 2019
@sirbrillig sirbrillig changed the title May I use value inside ignoreUnimportedSymbols as tidly Allow using array for ignoreUnimportedSymbols Dec 16, 2019
@challgren
Copy link

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
Copy link

adrianthedev commented Jun 18, 2020

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
Copy link
Owner

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
Copy link

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
Copy link
Owner

sirbrillig commented Jun 23, 2020

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
Copy link

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants