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

WIP: Add config to TestCpg and filter php files #2733

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

Conversation

johannescoetzee
Copy link
Contributor

@johannescoetzee johannescoetzee commented May 17, 2023

This introduces another way of filtering input files, so there's a TODO to go through the frontends and extract common functionality into x2cpg.

@johannescoetzee johannescoetzee changed the title Add config to TestCpg and filter php files WIP: Add config to TestCpg and filter php files May 19, 2023
@johannescoetzee
Copy link
Contributor Author

Changing to WIP pending #2741

@@ -28,7 +29,11 @@ private object Frontend {
programName("php2cpg"),
opt[String]("php-ini")
.action((x, c) => c.copy(phpIni = Some(x)))
.text("php.ini path used by php-parser. Defaults to php.ini shipped with Joern.")
.text("php.ini path used by php-parser. Defaults to php.ini shipped with Joern."),
opt[Seq[String]]("exclude-overrides")
Copy link
Contributor

Choose a reason for hiding this comment

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

can we split this in two? I think from an end-user's perspective they either want to disable the default exclusions or add additional exclusions, but rarely both at the same time.

"<unknown>"
).map(PathFilter.standardisePath)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

with other frontends we've seen bugs with filtering for untested combinations of absolute and relative paths for a) the input path and b) the excluded path. Can you add some tests for here to ensure that works as intended in all cases?

@max-leuthaeuser
Copy link
Contributor

Sorry, I did not see this PR.
Yeah, #2741 should make filtering easier.

@johannescoetzee
Copy link
Contributor Author

Sorry, I did not see this PR. Yeah, #2741 should make filtering easier.

No worries! The extracted filtering is definitely the better way to go

@gacevedo
Copy link
Contributor

@max-leuthaeuser with @johannescoetzee on vacation, could you please refactor this PR to make use of #2741? Needed to fix #2703. Thank you!

@gacevedo
Copy link
Contributor

@max-leuthaeuser with @johannescoetzee on vacation, could you please refactor this PR to make use of #2741? Needed to fix #2703. Thank you!

Never mind. Looks like the PR was reverted.

@DavidBakerEffendi DavidBakerEffendi added the php Relates to php2cpg label Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
php Relates to php2cpg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants