Added the feature to auto-detect workspace directories and set it as default analyzedProjectDirectory.

9brada6 commented 4 years ago

Added the feature to auto-detect workspace directories and set it as default analyzedProjectDirectory. analyzedProjectDirectory is no longer needed to be explicit set.

Other Improvement:

If no folder project for Phan was found, then only show a console warning, this will make Phan extension to be able to remain all time enabled, instead of being always disabled/enabled per workspace to not show unwanted interface alerts.

If "analyzedProjectDirectory" setting is set and the folder is not valid, it will trigger an interface alert like before.

Testing:

Tested manually on Windows 10, with no workspace containing .phan, one workspace containing .phan, and 2 containing, and it works. To fix #79 and #29. Maybe need to test on Linux/Mac, but it should work.

Modifications:

Firstly, I added the function findValidProjectDirectories() that will return first valid workspace directory, this can be easily redesigned to return all valid directories.
To valid a directory, I added a function pathContainsPhanFolderAndConfig(): bool, based on a similar approach in the function checkValidAnalyzedProjectDirectory and reformat for DRY.
I modified the output log inside the if (!analyzedProjectDirectories) {}, to add a more consistent console message. Note that analyzedProjectDirectories was always an array(which is an object in JS), so that the control structure was always false and not run. Using a console phan will not need to be disabled for non-PHP projects because of unwanted interface alerts(will trigger an alert only when analyzedProjectDirectory was explicitly set).

9brada6 commented 4 years ago

Hi, what do you think about this implementation:

phan.analyzedProjectDirectories =

bool

true - Autoanalyze the workspaces and enable for all inside the directory project found.
false - Will not auto-detect, will not emit an error that nothing was found. Extension will be like disabled.

string|string[]

If string is an absolute path "W:\projects..." Use that.
%WorkspaceName% or %WorkspaceName%/external/php-program/ -> Enable only for wanted workspaces, and if workspace are moved, then the settings are not needed to be rewritten.
./external/php-program/ If we can use absolute paths, then lets use relative paths. String can be a relative path from root workspace only, this is what you think first when you see that you must enter a path. ==> When a string is not found then Phan will display an error.

Still, I'd really rather not enable this by default

Projects may have their own plugins in .phan/config.php which aren't compatible with the version of phan bundled with vs code (or configured by the user in vs code settings for other projects)

Projects may use Phan configuration settings that are extremely, extremely slow or memory intensive, or contain files that take a long time to parse/run plugins on

This is about the setting that select which phan script to use: "phan.phanScriptPath", I think that this setting should also be modified. For example Prettier extension will look first into the workspace directory, and if it finds the program(here /vendor/bin/phan should be) then will use that instead of the one bundled with extension. This is how almost all popular linters/formatters that I've seen using VS Code works (Prettier, ESLint, StyleLint, ...etc)
Maybe we should show a warning that the Phan couldn't parse a file in X seconds? There is a VS Code setting that will not format a file if the format will take longer than 750 mili-seconds, maybe we can use something to show an error like that(and to modify this timeout via a config setting)? Anyway this problem can arise in both situations. This problem should be also partial fixed if we add an indicator in the status bar when the VS Code is waiting for a response, like #66 .

TysonAndre commented 4 years ago

false - Will not auto-detect, will not emit an error that nothing was found. Extension will be like disabled.

A setting of just false might make sense in workspace settings file overrides, but workspace overrides don't work well at the moment

%WorkspaceName% or %WorkspaceName%/external/php-program/ -> Enable only for wanted workspaces, and if workspace are moved, then the settings are not needed to be rewritten.

Not sure which setting that comment is referring to (php version used to run phan?). It seems like an extremely uncommon use case to install php within the project being run (and not cross platform), so I'd rather not complicate phan by adding that

If a user has multiple directories, ./external/vendor/phan/phan/phan might not work as well. Maybe a hash map from directories to relative or absolute path overrides to use for Phan.

This is about the setting that select which phan script to use: "phan.phanScriptPath", I think that this setting should also be modified. For example Prettier extension will look first into the workspace directory, and if it finds the program(here /vendor/bin/phan should be) then will use that instead of the one bundled with extension. This is how almost all popular linters/formatters that I've seen using VS Code works (Prettier, ESLint, StyleLint, ...etc)

Good to know, will check out those extensions and consider - would need to check versions for some minimum compatible version if newer CLI options such as --language-server-* start getting passed in, or checking if phan --version output and exit code is sane (i.e. not a runtime error)

9brada6 commented 4 years ago

I don't think you understand what I was saying. The php thing was a bad example and you didn't understand, sorry.

TLDR: In short, Instead of writing an absolute path(C:\my_projects...) to the project, just write the workspace name between the %%, and if we do this then the project can easily be moved between different computers.

Let's make some uses case:

A user has a normal project(single VS Code workspace), with .phan folder and a local phan(via composer) instaled (this is what most users have, let's say 90%).
A user has 2 workspaces, and both have .phan folder, and he wants both to be analized.
A user has 2 workspaces, and both have .phan folder, but we wants only one to be analized.
A user has 3 workspaces, all 3 have .phan folders, and he wants 2 to be analized.

For 1 we do: "phan.analyzedProjectDirectories": true. (auto-analizing) For 2 we do ""phan.analyzedProjectDirectories": true, (auto-analizing) For 3 we do: "phan.analyzedProjectDirectories": "%NameOfWorkspace%" For 4 we do: "phan.analyzedProjectDirectories": ["%NameOfWorkspace1%", "%NameOfWorkspace2%"].

And the path can also be relative to workspace name: "%NameOfWorkspace1%/../../a_folder/"

9brada6 commented 4 years ago

Also, a workspace name is the one set in the .code-workspace setting: Ex: "folders": [ { "name": "Main", "path": "wordpress\wp-content\plugins\tabs-with-recommended-posts" }, { "name": "Server", "path": "." } ], If the name is not set explicit, then the name will be default to the folder name: "tabs-with-recommended-posts" in first case.

For vscode folder settings, we have relative paths: "phan.analyzedProjectDirectories": "./my_folder/".

TysonAndre commented 3 years ago

For vscode folder settings, we have relative paths: "phan.analyzedProjectDirectories": "./my_folder/".

You will need to downgrade vscode-php-phan to 3.0.0 for your workflow to continue working (and can use phan.phanScriptPath to point to newer phan installations) - I had meant to forbid it entirely but could not even figure out if vs code provided a mechanism to disable user and workspace settings when I was working on this years ago.

I know this is inconvenient - I'd been discussing how this could be done in a user-friendly and safe way with the reporter of CVE-2021-31416

For current vscode versions:

I looked into how https://github.com/microsoft/vscode-maven/pull/526/files handled it since the Memento API is backed by a sqlite database in a global configuration database (in a user's home directory?), which is reasonable.

The first time phan opens a folder and finds a .phan/config.php, prompt the user if they want to trust the extension before executing anything (e.g. checking the php version, etc)

For future vscode versions

https://github.com/microsoft/vscode/issues/106488 is something along the lines of what I wanted (with respect to phan configs and phan plugins being executable code, etc)

The trusted workspaces concept is intended to centralize and unify a security conscious decision required by a variety of VS Code features. The easiest existing example to understand of this decision is with the ESLint extension. The ESLint extension will try to use the eslint module in the current folder that is opened in VS Code and execute code from it. Since you may have checked out a random repository from the web, this could be dangerous if the repository contains a corrupt eslint module. Notice that ESLint is not trying to be malicious, but rather, the repository/corrupt module is taking advantage of this automatic code execution.

TysonAndre commented 3 years ago

https://code.visualstudio.com/updates/v1_57 VS Code 1.57 added that in May, though 106488 is still open and the functionality may change.

TysonAndre / vscode-php-phan