Skip to content

Feature/php strategy#86

Open
Sangoku wants to merge 2 commits into
johnhuang316:masterfrom
Sangoku:feature/php-strategy
Open

Feature/php strategy#86
Sangoku wants to merge 2 commits into
johnhuang316:masterfrom
Sangoku:feature/php-strategy

Conversation

@Sangoku

@Sangoku Sangoku commented Mar 11, 2026

Copy link
Copy Markdown

Added a php tree sitter parser so we have support for php.

Sinisa Culic added 2 commits March 11, 2026 06:46
Two bugs caused the shallow index to be rebuilt on every server startup:

1. Hash length mismatch: shallow_index_manager.py and sqlite_index_manager.py
   used a 12-char truncated MD5 hash (hexdigest()[:12]) for the storage
   directory name, while project_settings.py used the full 32-char MD5 hash.
   This meant the index was written to e.g. /tmp/code_indexer/32697df368a6/
   but looked for at /tmp/code_indexer/32697df368a617d19aac2f121d60624f/,
   so load_index() always failed and the index was always rebuilt from scratch.

   Fix: remove [:12] truncation from both files so all components use the
   same full 32-char MD5 hash.

2. --indexer-path flag ignored by shallow index: shallow_index_manager.py
   hardcoded tempfile.gettempdir() and ignored ProjectSettings.custom_index_root,
   so the --indexer-path CLI argument had no effect on the shallow index storage
   location (only the deep SQLite index respected it).

   Fix: check ProjectSettings.custom_index_root first, falling back to
   tempfile.gettempdir() when not set.

Together these fixes ensure the shallow index is written to and loaded from
the correct persistent location, eliminating unnecessary rebuilds on startup.
Implement PhpParsingStrategy using tree-sitter-php to parse PHP files (.php, .phtml, .php3, .php4, .php5, .phps). Extracts namespaces, classes, interfaces, traits, enums, functions, methods, use declarations, and tracks method/function call relationships. Register strategy in factory and add tree-sitter-php>=0.22.0 dependency.

@johnhuang316 johnhuang316 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the PR - adding PHP support with a dedicated tree-sitter strategy is definitely the right direction.

I’m going to request changes before merging though.

Main concerns:

  • there are no tests for the new PHP strategy yet
  • call relationship tracking seems to miss forward references
  • member call resolution looks a bit too broad and may resolve to the wrong method if multiple classes use the same method name
  • this PR also mixes the shallow index path/hash fix with the PHP parser work, which makes it harder to review

Could you update this by:

  • adding PHP strategy tests
  • handling deferred/pending calls for forward references
  • tightening member call resolution
  • ideally splitting the index-path fix from the PHP feature

Happy to take another look after that.

@johnhuang316 johnhuang316 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Code Review Summary

Thanks for adding PHP support — this is a useful direction for the project. However, I found several issues that should be addressed before this can be merged.

Blocking

  1. Circular import introduced by ShallowIndexManager

    src/code_index_mcp/indexing/shallow_index_manager.py now imports ProjectSettings at module import time:

    from ..project_settings import ProjectSettings

    This creates a circular import through:

    project_settings.py
     -> search.ag
     -> search.base
     -> indexing.__init__
     -> shallow_index_manager
     -> project_settings
    

    Running the test suite currently fails during import:

    ImportError: cannot import name 'ProjectSettings' from partially initialized module 'code_index_mcp.project_settings'
    

    Please avoid importing ProjectSettings at module scope here. A lazy import inside the method may be enough as a minimal fix, but the cleaner approach would be to move shared index-root resolution into a small helper that does not import the indexing package back through project_settings.

  2. uv.lock is not updated

    pyproject.toml adds:

    tree-sitter-php>=0.22.0

    but uv.lock is not included in the PR. Please run:

    uv lock

    and commit the updated lockfile.

  3. PHP call relationship tracking is incomplete

    The PR description says this tracks function/method call relationships, but the current implementation only resolves calls to symbols that have already been encountered earlier in the same traversal.

    For common PHP code like this:

    <?php
    class A {
        function b() {
            helper();
            $this->validate();
        }
    
        function validate() {}
    }
    
    function helper() {}

    both helper() and $this->validate() are missed because those symbols are registered later in the file.

    Please consider a two-pass approach:

    • first pass: register all classes/functions/methods
    • second pass: analyze calls and fill called_by

    Alternatively, unresolved calls should be stored as pending calls, similar to the existing strategies that handle forward references.

Warnings / Suggestions

  • self::method() / static::method() are parsed as scoped_call_expression, but the strategy currently only handles function_call_expression and member_call_expression.
  • Grouped imports such as use Foo\{Baz, Qux as Alias}; are not extracted correctly because namespace_use_group is not traversed for individual clauses.
  • Please add focused tests for the new PHP strategy, including:
    • extension routing to PhpParsingStrategy
    • namespace and use extraction
    • class / interface / trait / enum extraction
    • function and method extraction
    • $this->method() relationships
    • forward-reference function/method calls
    • self::method() / scoped calls
    • grouped use declarations

Additional note

GitHub currently reports this PR as conflicting with master, so it will also need to be rebased or updated after the above issues are fixed.

Thanks again for the contribution — once the import failure, lockfile, and PHP call-resolution behavior are addressed, this should be much safer to review again.

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