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

include php blade plugin in php cluster #7618

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

haidubogdan
Copy link

@haidubogdan haidubogdan commented Jul 29, 2024

Laravel is one of the most used frameworks in php, yet it still doesn't have support in Netbeans. (#7531 , #7231).
Mostly the main missing support is for blade templates syntax.

I've started to work on a plugin https://github.com/haidubogdan/netbeans-php-blade-plugin 3 years ago.
After using antlr as a lexer and parser, I found that scaling the plugin was much maintainable.
It's not the cleanest code, but I realized that I will always reach to the 99% finish status if I don't do the first pull request.

Most of the available features to be included are listed at https://github.com/haidubogdan/netbeans-php-blade-plugin.

  • syntax highlighting
  • blade syntax completion
  • declaration finder for specific blade template
    ....

And some are still missing

  • php embedded syntax validation
  • more advanced php syntax completion.

All feedbacks are welcome, I hope to reach nb 23 deployment :) .

TODO

  • Submit an ICLA (sent on 3 august)
  • Change to the full name in commits
  • Check the license of the image (no response received from Tailor [laravel], using personal icon)
  • Add unit tests
  • Write all features with screenshots

@ebarboni ebarboni requested a review from junichi11 July 29, 2024 08:56
@junichi11 junichi11 added the PHP [ci] enable extra PHP tests (php/php.editor) label Jul 30, 2024
@junichi11
Copy link
Member

First of all, thank you for your contribution!

I hope to reach nb 23 deployment

Unfortunately, it's too late. The feature freeze date is July 26th.

Please write all features of this module with screenshots here as well. (not only the link)

Please add unit tests for features. e.g. code completion, indexer, navigator, parser, lexer, formatter etc. (also see: CslTestBase.java)

Did you submit an ICLA?

Probably, it takes a lot of time to review this.

@junichi11 junichi11 requested a review from tmysik July 30, 2024 05:03
@junichi11 junichi11 added this to the NB24 milestone Jul 30, 2024
@haidubogdan
Copy link
Author

Hi,
thank you for your feedback.
I will prepare the unit tests. ( I hope the base parser unit test classes will be available now that it's included in the IDE folder).
I didn't submit the ICLA. I will read about it.

@junichi11
Copy link
Member

How did you generate icons? (Are there icons based on something?)

https://github.com/apache/netbeans/pull/7618/files#diff-24c53d9c7defe885e1ee8adee9ff009c57d69e75621ad84625d8f9eaa8d7c9e1

@Override
public String getCustomInsertTemplate() {
if (namespace != null && namespace.length() > 0) {
return "\\" + namespace + "\\" + element.getName();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can change these("\\") to constants?

}
break;
case HTML_COMPONENT_PREFIX:
String compPrefix = currentToken.getText().length() > 3 ? StringUtils.kebabToCamel(currentToken.getText().substring(3)) : "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does 3 mean?
// NOI18N

@junichi11
Copy link
Member

junichi11 commented Sep 3, 2024

Please check whether we should translate yourself once for strings of all files. (add // NOI18N or @Messages)
Please check whether annotations are needed. (e.g. @CheckForNull, @NullAllowed,...)

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't forget to hook the tests into CI so that we can let it run over this PR - just in case there are any surprises.

would look like:

- name: php.latte
run: ant $OPTS -f php/php.latte test

and someone can trigger the workflow once done

Comment on lines 62 to 64
return this.cacheMap.keySet().parallelStream()
.filter(this::isExpired)
.collect(Collectors.toSet());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you sure parallelStream() is a good idea here? It always comes with setup and runtime overhead and all this stream does is to filter a set - so the parallel task itself is tiny.

I would only recommend using parallel streams if it is also demonstrated in a benchmark that they make a difference in that usecase.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a borrowed implementation, so I think I could simplify the flow to use simple stream

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I understand, I have nothing against the stream usage itself. But the fact that parallelStream() is used instead of stream().

Copy link
Member

@mbien mbien Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I think I get what you mean, there are two getExpiredKeys() methods which use parallelStream() you updated one so far. I didn't realize that there were two sorry.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah no :).
It was a short comment about my intention to replace the usage of parallelStream() with stream(), but I wasn't very clear.

Comment on lines 40 to 44
public static final List<Integer> TOKENS_WITH_IDENTIFIABLE_PARAM = Arrays.asList(new Integer[]{
D_EXTENDS, D_INCLUDE, D_INCLUDE_IF, D_INCLUDE_WHEN,
D_INCLUDE_UNLESS, D_EACH, D_SECTION, D_HAS_SECTION, D_SECTION_MISSING,
D_PUSH, D_PUSH_IF, D_PREPEND, D_USE, D_INJECT, D_ASSET_BUNDLER
});
Copy link
Member

@mbien mbien Sep 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List.of would make this immutable. Should this be a Set? (see next comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately this didn't solve the issue :(

the array and the set are both mutable + you don't seem to use the array anywhere outside of this class.

public static final Set<Integer> TOKENS_WITH_IDENTIFIABLE_PARAM = Set.of(
    D_EXTENDS, D_INCLUDE, D_INCLUDE_IF, D_INCLUDE_WHEN,
    D_INCLUDE_UNLESS, D_EACH, D_SECTION, D_HAS_SECTION, D_SECTION_MISSING,
    D_PUSH, D_PUSH_IF, D_PREPEND, D_USE, D_INJECT, D_ASSET_BUNDLER
);

This would make it immutable.

you are using in many places lines like: new HashSet<>(Arrays.asList(new String[]{directive})); which could be Set.of(directive).

Comment on lines 721 to 727
private static final String _serializedATNSegment0 =
"\u0004\u0000\u00ab\u0b4c\u0006\uffff\uffff\u0006\uffff\uffff\u0006\uffff"+
"\uffff\u0006\uffff\uffff\u0006\uffff\uffff\u0006\uffff\uffff\u0006\uffff"+
"\uffff\u0006\uffff\uffff\u0006\uffff\uffff\u0006\uffff\uffff\u0006\uffff"+
"\uffff\u0006\uffff\uffff\u0006\uffff\uffff\u0006\uffff\uffff\u0002\u0000"+
"\u0007\u0000\u0002\u0001\u0007\u0001\u0002\u0002\u0007\u0002\u0002\u0003"+
"\u0007\u0003\u0002\u0004\u0007\u0004\u0002\u0005\u0007\u0005\u0002\u0006"+
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding generated code. I believe we ended up agreeing to try to generate antlr outputs during build but don't commit it to the repo. The reason is because generated antlr code is not very "stable", it produces huge diffs when the version updates and those aren't reviewable anyway. (e.g the marked section)

The second reason would be antlr updates, this makes sure the code fits to the version if someone happens to update the antlr lib at some point.

example: #7189, #7186 etc

cc @neilcsmith-net @lkishalmi

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will see how I can add it to .gitignore and have them build before the unit tests.
@junichi11 is it ok for you if I remove the generated antlr parser and lexer files from git ?

@haidubogdan haidubogdan force-pushed the 7531_php_blade branch 3 times, most recently from 97780ce to 3b74d77 Compare September 30, 2024 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Don't merge this PR, it is not ready or just demonstration purposes. PHP [ci] enable extra PHP tests (php/php.editor)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants