Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor fixes to PHP syntax #22

Closed
wants to merge 6 commits into from
Closed

Minor fixes to PHP syntax #22

wants to merge 6 commits into from

Conversation

jobedom
Copy link

@jobedom jobedom commented Jun 20, 2015

I am the creator of the PHP Modern package. I've decided to deprecate it and add any enhancement it contains to the official PHP package, so I'll be sending PRs for any past and future improvements.

@gerardroche
Copy link
Contributor

Some of these are not minor fixes. Without extensive tests I don't see how they can be accepted.

For example trait should match meta.trait.php storage.type.trait.php. See https://github.com/gerardroche/sublime-php-grammar

@jobedom
Copy link
Author

jobedom commented Jun 20, 2015

I'll be happy to provide any needed tests. What is the procedure or actual mechanics of testing a grammar?

@gerardroche
Copy link
Contributor

@gerardroche
Copy link
Contributor

break it down into smaller commits. The finally and yield keywords don't need tests.

@jobedom
Copy link
Author

jobedom commented Jun 20, 2015

Great! Thanks a lot, gerardroche. I'll create needed tests.

Is your package sublime-php-grammar an unofficial one?

@gerardroche
Copy link
Contributor

Yes it's unofficial.

@jobedom
Copy link
Author

jobedom commented Jun 20, 2015

So I'm guessing it's not an easy path asking for integration into the official PHP package. I'm checking yours and maybe it has all I need.

@gerardroche
Copy link
Contributor

It's a tough one. I think most if not all the changes in php-grammar will be accepted. Even if a few small things are not accepted I would prefer to contribute to the default and run with that.

Splitting the packages into their own repositories and the ability to disable/override default completions and snippets. These are prerequisite to integrating php-grammar into the default package.

@jobedom
Copy link
Author

jobedom commented Jun 20, 2015

I get it. Thanks again for all the info.

@rtheunissen
Copy link

Thanks for taking the time to do this @jobedom. I'm on the edge of switching to another editor simply because the PHP highlighting is broken, so I very much appreciate the effort to fix it. 👍

@Gerst20051
Copy link

👍

1 similar comment
@vinkla
Copy link

vinkla commented Jul 2, 2015

👍

@jbrooksuk
Copy link
Contributor

Yeah, I like whatever option improves PHP highlighting.

@svivian
Copy link

svivian commented Jul 8, 2015

Hey, just want to note a couple of issues with the current PHP syntax highlighter. If you've already fixed these then just ignore me :)

First, type hints and default arguments don't play nice. In the following code the [] gets marked in dark red (indicating an error):

<?php
function test(array $arr=[]) {
    // code
}

Each works individually (i.e. array $arr or $arr=[]) but not together. Oddly, array $arr=array() works fine.

On a similar topic, since PHP7 beta was just released I guess you might want to add string/int/float/bool as keywords and ensure they work with type hints.

Finally, when using DocBlock comments I regularly get the "dark red background of death". As soon as I type /** everything below goes red because the following lines don't start with * (yet). Is it within the scope of this package to automatically format DocBlocks, or is that elsewhere? So when you type /** and press Enter, you get something like the following, where | is the cursor:

/**
 * |
 */

@gerardroche
Copy link
Contributor

@svivian array type hints issue duplicate here jskinner/DefaultPackages#17 should be fixed in php-grammar gerardroche/sublime-php-grammar@388eae9

@svivian php7 features would be good. Open specific issues for these.

@svivian Try docblockr https://github.com/spadgos/sublime-jsdocs Perhaps the docblock feature you mention should be part of a php package or at least the basic feature. But the docblockr package covers all languages not just php.

@svivian
Copy link

svivian commented Jul 8, 2015

@gerardroche great, thanks for the info.

DocBlockr has the functionality I suggested which is great, but there is the still the issue with the dark red background. When I type /** a huge chunk of code goes red momentarily which is rather ugly.

Is that really necessary? It's perfectly valid PHP to have a comment starting /** with anything inside it.

@gerardroche
Copy link
Contributor

@svivian open an issue. I kinda agree with you. While it might be an invalid phpdoc it's not an invalid comment. Perhaps the scope could be changed to something else to indicate it's an invalid phpdoc but not an invalid comment.

@rtheunissen
Copy link

@jobedom What's your status regarding this PR? I'm still using PHP Modern because the official package is too far behind. PHP 7 is ready and ST doesn't even support 5.4 properly. Thanks for your work on PHP Modern.

@gerardroche Do you know when we might expect some action here? I find it ridiculous that something like syntax highlighting is broken on arguably the most popular text editor for a widely-used language, 2 months after a partial patch was put forward. If the procedure to get these merged is that difficult, I don't see why these were made open in the first place.

@gerardroche
Copy link
Contributor

@rtheunissen PHP Grammar, which includes these fixes and many other fixes and improvements is available on package control https://packagecontrol.io/packages/php-grammar.

@wbond
Copy link
Member

wbond commented Apr 20, 2016

All of these features have been implemented in the master branch, along with tests.

In addition, the PHP syntax has improved performance, from removing regex constructs incompatible with the new Sublime Text regex engine.

Please take the time to check out your PHP with the new version and report any bugs you find, or features from PHP7 that are missing. https://github.com/sublimehq/Packages#installation has instructions on how to use the latest version of the syntax on your machine.

Issues should be filed at https://github.com/sublimehq/Packages/issues, and please be sure to include the a snippet of code that exposes the bug.

@wbond wbond closed this Apr 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants