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

Option to assume all files get included? #1154

Closed
robehickman opened this issue Dec 29, 2018 · 18 comments
Closed

Option to assume all files get included? #1154

robehickman opened this issue Dec 29, 2018 · 18 comments
Labels

Comments

@robehickman
Copy link

Relating to the comment posted yesterday on my previous issue, this is a minimal case:

    <projectFiles>
        <directory name="./app/" />
    </projectFiles>

File app/a.php

<?php
function select_keys($array, $keys) {
    return array_intersect_key($array, array_flip($keys));
}

b.php

<?php
class b {
    function  index() {
         select_keys(['a' => 1, 'b' => 2], ['a']);
    }
}

main.php

include "a.php";

<dynamic dispatch based on URL that may include b.php>

What I had assumed is that 'directory' recursively includes all files into the global namespace, which does not appear to be the case.

In my actual code, the main entry point is './src/main.php', this includes a small number of always loaded files which define global helper functions like select_keys() above. From this point, the system calls a dispatcher which includes a 'controller' file depending on the URL (validated against a statically built array), represented here by b.php. As this is data driven, Psalm can't see it.

It is safe to assume that all files always get included, as this will not cause name collision, the lazy loading is done as an optimisation. Is there a configuration option to say 'assume that every file is included?', or do I need to create a 'fake' entry point file that includes everything?

@robehickman
Copy link
Author

Creating a fake entry point that includes everything solves the noted problem, I'd still be interested if there is a configuration option to assume this mode of operation.

<?php
$rii = new RecursiveIteratorIterator(new RecursiveDirectoryIterator('../framework/git/'));

$out = "<?php\n";
foreach ($rii as $file) {
    if ($file->isDir()) continue;
    if(!preg_match("/\.php$/",$file->getPathname())) continue;
    if(preg_match("/views/",$file->getPathname())) continue;
    if(preg_match("/theme/",$file->getPathname())) continue;

    $name = $file->getPathname();
    $out .= "include '$name';\n";
}

file_put_contents('fake_entry_point.php', $out);

Psalm dosn't seem to be aware of the CURL constants like CURLOPT_POSTFIELDS, or Imagick extension. It's also missing the mcrypt constants which are used by phpmailer.

It's also getting confused by my template system which uses extract() I've just filtered out the templates as I don't see that much value in checking them.

@weirdan
Copy link
Collaborator

weirdan commented Dec 29, 2018

As far as I understand, Psalm expects you to include those files from your autoloader script, and specify that script in Psalm config file (see autoloader attribute).

@robehickman
Copy link
Author

Hi weirdan, with the exception of some 3rd party code, this system doesn't use php's autoload system, everything is included manually, most of it optionally at runtime. I've discussed this with the author previously.

Issues with curl was due to me not having curl installed locally, I usually edit this code remotely with sshfs, but an running psalm locally.

I just attempted to ignore errors from the 3rd party code I use with:

<directory name="../framework/remote/code/src/extern/" />

Doing so throws an exception, it works without this line but generates a lot of errors from the 3rd party code:

Fatal error: Uncaught InvalidArgumentException: Could not get class storage for league\oauth2\client\provider\abstractprovider in /home/a/H-WEB/psalm/vendor/vimeo/psalm/src/Psalm/Internal/Provider/ClassLikeStorageProvider.php:43
Stack trace:
#0 /home/a/H-WEB/psalm/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php(82): Psalm\Internal\Provider\ClassLikeStorageProvider->get('league\\oauth2\\c...')
#1 /home/a/H-WEB/psalm/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php(115): Psalm\Internal\Analyzer\Statements\Expression\Call\StaticCallAnalyzer::analyze(Object(Psalm\Internal\Analyzer\StatementsAnalyzer), Object(PhpParser\Node\Expr\StaticCall), Object(Psalm\Context))
#2 /home/a/H-WEB/psalm/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php(585): Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer::analyze(Object(Psalm\Internal\Analyzer\StatementsAnalyzer), Object(PhpParser\Node\Expr\StaticCall), Object(Psalm\Conte in /home/a/H-WEB/psalm/vendor/vimeo/psalm/src/Psalm/Internal/Provider/ClassLikeStorageProvider.php on line 43

@muglug
Copy link
Collaborator

muglug commented Dec 29, 2018

You mentioned Wordpress in your earlier ticket, and the way I analysed that is instructive here. Psalm was only configured to analyse a couple of entrypoint PHP files (e.g. app/main.php in your example) and it then analysed the includes as they appeared in the files, recursively.

@robehickman
Copy link
Author

Thanks, that was a misinterpretation on my part then.

@robehickman
Copy link
Author

Do you have any idea what would cause the exception above?

@muglug
Copy link
Collaborator

muglug commented Dec 29, 2018

@robehickman running with --debug-by-line would point to the line that Psalm was analysing when it hit that error. I think there might be a bug with included-file analysis when the file in question is in the list of ignored files.

@robehickman
Copy link
Author

robehickman commented Dec 29, 2018

@muglug It's crashing on PHPMailer get oauth token. PHPMailer optionally has support for oauth, I'm not using that feature.

/home/a/H-WEB/framework/remote/code/src/extern/PHPMailer/get_oauth_token.php:81
PHP Fatal error:  Uncaught InvalidArgumentException: Could not get class storage for league\oauth2\client\provider\abstractprovider in /home/a/H-WEB/psalm/vendor/vimeo/psalm/src/Psalm/Internal/Provider/ClassLikeStorageProvider.php:43
Stack trace:
#0 /home/a/H-WEB/psalm/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php(82): Psalm\Internal\Provider\ClassLikeStorageProvider->get('league\\oauth2\\c...')
#1 /home/a/H-WEB/psalm/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php(115): Psalm\Internal\Analyzer\Statements\Expression\Call\StaticCallAnalyzer::analyze(Object(Psalm\Internal\Analyzer\StatementsAnalyzer), Object(PhpParser\Node\Expr\StaticCall), Object(Psalm\Context))
#2 /home/a/H-WEB/psalm/vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php(585): Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer::analyze(Object(Psalm\Internal\Analyzer\StatementsAnalyzer), Object(PhpParser\Node\Expr\StaticCall), Object(Psalm\Conte in /home/a/H-WEB/psalm/vendor/vimeo/psalm/src/Psalm/Internal/Provider/ClassLikeStorageProvider.php on line 43

If I remove the two files related to oauth from pmpmailer, the issue goes away.

@muglug
Copy link
Collaborator

muglug commented Dec 29, 2018

Awesome, thanks. I’ll have a look tonight.

@muglug muglug added the bug label Dec 29, 2018
@muglug
Copy link
Collaborator

muglug commented Dec 29, 2018

Tried reproducing with latest version of PHPMailer, no luck. Would you mind zipping your version of PHPMailer and sending?

@robehickman
Copy link
Author

@muglug No problem, will do tomorrow.

@robehickman
Copy link
Author

It is version 5.2.23.

@robehickman
Copy link
Author

Here you go, this is a minimal example that crashes:

http://files.robehickman.com/psalm_crash.zip

I'm using PHP 7.0.32-0ubuntu0.16.04.1 (cli) ( NTS ) in case that matters.

@muglug
Copy link
Collaborator

muglug commented Dec 30, 2018

perfect - should have a fix tonight

@robehickman
Copy link
Author

Hopefully it crashes for you.

Relating to dead code detection, psalm detects both of the following vars as unused, when they are being used. They are passed as an array into a template.

        $new_text = null;
        $new_url  = null;

        if(user_has_permission('create_'.$content_type)) {
            $new_text = get_if_set($ctrl->internal, 'list_btn_new_text', $new_text);
            $new_url = make_url('admin', 'create', $content_type);
        }

        $view = instance_view('admin/display_table');
        $view = $view->parse_to_variable([
            'new_text'      => $new_text,
            'new_url'       => $new_url
        ]);

@muglug
Copy link
Collaborator

muglug commented Dec 30, 2018

Reproduced here: https://getpsalm.org/r/d8e47866e9

@muglug muglug closed this as completed in 0f6ce98 Dec 30, 2018
@muglug
Copy link
Collaborator

muglug commented Dec 30, 2018

Thanks for your patience, and for helping to improve Psalm!

@robehickman
Copy link
Author

No problem, It's already found a few problems with my code and I foresee it being a useful tool for me.

If you have time, it would be good if the section of the documentation on checking templates was fleshed out a bit more, with an example of variables being passed to a template, a template which uses them, and the plasm plug-in which processes this. I can't currently follow the example template checker class as I'm unsure what the expected input is.

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

No branches or pull requests

3 participants