[CLOSED] Php Tooling Extensions Using LSP Framework

[core-ai-bot]

Issue by niteskum
Wednesday Mar 20, 2019 at 07:22 GMT
Originally opened as adobe/brackets#14671

---

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

Requirements:

• github organisation  #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 included the following code: https://github.com/adobe/brackets/pull/14671/commits

[core-ai-bot]

Comment by niteskum
Wednesday Mar 20, 2019 at 07:24 GMT

---

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

@shubhsnov@swmitra@subhashjha333 please review

[core-ai-bot]

Comment by shubhsnov
Wednesday Mar 20, 2019 at 14:15 GMT

---

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

[core-ai-bot]

Comment by shubhsnov
Thursday Mar 21, 2019 at 09:23 GMT

---

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

[core-ai-bot]

Comment by shubhsnov
Thursday Mar 21, 2019 at 10:35 GMT

---

@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.

[core-ai-bot]

Comment by niteskum
Thursday Mar 21, 2019 at 11:33 GMT

---

@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.

[core-ai-bot]

Comment by shubhsnov
Thursday Mar 21, 2019 at 13:07 GMT

---

@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.

[core-ai-bot]

Comment by niteskum
Friday Mar 22, 2019 at 06:44 GMT

---

@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.

[core-ai-bot]

Comment by shubhsnov
Friday Mar 22, 2019 at 09:22 GMT

---

@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.

[core-ai-bot]

Comment by niteskum
Friday Mar 22, 2019 at 17:39 GMT

---

@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.

[core-ai-bot]

Comment by shubhsnov
Friday Mar 22, 2019 at 23:45 GMT

---

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.

[core-ai-bot]

Comment by niteskum
Saturday Mar 23, 2019 at 06:40 GMT

---

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.

[core-ai-bot]

Comment by niteskum
Saturday Mar 23, 2019 at 16:08 GMT

---

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

[core-ai-bot]

Comment by niteskum
Sunday Mar 24, 2019 at 19:53 GMT

---

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

[core-ai-bot]

Comment by shubhsnov
Thursday Mar 28, 2019 at 05:43 GMT

---

@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.

[core-ai-bot]

Comment by shubhsnov
Thursday Mar 28, 2019 at 05:44 GMT

---

LGTM 👍 💯

[core-ai-bot]

Comment by swmitra
Tuesday Apr 02, 2019 at 09:39 GMT

---

Please resolve the conflicts@niteskum.

[core-ai-bot]

Comment by niteskum
Tuesday Apr 02, 2019 at 09:52 GMT

---

Please resolve the conflicts@niteskum.

resolved conflicts in strings.js

[core-ai-bot]

Comment by shubhsnov
Tuesday Apr 02, 2019 at 10:16 GMT

---

@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.