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

Problem injecting php #5

Closed
EmranMR opened this issue Jun 23, 2023 · 9 comments
Closed

Problem injecting php #5

EmranMR opened this issue Jun 23, 2023 · 9 comments

Comments

@EmranMR
Copy link
Owner

EmranMR commented Jun 23, 2023

All editors have an issue injecting php.
The problem arises because of the fact phpparser has injection query for html.

Temporary fix ideas:

  • try to fork php, and create a php-only tree-sitter parser with no injection.
  • change the package.json to reflect
@calebdw
Copy link
Contributor

calebdw commented Jun 24, 2023

Instead of forking the repo and maintaining a separate copy of the original, the best thing to do would be to submit a PR to the original repo that splits into two different parsers, similar to the markdown parser in which two parsers live in the same repo:

image

@EmranMR
Copy link
Owner Author

EmranMR commented Jun 24, 2023

Instead of forking the repo and maintaining a separate copy of the original, the best thing to do would be to submit a PR to the original repo that splits into two different parsers, similar to the markdown parser in which two parsers live in the same repo:

image

Very interesting @calebdw. Quick question 🤔, that doesn’t look like a submodule as well, are they maintaining two separate folders?

PHP is ever growing so the main branch would be changing frequently as the new versions come out. Wouldn’t that mean duplicate work for writing new php grammars inside two separate folders?

Now what I had in mind in terms of maintainability (which might not even be practical, as I am not really an expert in GitHub/git) was the following:

  1. fork the branch, do all the necessary adjustment, changing the parser name and somehow remove the html injection etc
  2. The tree-sitter-php evolves on its own branch
  3. whenever there is a new release we merge the main into the phpbase branch this way the “php only” version stays update to date by just a merge commit.

However that would mean either a separate forked project, or separate branch named phpbase/phpo in the main repo? But again as you mentioned not the most elegant solution.

I played around a bit with tree-sitter-php this morning to see if I can adjust, it became apparent easier said than done. I might get in touch in discord and see if I could ask for help from someone who has/had done work on the tree-sitter-php. They use a lot of external scanners and I am not sure how to adjust them for the new parser name. I get errors if I npm run build after changing the parser name. Narrowed it down to the way makefile figures the $PARSER_NAME and again not my forte. So hopefully I figure out how I can successfully generate and once done, I see if I can experiment with the grammar so that we can only inject php 🤞

@calebdw
Copy link
Contributor

calebdw commented Jun 25, 2023

@EmranMR,

No those are not submodules, and the common grammar between the two is in a folder the next level up. In the case of the PHP parser I've already split them up and will try to submit a draft PR to garner feedback from the project maintainers.

There's no duplicate work, the entire php grammer is in the php-only directory and the php grammer just parses the text and php_only sections and then injects those languages into the nodes

@EmranMR
Copy link
Owner Author

EmranMR commented Jun 25, 2023

@EmranMR,

No those are not submodules, and the common grammar between the two is in a folder the next level up. In the case of the PHP parser I've already split them up and will try to submit a draft PR to garner feedback from the project maintainers.

There's no duplicate work, the entire php grammer is in the php-only directory and the php grammer just parses the text and php_only sections and then injects those languages into the nodes

Oh wow, you have already done all the work + the PR draft!! 🎉🎉
Can’t wait to actually clone and try it out myself as well. Hopefully there are no issues and we get the merge soon 🤞 one of the roadblocks out the way, one step at a time.

@stormherz
Copy link

@calebdw sorry for bothering you about this. It seems like split parser support has landed (many thanks for that effort), but I wasn't able to verify it on my local setup (I'm the author of #51).

Judging from the code of tree-sitter-blade, I'm assuming treesitter won't build php_only parser right away during :TSInstall, since Makefile still uses php parser by default, is this correct or am I missing the plot entirely?

@calebdw
Copy link
Contributor

calebdw commented Jan 10, 2024

@stormherz no need to be sorry! There's an open PR to add the needed configs so that nvim-treesitter knows about php_only, but until that is merged you could always manually add it to treesitter

@stormherz
Copy link

@calebdw thanks! totally forgot about that option. works like a charm, actually

@calebdw
Copy link
Contributor

calebdw commented Jan 22, 2024

php_only is now in nvim-treesitter so this issue can probably be closed

@EmranMR
Copy link
Owner Author

EmranMR commented Jan 22, 2024

@calebdw Amazing! thank you again for sorting that out!

@EmranMR EmranMR closed this as completed Jan 22, 2024
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

No branches or pull requests

3 participants