PHP language support

[morgante]

We would like to support PHP as a language for matching against.

Review this guide and join us on Discord to discuss.

Acceptance criteria

1. Working grammar + language support
2. At least 10 test cases, including rewrites and metavariables.

[morgante]

/bounty $1500

[abhishek818]

/attempt #48

Algora profile	Completed bounties	Tech	Active attempts	Options
@abhishek818	1 bounty from 1 project	JavaScript, TypeScript		Cancel attempt

[morgante]

@iamnamananand996 Do you want to attempt this bounty?

[abhishek818]

@morgante can I get assigned?

[urbit-pilled]

/attempt #48
I've made a few tools using tree-sittter (https://github.com/urbit-pilled/tree-sitter-hoon and https://github.com/urbit-pilled/hoon-ts-editors) so I'd like to work on this

Algora profile	Completed bounties	Tech	Active attempts	Options
@urbit-pilled	3 bounties from 2 projects	Rust, Scheme, Scala & more		Cancel attempt

[iamnamananand996]

@iamnamananand996 Do you want to attempt this bounty?

hi @morgante, yes I am on it, look like a race now, 😃, let see who wins.

[morgante]

@abhishek818 @urbit-pilled @iamnamananand996 I will assign this bounty to someone once they land the first main PR (ex. adding the grammar + parse test + initial metavariables).

[algora-pbc]

💡 @urbit-pilled submitted a pull request that claims the bounty. You can visit your bounty board to reward.

[morgante]

@urbit-pilled asked about the prefix to use since PHP uses $ for its own variables.

For now, just override the metavariable to something like %. This is simplest.

Alternative, we can use $[name] for Grit metavariables in PHP, since $[name] is not a valid PHP variable.

[ayewo]

@morgante

The tree-sitter-php project adopted the same split as tree-sitter-typescript two months ago¹ where they have two dialects: php_only and php.

• php_only: which allows top level statements, start/end tags are optional;
• php: which parses both PHP and HTML (i.e. normal PHP parser since PHP started out as a templating language).

1. Which of these two grammars should we pick for the initial support? I imagine this will be php_only?
2. How do we approach naming of the two dialects within the Grit project? Naming is hard and agree that in an ideal world² it would have been better to use the names:
   • php_only => php
   • php => php_html

Footnotes

1. https://github.com/tree-sitter/tree-sitter-php/pull/192 ↩

2. https://github.com/tree-sitter/tree-sitter-php/pull/180#issuecomment-1774160626 ↩

[morgante]

Hi @ayewo, thanks for flagging this.

I think we probably want to use php (with HTML) as the default, since that matches the largest number of PHP codebases in the wild.

For toggling between them, we should use the flavor mechanism we added for JS/TS here. This would support language php(html) or language php(only) with the default as php(only).

[urbit-pilled]

Finally got metavariables to work in my PR, this commit contains the biggest changes I had to make: :197575d

[morgante]

/assign @urbit-pilled

[digital-phoenix]

@morgante I just submitted a pull request that solves this issue why are you assigning this to someone else?

[morgante]

@digital-phoenix I'm sorry you went into the effort of submitting a pull request, but we can only award the bounty to one person. @urbit-pilled's PR is earlier and closer to completion.

[digital-phoenix]

@morgante @urbit-pilled's test cases are not even generating abstract syntax trees yet it is just doing regex comparisons. In order for php to see text as code it has to be contained within php tags. My test cases work not just as regex comparisons but as actual code.

[morgante]

I don't think <?php is required when using the php-only grammar.

[digital-phoenix]

@morgante I tested it before when I was adding php. Unless you use <?php the test will just be interpreted as text. Just using regex leads to issues such as not being able to match a function body when it is split across multiple lines.

[morgante]

Language support is usually not completed in a single PR. I plan to merge @urbit-pilled's PR and award part of the bounty, but will award additional bounties for PRs adding more test cases and fixing associated bugs.

[digital-phoenix]

@morgante @urbit-pilled's grammar file does not seem to even compile correctly. It seems to contain conflicts that have not been included in the conflict table I'm not convinced they have ever even used their own grammar file. Whereas my code includes support for a broad range of language features.

[morgante]

If the grammar file wasn't used I don't see how the test cases could pass.

This is an open source project, I prefer to have a collaborative community where we all focus on producing good code not tearing down each other's work.

[digital-phoenix]

@morgante I understand the importance of collaboration but you're backing code that doesn't work properly over everyone else's efforts

[morgante]

@morgante I understand the importance of collaboration but you're backing code that doesn't work properly over everyone else's efforts

90% of the work is the same in your PRs. It would not be fair to discard the earlier PR, even if it is incomplete.

As I noted, you will have an opportunity to add (and fix) more test cases for your share.

For what it's worth, this is making me see the problems with the bounty model. We put this out as an experiment, but it's becoming a lot of trouble to manage.

[ayewo]

For what it's worth, this is making me see the problems with the bounty model. We put this out as an experiment, but it's becoming a lot of trouble to manage.

I was trying to point this out this very scenario playing out in Discord. Glad you've also come to the same conclusion—it can cause some bad blood for contributors and logistical headaches for projects that is completely avoidable.

As I said, battle royale style, only works well if the bounty is a few hours of work. This is a multi-day PR.

[digital-phoenix]

@morgante if you are going to have a public bounty either you need to accept whoever provides the first work that solves the requirements or you need to assign the task to someone.

Most of the code will be the same because this project requires a lot of boiler plate to add a language. But the most important part the changes to the base grammar files are different.

[digital-phoenix]

@ayewo battle royale style is a lot riskier but I think it can work okay as long as the submissions are easy to measure and are all compared against a well defined benchmark.

[morgante]

@digital-phoenix As far as I know, I am not required to accept any pull request. If you prefer not to contribute to our repository, I understand, but I'm just trying to be fair to everyone. You knew there were already PRs open, that I was engaged with and reviewing, and chose to start a new PR anyways.

I didn't assign it to anyone up front because, frankly, many contributors aren't prepared to work on something like this and I wanted to see material demonstration of ability before putting it on a single person. I have learned my lesson and will avoid big bounties in the future.

[digital-phoenix]

@morgante if you base this off of whoever opens a PR first that just encourages people to open broken buggy PRs. Which will just make work for you.

[urbit-pilled]

@morgante @urbit-pilled's grammar file does not seem to even compile correctly. It seems to contain conflicts that have not been included in the conflict table I'm not convinced they have ever even used their own grammar file. Whereas my code includes support for a broad range of language features.

@morgante @urbit-pilled's test cases are not even generating abstract syntax trees yet it is just doing regex comparisons. In order for php to see text as code it has to be contained within php tags. My test cases work not just as regex comparisons but as actual code.

You may be testing an older version? Because the grammar file does compile correctly. And the test cases are not regex matches, and the php tags are optional if you set php_only as the default which was requested earlier in the comment thread.

I think you started too late @digital-phoenix , because your PR doesn't implement the prefix change (^ instead of $) or support php without tags yet. I already finished what you did in the first day of my PR (get php working without prefix), however you did add more tests.

Sumbitting a PR this late when someone else is nearly finished is bad, especially when you haven't fully fixed the issue or caught up to my progress yet.

I am new to bounties and this is the first time I've had a bad experience with another bounty hunter trying to do this.

[morgante]

Thanks @urbit-pilled for landing the main PR here. I'll award $1k from the bounty for that. There's still a bit more work to be done to consider this complete:

• PHP templates (ie. in HTML) should be supported as an alternative flavor (like we have in JS) with language php(html)
• We definitely need more test coverage, I'd like at least 5-6 more cases.

[ayewo]

I'll award $1k from the bounty for that.

@morgante Algora has a command for making partial awards on GitHub. Simply add a comment on this issue with:
/tip $1000 @urbit-pilled

Apologies if you already know this.

[morgante]

/tip $1000 @urbit-pilled

[algora-pbc]

👉 @morgante: Navigate to your dashboard to proceed

[urbit-pilled]

Thanks morgante, I'll work on the rest today.

[morgante]

Tests cases that should work:

• echo $_

[urbit-pilled]

hey @morgante you need to click on the link posted by algora-pbc to award the tip. it hasn't been awarded yet.

👉 @morgante: Navigate to your dashboard to proceed

[urbit-pilled]

Tests cases that should work:

• echo $_

just added a test case for this in crates/core/src/test.rs

`echo ^_;` => `print "modified;`

However instead of echo $_ we use echo ^_ because:

• the prefix in php is ^ instead of $
• there is a semicolon needed due to: feat: add PHP language #52 (comment)

[morgante]

/tip $250 @urbit-pilled

[urbit-pilled]

@algora-pbc what are the correct commands to use when paying out a bounty incrementally?
Because I haven't received any of it yet...

[algora-pbc]

🎉🎈 @urbit-pilled has been awarded $1,000! 🎈🎊

• Share on socials
• Give feedback

[algora-pbc]

🎉🎈 @urbit-pilled has been awarded $250! 🎈🎊

• Share on socials
• Give feedback

[urbit-pilled]

Thanks Morgante, I received the $1,250 tip. Anything else that needs to be done for the rest of this bounty @morgante?

[morgante]

Hi @urbit-pilled, before we consider a language "complete" we usually like to try it on a few repos and run some sample migrations. For this to be complete, I think I'd like to see:

• Run this on some larger open source codebases and make sure you don't see tons of parse errors
• Add 1-2 sample rules to the PHP stdlib: https://github.com/getgrit/stdlib/tree/main/.grit/patterns

[varshith257]

@morgante Anything left with this to complete the issue further. From conversations I have seen there is some part of bounty to add test cases more and increase test coverage.

Would I proceed with it ?

[morgante]

@varshith257 We haven't seen much PHP usage so I'm closing this out. No further bounty is available.