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

PHP Integration #983

Merged
merged 13 commits into from
Mar 21, 2024
Merged

PHP Integration #983

merged 13 commits into from
Mar 21, 2024

Conversation

tuxology
Copy link
Contributor

@tuxology tuxology commented Feb 29, 2024

  • integrate PHP Frontend from upstream Joern into Privado
  • vendor PHP Parser from upstream fork into Privado and use it
  • Use 4GB heap size for the tests to run in CI.

@tuxology tuxology marked this pull request as ready for review February 29, 2024 20:26
Copy link
Member

@khemrajrathore khemrajrathore left a comment

Choose a reason for hiding this comment

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

Overall changes look good to me, If we can also have some test cases around PHP parsing and tagging that would be great

@karan-batavia
Copy link
Contributor

karan-batavia commented Mar 1, 2024

Can we please have some tests on the basic tagging process? Let's ensure the integration stays solid from the start itself.
@pawan-privado

@pawan-privado
Copy link
Contributor

Can we please have some tests on the basic tagging process? Let's ensure the integration stays solid from the start itself. @pawan-privado

Sure!

@karan-batavia
Copy link
Contributor

karan-batavia commented Mar 1, 2024

Also, when are we planning to release these changes? Let's make sure we don't conflict with the C# release on Monday.
@pawan-privado @tuxology

build.sbt Outdated Show resolved Hide resolved
@pawan-privado
Copy link
Contributor

@tuxology Test failed because PHP parser could not be executed.

Error:  Invalid path for PhpParserBin: /home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/io/joern/php2cpg/bin/php-parser/php-parser.php

The path doesn't look right to me. Is this the intended path for vendoring the parser?

@tuxology
Copy link
Contributor Author

tuxology commented Mar 6, 2024

@tuxology Test failed because PHP parser could not be executed.

Error:  Invalid path for PhpParserBin: /home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/io/joern/php2cpg/bin/php-parser/php-parser.php

The path doesn't look right to me. Is this the intended path for vendoring the parser?

@pawan-privado It looks like this is only happening in Github - the path for php-parser is possibly null and hence the default path is being picked (which is not vendored path). This was working before C# push (a883357) so if you can investigate and fix, it would help 👍

@tuxology tuxology requested a review from khemrajrathore March 6, 2024 18:05
@pawan-privado
Copy link
Contributor

pawan-privado commented Mar 6, 2024

@tuxology Test failed because PHP parser could not be executed.

Error:  Invalid path for PhpParserBin: /home/runner/.cache/coursier/v1/https/repo1.maven.org/maven2/io/joern/php2cpg/bin/php-parser/php-parser.php

The path doesn't look right to me. Is this the intended path for vendoring the parser?

@pawan-privado It looks like this is only happening in Github - the path for php-parser is possibly null and hence the default path is being picked (which is not vendored path). This was working before C# push (a883357) so if you can investigate and fix, it would help 👍

The reason why we did not notice this happen in our dev machines is because we had the required env variable set and the path was being picked up from the variable. I've made changes accordingly.

Copy link

@kbatavia12 kbatavia12 left a comment

Choose a reason for hiding this comment

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

LGTM

karan-batavia
karan-batavia previously approved these changes Mar 20, 2024
karan-batavia
karan-batavia previously approved these changes Mar 20, 2024
@khemrajrathore
Copy link
Member

khemrajrathore commented Mar 21, 2024

@pawan-privado Take a merge of dev, they we are good to go

pawan-privado and others added 4 commits March 21, 2024 16:58
* Check-in `PhpProcessor`

* Check in taggers

* Fix crash on scanning and address review comment

* fix: `PhpProcessor` should take in `S3DatabaseDetailsCache`

---------

Co-authored-by: Suchakra Sharma <[email protected]>
Copy link
Member

@khemrajrathore khemrajrathore left a comment

Choose a reason for hiding this comment

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

LGTM

@khemrajrathore khemrajrathore merged commit cafd939 into dev Mar 21, 2024
12 checks passed
@khemrajrathore khemrajrathore deleted the php-integration branch March 21, 2024 13:43
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.

5 participants