Php Tooling Extensions Using LSP Framework #14671

niteskum · 2019-03-20T07:22:07Z

Language Server Client Extension using the https://github.com/felixfbecker/php-language-server

Requirements:

Language Server Protocol Support for Brackets #14606
Php 7
Composer 1.8.4

Installation: npm install

Set the following preferences in

brackets.json

to configure the extension:

"php": {
    "enablePhpTooling": true,             // Enable/Disable PHP Tooling
    "executablePath": "php",              //Path of the php 7 executable
    "memoryLimit": "4095M",               //Memory limit for the Language Server Process
    "validateOnType": "false"             //Get validation results on Type or on Save
}

niteskum · 2019-03-20T07:24:47Z

This PR is Using LSP Framework which are being reviewed in separate PR #14606 .

@shubhsnov @swmitra @subhashjha333 please review

shubhsnov · 2019-03-20T13:43:08Z

src/extensions/default/PhpTooling/client.js

+}
+
+var serverOptions = function (){
+        return new Promise(function (resolve, reject) {


nit: The indentation seems off here.

indentation is correct

shubhsnov · 2019-03-20T13:44:13Z

src/extensions/default/PhpTooling/client.js

+var LanguageClient = require(global.LanguageClientInfo.languageClientPath).LanguageClient,
+    net = require("net"),
+    cp = require("child_process"),
+    execa = require("execa"),


Are you introducing a new node_module? (execa)

yes execa and semver

shubhsnov · 2019-03-20T13:47:06Z

src/extensions/default/PhpTooling/client.js

+        confParams["validate_executablePath"] ||
+        (process.platform === 'win32' ? 'php.exe' : 'php');
+
+    memoryLimit = confParams["memoryLimit"] || 'default';


Keep the default preferences in Brackets itself, and send from there, instead of using a 'default' string.

shubhsnov · 2019-03-20T13:48:12Z

src/extensions/default/PhpTooling/client.js

+            });
+            // Listen on random port
+            server.listen(0, '127.0.0.1', function () {
+                var pathToPHP = __dirname + "/vendor/bin/php-language-server.php";


Make sure this path works for Win and Mac both.

shubhsnov · 2019-03-20T13:50:17Z

src/extensions/default/PhpTooling/main.js

+    var clientFilePath = ExtensionUtils.getModulePath(module, "client.js"),
+        clientName = "PhpClient",
+        _client = null,
+        phpConfig = {},


See my comment above. Have defaults set here, which are overridden when the user sets the preferences.

shubhsnov · 2019-03-20T13:52:22Z

src/extensions/default/PhpTooling/main.js

+
+    PreferencesManager.definePreference("php", "object", {
+        enablePhpTooling: true,
+        showDiagnosisOnType: false,


showDiagnosisOnType => validateOn{Type, Save} . since we are using validate elsewhere as well.

shubhsnov · 2019-03-20T13:54:16Z

src/extensions/default/PhpTooling/main.js

+        var newPhpConfig = PreferencesManager.get("php");
+
+        if((newPhpConfig["executablePath"] !== phpConfig["executablePath"])
+            || (newPhpConfig["enablePhpTooling"] !== phpConfig["enablePhpTooling"]))  {


What about validation_executablePath?

removed validation_executablePath , as it is not required.

shubhsnov · 2019-03-20T14:00:39Z

src/extensions/default/PhpTooling/main.js

+    }
+
+    function startPhpServer() {
+        if(_client && phpConfig["enablePhpTooling"]) {


Two suggestions:

Decouple the validation logic from this function. You can do it on App Ready and then prompt the UI on opening a PHP file the first time. Save that to state.

I know it seems like a nice optimization to start the server on first open of PHP file, but it doesn't seem like valid enforcement if the extension itself enforces it. Since this is a reference implementation, we'd want to provide the developer with a way where they can defer the activation of the server if they want from the brackets core itself. This is a nice optimization which other LSP extension can leverage as well.

I think we should always validate Server config before starting.
otherwise if server start fails for any reason (e.g user set wrong executable Path), we wont be able to notify user about failure.

It's up to you. I find the current way a bit clumsy. Also, I don't think you clearly understood what I meant.

shubhsnov · 2019-03-20T14:15:53Z

Another thing to note: To test the validation logic against PHP5 and PHP7. Both should give different results, based on syntax.

shubhsnov · 2019-03-21T09:23:16Z

@niteskum Please add support for PHP static code hinting as part of this on priority, for completeness.

shubhsnov · 2019-03-21T10:35:13Z

@niteskum Some other required features for reference impl:
Currently, there are two ways to handle the project root change, either restart or send folder change event. Tap into the server capability which is received on client start to figure out whether the server can handle the folder change event, and then decide dynamically which way to go.

Same for completion resolve.

niteskum · 2019-03-21T11:33:45Z

@shubhsnov I can see all static code hints and Parameter hints comes from LSP Server except below few variables.
$GLOBALS
$_SERVER
$_GET
$_POST
$_FILES
$_REQUEST
$_SESSION
$_ENV
$_COOKIE
$php_errormsg
$HTTP_RAW_POST_DATA
$http_response_header
$argc
$argv
$this

are we sure we have to include this in code hints.??, There might be some reason lsp server is not giving these variables as hints.

shubhsnov · 2019-03-21T12:57:24Z

src/extensions/default/PhpTooling/main.js

+        if ((newPhpConfig["executablePath"] !== phpConfig["executablePath"])
+                || (newPhpConfig["enablePhpTooling"] !== phpConfig["enablePhpTooling"])) {
+            phpConfig = newPhpConfig;
+            startPhpServer();


I think we need to differentiate between starting the server and restarting the server. You seem to be just starting the server again and again. I don't see any stop anywhere. Somebody can always change a valid path to another valid path.

shubhsnov · 2019-03-21T12:58:07Z

src/extensions/default/PhpTooling/main.js

+    var handleProjectOpen = function (event, directory) {
+        _client.stop()
+            .done(function () {
+                setTimeout(function () {


This timeout is not required. It was for test purpose only,

It does not work without timeout function, Can you check from core side.
Better provide a restart function so Extension Developer need not to worry about these things

shubhsnov · 2019-03-21T13:01:55Z

src/extensions/default/PhpTooling/main.js

+
+    PreferencesManager.definePreference("php", "object", {
+        enablePhpTooling: true,
+        showDiagnosisOnType: false,


shubhsnov · 2019-03-21T13:07:06Z

@shubhsnov I can see all static code hints and Parameter hints comes from LSP Server except below few variables.
$GLOBALS
$_SERVER
$_GET
$_POST
$_FILES
$_REQUEST
$_SESSION
$_ENV
$_COOKIE
$php_errormsg
$HTTP_RAW_POST_DATA
$http_response_header
$argc
$argv
$this

are we sure we have to include this in code hints.??, There might be some reason lsp server is not giving these variables as hints.

Get this resolved. My take is that most of these appear to quite common globals. It would be good to have.

niteskum · 2019-03-22T06:44:39Z

@shubhsnov Regarding FolderChange Event my understanding is "workspace/didChangeWorkspaceFolders" event is for workspace folder structure change like (folder added or removed from workspace ), not for the workspace project root folder change.

correct me if my understanding is wrong.

shubhsnov · 2019-03-22T09:22:54Z

@niteskum

@shubhsnov Regarding FolderChange Event my understanding is "workspace/didChangeWorkspaceFolders" event is for workspace folder structure change like (folder added or removed from workspace ), not for the workspace project root folder change.

correct me if my understanding is wrong.

Quoting from the actual specification here :
https://microsoft.github.io/language-server-protocol/specification#workspace_workspaceFolders

Many tools support more than one root folder per workspace. Examples for this are VS Code’s multi-root support, Atom’s project folder support or Sublime’s project support. If a client workspace consists of multiple roots then a server typically needs to know about this. The protocol up to now assumes one root folder which is announced to the server by the rootUri property of the InitializeParams. If the client supports workspace folders and announces them via the corresponding workspaceFolders client capability, the InitializeParams contain an additional property workspaceFolders with the configured workspace folders when the server starts.

In Brackets we only have one project root, so workspace and project are synonymous. This way we can piggyback on the client capability "workspaceFolders", to change project root. I've done so in the default capabilities. A change in the project root can thus be translated as an addition of a folder and removal of a folder from a workspace which only has one folder.

So quoting again:

The workspace/didChangeWorkspaceFolders notification is sent from the client to the server to inform the server about workspace folder configuration changes.

I believe the reason we have both "rootUri" and "workspaceFolders" in the specification is because of backward compatibility. Earlier the servers didn't support multiple workspace folders, and separate clients had to be created per workspace folder manually in the extension.

shubhsnov · 2019-03-22T16:13:13Z

src/extensions/default/PhpTooling/CodeHintsProvider.js

+        return true;
+    };
+
+    CodeHintsProvider.prototype.getHints = function (implicitChar) {


I don't think we need this file. You can create override the getHInts function of the DefaultProvider object since that is the only place is where you need to change anything. See my comments below.

see my comment below

shubhsnov · 2019-03-22T16:14:48Z

src/extensions/default/PhpTooling/main.js

+    };
+
+    function registerToolingProviders() {
+        var chProvider = new CodeHintsProvider(_client),


do this instead:

var chProvider = new DefaultProviders.CodeHintsProvider(_client), chProvider.getHints = yourOwnGetHintsFunction;

I don't see this gives us any benefits, I understand that I have changes only in getHints function but still I have to rewrite all Util functions which are defined in default code Hints provider and I have to access "this" object, If i use "this" variable in normal functions that will look weird. this seems to be an hack code not recommended coding practise.
If you are going to externalise all util functions, then I can reuse your util functions from default provider.

niteskum · 2019-03-22T17:39:21Z

@niteskum

@shubhsnov Regarding FolderChange Event my understanding is "workspace/didChangeWorkspaceFolders" event is for workspace folder structure change like (folder added or removed from workspace ), not for the workspace project root folder change.
correct me if my understanding is wrong.

Quoting from the actual specification here :
https://microsoft.github.io/language-server-protocol/specification#workspace_workspaceFolders

Many tools support more than one root folder per workspace. Examples for this are VS Code’s multi-root support, Atom’s project folder support or Sublime’s project support. If a client workspace consists of multiple roots then a server typically needs to know about this. The protocol up to now assumes one root folder which is announced to the server by the rootUri property of the InitializeParams. If the client supports workspace folders and announces them via the corresponding workspaceFolders client capability, the InitializeParams contain an additional property workspaceFolders with the configured workspace folders when the server starts.

In Brackets we only have one project root, so workspace and project are synonymous. This way we can piggyback on the client capability "workspaceFolders", to change project root. I've done so in the default capabilities. A change in the project root can thus be translated as an addition of a folder and removal of a folder from a workspace which only has one folder.

So quoting again:

The workspace/didChangeWorkspaceFolders notification is sent from the client to the server to inform the server about workspace folder configuration changes.

I believe the reason we have both "rootUri" and "workspaceFolders" in the specification is because of backward compatibility. Earlier the servers didn't support multiple workspace folders, and separate clients had to be created per workspace folder manually in the extension.

Php server does not support WorkspaceFolders change Notification. so this can not be tested with Php client. so I am not using this in PHP client. we should not ship any feature without having tested.

instead I have used servercapabilities.definitionProvider in PHP client this flag is true if Language Server support "go to definition". PHP Server support so i have used this PHP client.

shubhsnov · 2019-03-22T23:45:10Z

Php server does not support WorkspaceFolders change Notification. so this can not be tested with Php client. so I am not using this in PHP client. we should not ship any feature without having tested.

instead I have used servercapabilities.definitionProvider in PHP client this flag is true if Language Server support "go to definition". PHP Server support so i have used this PHP client.

@niteskum Don't worry, I have added tests for the same already, by creating a sample LSP server, which supports the event. What you need to do here is just check the existence of the server capability(ServerCapabilities/workspace/workspaceFolders). If it's available then use the "didChangeWorkspaceFolder" event, otherwise do an explicit restart.

shubhsnov · 2019-03-22T23:54:29Z

src/extensions/default/PhpTooling/CodeHintsProvider.js

+                // There is a bug in Php Language Server, Php Language Server does not provide superGlobals
+                // Variables as completion. so these variables are being explicity put in response objects
+                // below code should be removed if php server fix this bug.
+                for(var key in phpSuperGlobalVariables) {


I don't think we should add the static hints here always, otherwise we'll show hints even when they are not required. It should only come when implicitChar is '$', or otherwise filtered by the current token's prefix in case the token starts with '$'.

it will shown only if prefix is '$'. in below code "filterWithQueryAndMatcher" will filter it.

niteskum · 2019-03-23T06:40:38Z

Php server does not support WorkspaceFolders change Notification. so this can not be tested with Php client. so I am not using this in PHP client. we should not ship any feature without having tested.
instead I have used servercapabilities.definitionProvider in PHP client this flag is true if Language Server support "go to definition". PHP Server support so i have used this PHP client.

@niteskum Don't worry, I have added tests for the same already, by creating a sample LSP server, which supports the event. What you need to do here is just check the existence of the server capability(ServerCapabilities/workspace/workspaceFolders). If it's available then use the "didChangeWorkspaceFolder" event, otherwise do an explicit restart.

If we are going to use this event , then in initialize function we should also send workspaceFolders which is root_path, this require a change in core side in initialize function.

niteskum · 2019-03-23T16:08:11Z

Another thing to note: To test the validation logic against PHP5 and PHP7. Both should give different results, based on syntax.

For LSP server to work we need PHP 7, with PHP 5 LSP server will not work

niteskum · 2019-03-24T19:53:22Z

Php server does not support WorkspaceFolders change Notification. so this can not be tested with Php client. so I am not using this in PHP client. we should not ship any feature without having tested.
instead I have used servercapabilities.definitionProvider in PHP client this flag is true if Language Server support "go to definition". PHP Server support so i have used this PHP client.

@niteskum Don't worry, I have added tests for the same already, by creating a sample LSP server, which supports the event. What you need to do here is just check the existence of the server capability(ServerCapabilities/workspace/workspaceFolders). If it's available then use the "didChangeWorkspaceFolder" event, otherwise do an explicit restart.

If we are going to use this event , then in initialize function we should also send workspaceFolders which is root_path, this require a change in core side in initialize function.

fixed

shubhsnov · 2019-03-25T08:07:01Z

src/extensions/default/PhpTooling/phpGlobals.json

+		"description": "Contains an array of all the arguments passed to the script when running from the command line.",
+		"type": "array"
+	},
+	"$this": {


Add the "parent" and "self" keyword as well.

Also __destruct.

niteskum · 2019-03-25T09:17:51Z

src/extensions/default/PhpTooling/main.js

+
+    function registerToolingProviders() {
+        var chProvider = new CodeHintsProvider(_client),
+            phProvider = new DefaultProviders.ParameterHintsProvider(_client),


We need servercapabilities in client

shubhsnov · 2019-03-25T09:33:20Z

src/extensions/default/PhpTooling/CodeHintsProvider.js

+        return matchResults;
+    }
+
+    CodeHintsProvider.prototype.hasHints = function (editor, implicitChar) {


I'll add the ability to add static hints in the Default Provider itself.

shubhsnov · 2019-03-25T09:35:50Z

src/extensions/default/PhpTooling/main.js

+
+    function registerToolingProviders() {
+        var chProvider = new CodeHintsProvider(_client),
+            phProvider = new DefaultProviders.ParameterHintsProvider(_client),


@niteskum Please override the hasParameterHints function to handle bug. Use TokenUtils to check if the previous token is of type "variable", and return true only then.

@shubhsnov what is the bug?

shubhsnov · 2019-03-25T09:37:23Z

src/extensions/default/PhpTooling/main.js

+    }
+
+    function restart(project) {
+        var result = $.Deferred();


Try using _client.restart directly.

shubhsnov · 2019-03-28T05:43:27Z

@niteskum There is a bug with the php server, where it provides the parameter hints just outside the function after the closing parentheses ')'. Can you take a look at this and see if we can do a targeted fix from Brackets side.

shubhsnov · 2019-03-28T05:44:57Z

LGTM 👍 💯

swmitra · 2019-04-02T09:39:50Z

Please resolve the conflicts @niteskum.

niteskum · 2019-04-02T09:52:11Z

Please resolve the conflicts @niteskum.

resolved conflicts in strings.js

shubhsnov · 2019-04-02T10:16:07Z

@gavankar I think the Travis CI env configuration should also be updated, for PHP 7 and composer 1.8.4. The CI is failing due to this and will continue to fail otherwise.

php-Tooling npm package requires felixfbecker/language-server php package, which can only be installed on php7. this should fix the break in brackets travis build

Switch to php7 in travis before dependency installation in travis

Php Tooling Extensions Using LSP Framework

7f75eb4

Nitesh Kumar added 2 commits March 20, 2019 15:22

Corrected Indentation space

9dbc24a

Corrected ESLint Error

415a08f

shubhsnov reviewed Mar 20, 2019

View reviewed changes

Nitesh Kumar added 4 commits March 21, 2019 00:43

addressed review comments

8893083

Addressed review comments

b7cf0ed

addressed review comments

daf09de

Added Preferences description string

7637c4e

shubhsnov reviewed Mar 21, 2019

View reviewed changes

Addressed review comments

c0a1c6a

Addressed review comments

afdaaed

shubhsnov reviewed Mar 22, 2019

View reviewed changes

Nitesh Kumar added 3 commits March 24, 2019 12:59

addressed review comments

55e565c

Addresed review comments

b2b1573

Addresed review comments

eeb774e

shubhsnov reviewed Mar 25, 2019

View reviewed changes

niteskum commented Mar 25, 2019

View reviewed changes

shubhsnov reviewed Mar 25, 2019

View reviewed changes

Nitesh Kumar added 5 commits March 26, 2019 21:29

Addressed some bug

f41645a

Adding Unit test Files

7de777b

Corrected the strings

18b665b

Fixed Eslint Errors

51e81f5

Added some unit test Cases

006612f

using restart function

de4531b

jhasubhash mentioned this pull request Apr 1, 2019

Language Server Protocol Support for Brackets #14606

Merged

15 tasks

pulled latest from master and resolved conflicts

ed77929

gavankar and others added 2 commits April 3, 2019 13:34

Switch to php7 in travis before dependency installation

b22e854

php-Tooling npm package requires felixfbecker/language-server php package, which can only be installed on php7. this should fix the break in brackets travis build

Merge pull request #2 from gavankar/fix_travis

6b8e24f

Switch to php7 in travis before dependency installation in travis

shubhsnov approved these changes Apr 3, 2019

View reviewed changes

gavankar closed this Apr 3, 2019

gavankar reopened this Apr 3, 2019

shubhsnov merged commit 9516c8d into adobe:master Apr 3, 2019

This was referenced Aug 30, 2021

[CLOSED] Language Server Protocol Support for Brackets brackets-archive/bracketsIssues#11952

Open

[CLOSED] Php Tooling Extensions Using LSP Framework brackets-archive/bracketsIssues#11969

Open

Php Tooling Extensions Using LSP Framework #14671

Php Tooling Extensions Using LSP Framework #14671

Conversation

niteskum commented Mar 20, 2019 • edited by shubhsnov Loading

niteskum commented Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubhsnov Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

shubhsnov commented Mar 20, 2019

shubhsnov commented Mar 21, 2019

shubhsnov commented Mar 21, 2019 • edited Loading

niteskum commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubhsnov commented Mar 21, 2019

niteskum commented Mar 22, 2019

shubhsnov commented Mar 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niteskum commented Mar 22, 2019

shubhsnov commented Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niteskum commented Mar 23, 2019

niteskum commented Mar 23, 2019

niteskum commented Mar 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubhsnov commented Mar 28, 2019

shubhsnov commented Mar 28, 2019 • edited Loading

swmitra commented Apr 2, 2019

niteskum commented Apr 2, 2019

shubhsnov commented Apr 2, 2019

niteskum commented Mar 20, 2019 •

edited by shubhsnov

Loading

niteskum commented Mar 20, 2019 •

edited

Loading

shubhsnov Mar 21, 2019 •

edited

Loading

shubhsnov commented Mar 21, 2019 •

edited

Loading

shubhsnov commented Mar 22, 2019 •

edited

Loading

shubhsnov commented Mar 28, 2019 •

edited

Loading