search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] Php Tooling Extensions Using LSP Framework #11969

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by niteskum Wednesday Mar 20, 2019 at 07:22 GMT Originally opened as https://github.com/adobe/brackets/pull/14671

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

Requirements:

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

LGTM 👍 💯

core-ai-bot commented 3 years ago

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

Please resolve the conflicts@niteskum.

core-ai-bot commented 3 years ago

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

Please resolve the conflicts@niteskum.

resolved conflicts in strings.js

core-ai-bot commented 3 years ago

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.