search

bmewburn / vscode-intelephense

PHP intellisense for Visual Studio Code
https://intelephense.com
Other
1.64k stars 94 forks source link

Strange formatting on `__halt_compiler` #624

Closed sudomabider closed 5 years ago

sudomabider commented 5 years ago

I understand PSR12 wants __HALT_COMPILER to be __halt_compiler and am happy with that. However the formatter inserts a space between the keyword and the parentheses, i.e. __halt_compiler (). While not a major issue, some of the phar related tools/libraries look for this call and can't find it due to the space.

sudomabider commented 5 years ago

Hold the horses man...I should have tested this out first. Apologies.

Turns out __halt_compiler() actually doesn't work. Only __HALT_COMPILER() or __halt_compiler () works. I'm not sure why since the manual clearly shows __halt_compiler(). Very very weird. But apparently I'm not the first one to encounter this: https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/754

I'm guessing this is why intelephense formats it to __halt_compiler (). Not sure in the context of PSR12 what is the best option here since the PSR wants it in lowercase but no space in between as well...

Either way this is probably beyond the scope of intelephense. Feel free to close.

bmewburn commented 5 years ago

I tried __halt_compiler() on php 7.0 and it was ok. The issue you linked looks old, specific to a minor version of php 5.3? Which version of php are you on? For the formatting you are correct in that it shouldn't have the space, it's not actually a function but should be treated the same as the other function-like language constructs like isset(), empty(), die(). So I'm inclined to fix it.

sudomabider commented 5 years ago

The issue is specific to phar (which is where most of the __halt_compiler call happens). I checked phpunit, php-cs-fixer, phpcs, phpab and phive itself. All use the uppercase version, which is consistent with the php manual in the context of phar, as opposed to the generic function reference. phpcs even explicitly skips the check for this function: https://github.com/squizlabs/PHP_CodeSniffer/pull/2377/commits/29a00f5bd629599f2bb30f693e4129420414d5ad

My impression is that this is a well known issue in the world of phar. But since I only very recently worked with phar for the first time I didn't find out until later.

However in the context of intelephense, I'm not sure if it should be aware of this issue or just leave it up to the user. I tend to agree as well that intelephense should stay neutral in this case. The only downside is that now I have to disable formatOnSave for even personal phar projects.

bmewburn commented 5 years ago

Thanks for investigating this @sudomabider . I'll remove the space still to maintain consistency there but I will not force the lowercasing and will offer the uppercase keyword as a completion suggestion too.

sudomabider commented 5 years ago

That sounds like a great idea. Loving your work 👍