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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

9brada6
Copy link

@9brada6 9brada6 commented Apr 19, 2020

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:

  1. Firstly, I added the function findValidProjectDirectories() that will return first valid workspace directory, this can be easily redesigned to return all valid directories.

  2. To valid a directory, I added a function pathContainsPhanFolderAndConfig(): bool, based on a similar approach in the function checkValidAnalyzedProjectDirectory and reformat for DRY.

  3. 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 added 2 commits April 19, 2020 10:03
… as default analyzedProjectDirectory. analyzedProjectDirectory is no longer needed to be explicit set.
… and do not show alerting errors if no directory was found.
analyzedProjectDirectories = findValidProjectDirectories();

if (!analyzedProjectDirectories.length) {
// Do not send an error to the interface, this is frustrating.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also frustrating to install an extension for a tool you're unfamiliar with and have it not work (e.g. some users may try to start using the extension before setting up a Phan config for the project).

I'd rather you added a config setting such as phan.silenceNoDirectoryWarning: bool

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, sounds good, but also think about this: when you install an extension, you go to the vscode extension page. There, one of the first thing a user must see is this:

"For this extension to work, a workspace folder must contain the ".phan" folder with a"config.php" file inside. To add custom folders and override auto-detection use the setting "analizedProjectDirectories" ".

Having a bunch of configs for every control structure I don't think it is a good idea. An extension should be pleasant and simple to use. Additionally we can alert an error when the setting to debug is on, this is when an user could not figure out how to use, and enabling this will throw the error.

Also I think analizedProjectDirectories should be also make to detect relative paths, but that's another story.

if (('uri' in obj) && ('fsPath' in obj.uri)) {
return obj.uri.fsPath;
}
return '';
Copy link
Owner

@TysonAndre TysonAndre Apr 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be clearer as a for loop - For example, this should filter out the objects without fsPath instead of using the empty string (Is that treated like the current working directory? I can't tell)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah,I think that the if is unnecessary, but I did that to be sure to not throw an exception accessing a undefined property. Changed to:

        const filePathExistInObject = ( ('uri' in obj) && ('fsPath' in obj.uri) );
        if ( filePathExistInObject ) {
            return obj.uri.fsPath;
        }
        return '';

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean this would be clearer as a for loop (written in an imperative style) instead of a call to map() and filter()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed to just "return obj.uri.fsPath;", since "uri" will always be in "WorkspaceFolder", check here, and "fsPath" will always be in uri: here

return workingFolders;
}

// Whether or not a path contains the ".phan" folder alongside with a config.php file.
Copy link
Owner

@TysonAndre TysonAndre Apr 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be just // Returns true if a path contains ".phan/config.php"

"alongside with" could be read as containing both $PROJECT/.phan and $PROJECT/config.php

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok


// Search all the workspace folders and return the first that contains .phan/config.php
// This will return the folder name in an array as required, and just only one folder,
// the first met. We can easy modify the function to add all valid workspace folders to the array.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is out of date - this is already returning all valid workspace folders that were open when this extension started (or is unpaused?). I also see onDidChangeWorkspaceFolders exists in https://code.visualstudio.com/api/references/vscode-api#workspace , but that's out of scope.

Copy link
Author

@9brada6 9brada6 Apr 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is running when extension is activated, so it runs with activate() function. Rewrite the comment to be:
// Search all the workspace folders and return the ones that contains .phan/config.php file.

The function makes exactly what the title says. Maybe in the future add a new function like "handleWorkspaceFoldersChanged()" and bound to the event 'onDidChangeWorkspaceFolders', that will call findValidProjectDirectories() and updated/restart the extension.

@@ -203,7 +198,7 @@ export async function activate(context: vscode.ExtensionContext): Promise<void>
const phanScriptPath = conf.get<string>('phanScriptPath') || defaultPhanScriptPath;
// Support analyzing more than one project the simplest way (always start every server).
const originalAnalyzedProjectDirectories = conf.get<string|string[]>('analyzedProjectDirectory');
const analyzedProjectDirectories = normalizeDirsToAnalyze(originalAnalyzedProjectDirectories);
let analyzedProjectDirectories = normalizeDirsToAnalyze(originalAnalyzedProjectDirectories);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option I thought of would be to have the magic value * for phan.analyzedProjectDirectory mean that this extension should attempt to open any workspace with a valid phan config (in addition to hiding any showOpenSettingsPrompt calls).

Or just a boolean phan.enableForAllProjects: true, which is equivalent.

If that was enabled, then nothing would be popped up. If that was disabled, then phan would show the existing popup, but mention the new alternatives for making the popup not show up

That way, it'd be possible both to force Phan to start a server for a few directories, and to analyze the currently open directores (e.g. in case a workspace had multiple phan configs, or a user occasionally opened different projects)

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If analyzedProjectDirectory was merged with findValidProjectDirectories(), the list of folders would need to be deduplicated, preferring the configured folder name.

https://nodejs.org/api/fs.html#fs_fs_realpathsync_path_options is one way, in case there are symlinks or different ways to represent the same folder

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand exactly what you mean here, I will try tomorrow, maybe there is a better solution for this to be worked out.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If analyzedProjectDirectory was merged with findValidProjectDirectories(), the list of folders would need to be deduplicated, preferring the configured folder name.

This I don't also understand, analyzedProjectDirectory is a setting and findValidProjectDirectories() is a function, if anything is passed into analyzedProjectDirectory, even a string that is not a path, then findValidProjectDirectories() will not run at all.

// the first met. We can easy modify the function to add all valid workspace folders to the array.
function findValidProjectDirectories(): string[] {
// Get the fsPath(file system path) of all workspace folders.
const VSCodeFolders = vscode.workspace.workspaceFolders;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: vscodeFolders would be more consistent with other names - uppercase looks like a class (e.g. import { DocumentFilter, RelativePattern } from 'vscode';)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I will change it to "vscodeFolders" if "vscode" is a standard

@9brada6
Copy link
Author

9brada6 commented Apr 24, 2020

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[]

  1. If string is an absolute path "W:\projects..." Use that.
  2. %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.
  3. ./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
  1. 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)

  2. 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 Add a "phan parsing..." text in the status bar. #66 .

@TysonAndre
Copy link
Owner

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

9brada6 commented Apr 24, 2020

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:

  1. 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%).

  2. A user has 2 workspaces, and both have .phan folder, and he wants both to be analized.

  3. A user has 2 workspaces, and both have .phan folder, but we wants only one to be analized.

  4. 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
Copy link
Author

9brada6 commented Apr 24, 2020

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

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

microsoft/vscode#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
Copy link
Owner

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.

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

Successfully merging this pull request may close these issues.

Default "phan.analyzedProjectDirectory" to project root
2 participants