Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

constant suggestion of "<?php" in mixed file #47

Closed
racedaemon opened this issue Sep 25, 2017 · 64 comments · Fixed by #101
Closed

constant suggestion of "<?php" in mixed file #47

racedaemon opened this issue Sep 25, 2017 · 64 comments · Fixed by #101
Labels

Comments

@racedaemon
Copy link

While editing php files that include html the plugin suggests "<?php" whenever i'm outside a php code block. This makes html editing really tedious.

@vinkla
Copy link
Contributor

vinkla commented Sep 25, 2017

I've noticed this today as well. It never stops suggesting the <?php snippet abbreviation.

@jens1o
Copy link
Contributor

jens1o commented Oct 2, 2017

That's a feature: https://github.com/felixfbecker/php-language-server/blob/35f33c8c91222b8409006cc68738e4000e33a4a3/src/CompletionProvider.php#L153-L164

@mikebronner
Copy link

I think the problem is that it makes this suggestion regardless of where the cursor is in the code. It would be great if it limited this suggestion only to the beginning of the file, as the code comments suggest it should. :)

@jens1o
Copy link
Contributor

jens1o commented Oct 2, 2017

Hmm, that's worth a discussion. @felixfbecker Thoughts?

@felixfbecker
Copy link

Please see felixfbecker/php-language-server#372 (and upvote the linked LSP issue)

@sebj54
Copy link

sebj54 commented Oct 16, 2017

While what you use is open-source, can't anyone fork it and add a configuration, leaving the actual behavior as default?
This is really annoying and I'm considering removing the whole atom-ide plugin just because of this bug... :(

@damieng
Copy link
Contributor

damieng commented Oct 16, 2017

It's not easy to change - it's part of the behavior of the underlying language server.

If there is a way to switch it off in ide-php we'll definitely consider a configuration switch here with no need to fork.

@sebj54
Copy link

sebj54 commented Oct 16, 2017

Sure, but I meant by "fork" forking it and making a PR.
I don't know anything of the implementation, that's why I asked. But if it is hard to implement and we have to wait, it is really sad.

But what do we wait? This issue to be closed? microsoft/language-server-protocol#138

In that issue (felixfbecker/php-language-server#372), someone suggested to remove the following condition:

$node instanceof Node\Statement\InlineHtml

Maybe the switch you're talking about can be on that condition?

@felixfbecker
Copy link

Folks this is blocked by Microsoft VS Code / LSP. If you want to get a fix faster please express your need on the linked issue, I can't make it happen faster :)

@sebj54
Copy link

sebj54 commented Oct 16, 2017

The fact is we know what we want on your module, but every time lose me more about VS Code, LSP or whatever is used. The only thing I can do is upvote the issue or eventually sign a petition 😄
Can you "up" the issue on LSP to make them understand their code is used on an Atom extension that is getting more popular everyday?

Thanks for your patience anyway :)

@damieng
Copy link
Contributor

damieng commented Oct 16, 2017

@felixfbecker Is there any way we can have a short term configuration option that just specifically disables <?php ?

The alternative I guess is I could just always filter this out client side?

@jens1o
Copy link
Contributor

jens1o commented Oct 16, 2017

Personally, I think this would fit in Configuration Options(as described in the LSP), but these aren't implemented yet, afaik. Another simple operator is some kind of unmaintainable when it scales some day and it does not fit(Application Options shouldn't be passed via command line arguments). (but this is just my opinion)

@damieng
Copy link
Contributor

damieng commented Oct 16, 2017

@jens1o There are plenty of examples of extensions that people have added for additional functionality like this. Basically just need to come up with a new message and response or add a custom block to the initialization message.

@reneroth
Copy link

open ~/.atom/packages/ide-php/vendor/felixfbecker/language-server/src/CompletionProvider.php and remove || $node instanceof Node\Statement\InlineHtml from line 155.

yup, that's as dirty a hack as dirty hacks get to be dirty, and you'll need to do this or some variation thereof on every update, but it beats not being able to code efficiently due to <?phps being shat all over your code

@newelement
Copy link

I agree something needs to change every time I type any html <p it wants to complete with <?php

@felixfbecker
Copy link

Fixed in https://github.com/felixfbecker/php-language-server/releases/tag/v5.0.1
Atom should start sending CompletionContext to textDocument/completion to make sure the suggestion still shows up when requested explicitly.

@damieng
Copy link
Contributor

damieng commented Nov 18, 2017

Working on that right now - requires work in atom-languageclient and in autocomplete-plus. Will bump up once we're there.

@damieng
Copy link
Contributor

damieng commented Nov 28, 2017

0.7.1 is out and i'm unable to repro :)

@damieng damieng closed this as completed Nov 28, 2017
@vinkla
Copy link
Contributor

vinkla commented Nov 28, 2017

I'm running version 0.7.3 with Atom 1.22.1 and I'm still having this issue.

@felixfbecker
Copy link

@vinkla could you show some LSP logs of the completion request?

@vinkla
Copy link
Contributor

vinkla commented Nov 29, 2017

@felixfbecker do you mean this? Let me know if there is anything else I can do!

@damieng
Copy link
Contributor

damieng commented Feb 2, 2018

I think between this and the other multitude of issues with the php integration we're going to have to cease development on this package. It requires more work than we have resources available.

@vinkla
Copy link
Contributor

vinkla commented Feb 2, 2018

@damieng do you mean that this package will become deprecated or removed?

@bhhaskin
Copy link

bhhaskin commented Feb 2, 2018

Wait, what? There are issues so you are just going to abandoned the package? If that is the case then I will move away from atom as a whole. It is crazy to just shelve such an important feature.

@jens1o
Copy link
Contributor

jens1o commented Feb 2, 2018

Lol, I'm not using Atom anymore but vscode(with the same php-language-server), but I'm still impressed.

@damieng
Copy link
Contributor

damieng commented Feb 2, 2018

@bhhaskin It does not make sense to continue to try to support a package that is not providing a good experience for its users when we lack the resources to address the issues.

The Atom team is very small and while the other IDE packages we support have enjoyed some success (Typescript, C#, Java) it would seem that the PHP one suffers from significantly more issues and problems than the others we support.

@reneroth
Copy link

reneroth commented Feb 2, 2018

to everyone looking for a fix:

either open ~/.atom/packages/ide-php/vendor/felixfbecker/language-server/src/CompletionProvider.php and remove || $node instanceof Node\Statement\InlineHtml around line 155.

or use the fabulous Visual Studio Code instead

@felixfbecker
Copy link

I am very sorry to hear that! As far as I am aware I fixed the issue on the language server sde and the fix here would be simple and benefit all languages (provide the language server with the extra parameter)

@damieng
Copy link
Contributor

damieng commented Feb 2, 2018

@felixfbecker Which parameter is missing?

@felixfbecker
Copy link

felixfbecker commented Feb 2, 2018

@damieng judging from the logs in this comment, the CompletionContext parameter to textDocument/completion. It's needed in PHP to not auto-trigger on HTML closing brackets >, but do trigger when explicitly invoked and still auto-trigger on access operators ->. I implemented that in 5.0.1 in November.

@damieng
Copy link
Contributor

damieng commented Feb 2, 2018

@felixfbecker the newer atom-languageclient sends the context however the language server then throws;

image

@felixfbecker
Copy link

Oh, I wasn't aware of that. That's a bug, should be fixed with felixfbecker/php-language-server#592. Will merge/release once CI passes

@felixfbecker
Copy link

felixfbecker commented Feb 2, 2018

Should be fixed in v5.3.5

@vinkla
Copy link
Contributor

vinkla commented Feb 2, 2018

I've submitted a pull request with the new changes: #101

@vinkla
Copy link
Contributor

vinkla commented Feb 3, 2018

I can't replicate this in version 0.7.5 and it seems to be fixed.

Thanks @damieng and @felixfbecker!

@Kaspik
Copy link

Kaspik commented Feb 4, 2018

screen shot 2018-02-04 at 11 34 44

@vinkla @felixfbecker Is this really expected suggestion of <?php? (Note that I'm writing <di)

@vinkla
Copy link
Contributor

vinkla commented Feb 4, 2018

@Kaspik what version of Atom and ide-php are you running?

@Kaspik
Copy link

Kaspik commented Feb 4, 2018

@vinkla 1.23.3, ide-php 0.7.5

screen shot 2018-02-04 at 12 00 44

@vinkla
Copy link
Contributor

vinkla commented Feb 4, 2018

@Kaspik hmm, did you restart Atom after updating the packages?

@Kaspik
Copy link

Kaspik commented Feb 4, 2018

Sure I did. :) It seems to be happening everywhere outside PHP code in .php files where is also HTML/CSS code.

@vinkla
Copy link
Contributor

vinkla commented Feb 4, 2018

Please send a Gist which we could test to reproduce the autocompletion with.

@Kaspik
Copy link

Kaspik commented Feb 4, 2018

Created absolutely simple file - https://gist.github.com/Kaspik/253512be0f020b151201171a2484c72f
Important note - it starts happening only after saving the file - if you just hit CMD-N and start writing code, it works and it's not suggesting the <?php everywhere.

@vinkla
Copy link
Contributor

vinkla commented Feb 4, 2018

Tried to follow your steps but I can't reproduce the error:

screen shot 2018-02-04 at 12 17 17 pm

@Kaspik
Copy link

Kaspik commented Feb 4, 2018

Seems like your file type (right bottom corner) is HTML, right? Then it works. Once I switch it to PHP, it stops.
I tried disabling other packages and extensions, still the same... saved file with PHP type is problem.

screen shot 2018-02-04 at 12 24 17
screen shot 2018-02-04 at 12 24 30

@vinkla
Copy link
Contributor

vinkla commented Feb 4, 2018

Seems like your file type (right bottom corner) is HTML, right? Then it works. Once I switch it to PHP, it stops.

I've tried switching between HTML och PHP and I can't reproduce this at all.

Try removing the package, restart Atom, install the package and restart Atom again. Also check if the the language server reindex your project (in the bottom right corner).

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

Successfully merging a pull request may close this issue.