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

Skip already visited files #178

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mihai-stancu
Copy link

@mihai-stancu mihai-stancu commented Aug 24, 2024

This PR attempts to fix the bugs reported in:

A self-referencing file or a cyclic dependency would generate an infinite loop during parsing.

Code example:

// test.php -- name of file must match the require_once path
<?php

require_once ('test.php');

The chosen solution is that the parser should keep track of files it has already visited and skip reparsing them.


All Submissions:

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you lint your code locally prior to submission?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@mihai-stancu
Copy link
Author

I opened this for review but left it as [wip] until I finish adding an integration test (I already added a unit test).

… cover syntactic edge-cases for switch cases
@mihai-stancu mihai-stancu changed the title [wip] Skip already visited files Skip already visited files Aug 28, 2024
@mihai-stancu
Copy link
Author

Given that the bugfix resides in the AbstracCollector it doesn't require a backwards compatibility flag, there is no valid usecase for the collector to enter an infinitely loop.

@mihai-stancu
Copy link
Author

I have added an integration test, but the test's assertion is fairly weak -- the empty file it loads has no symbols so I assert that it collects 0 symbols. Since I can't stop the infinite loop early (?) then I can't really check for the result of the collector and make assertions there.

However, without this PR the test would fail due to the infinite loop by exhausting all allocated memory.

Do you have any better strategies to test this?

My only other idea is to use the $includeCallback again and throw an exception if I receive the same path twice.
Throwing an exception from the callback would stop the infinite loop but since the desired case doesn't expect the Exception then the test will still fail due to an unexpected exception (instead of the exhausted memory).

…ips already visited files (integration test)
@mihai-stancu
Copy link
Author

Missed a file from the commit. Please reapprove workflows.

@mihai-stancu
Copy link
Author

@icanhazstring did you get a chance for a review on this?

@mihai-stancu
Copy link
Author

Ping

@icanhazstring
Copy link
Member

Hi. Sorry might have some time later that week.
Had to attend some family occasions.

@mihai-stancu
Copy link
Author

@icanhazstring ping

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.

2 participants