Skip to content

Conversation

@dantleech
Copy link
Collaborator

@dantleech dantleech commented Oct 26, 2025

Following conversations with @SerheyDolgushev we decided that it doesn't make sense to maintain independent forks of the Tolerant PHP Parser. I had updated my fork support most of the PHP features that people complained about as missing - all of those seem to be covered in this fork with the exception of property hook default values.

Minimal changes were required to get this fork to run in Phpactor.

  • Making PropertyHook implement the FunctionLike interface (e.g. it inherits the class scope)
  • Not corrupting the AST if a default value is used in a property hook (note this falls short of actually supporting default values)
  • Tolerating unterminated strings (e.g. $foo->bar(') - which is important f.e. in autocompletion.

On Phactor's side all of the work was related to updating the code to use PropertyElement on the PropertyDeclaration: phpactor/phpactor#2946


In addition my fork:

  • Minimal supported version of PHP is 8.1 - looks like Phan also has 8.1 as a minimum, so might be worth bumping.
  • Some quality-of-life improvements for the test suite (re-generating the snapshots if the file doesn't exist, simplifying the process and removing the CallbackListener because I honestly have no idea how that worked).

So:

  • Shall we bump the lib. to 8.1?
  • Shall I additionall create a PR to make the above testing improvements?

the tests are failing and I think the snapshots need to be updated - and perhaps I need to add test cases for the new changes (if I added them originally I didn't include them here).

return $this->parseNumericLiteralExpression($parentNode);

case TokenKind::StringLiteralToken:
case TokenKind::EncapsedAndWhitespace:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will parse StringLiteral if the offset is on an unterminated string quote (otherwise it resolves SourceFileNode)

@rlerdorf
Copy link
Member

I am ok bumping the minimum version to 8.1. Go ahead and do that. I had been meaning to clean up the test suite stuff, but didn't get around to it.

@rlerdorf
Copy link
Member

I found and fixed the test failures! The issue was with the property hook check added in parsePostfixExpressionRest at line 3194-3196. This check was too aggressive and was preventing the parser from handling curly brace array/string access (the deprecated $str{0} syntax).

The fix is simple - just remove those 4 lines since property hooks are already properly handled in parsePropertyElement at line 3672.

See my commit here: af0df06

With this fix, the test suite goes from 26 failures down to 0 (well, technically 1 failure but it passes when I run ParserGrammarTest directly - might be a flaky test).

Feel free to incorporate this change into your PR!

@dantleech
Copy link
Collaborator Author

dantleech commented Oct 29, 2025

@rlerdorf thanks - that commit actually fixes this:

<?php

class Example
{
    private bool $modified = false;

    public string $foo = 'bar' {
        get => $this->foo . ($this->modified ? ' (modified)' : '');
        set(string $value) {
            wrAssertType('Example', $this);
            wrAssertType('string', $value);
            wrAssertType('<missing>', $modified);

        }
    }
}

(the open brace after the default value of the property hook). Without that commit the AST is corrupted.

I guess we should add a test case for that here and ensure that both work.

@rlerdorf
Copy link
Member

@dantleech How about this then?
338e844
Also adds a test for this case and regenerates the snapshots

@dantleech
Copy link
Collaborator Author

dantleech commented Oct 29, 2025

@rlerdorf hm, seems we get different failures with that (cherry-picked into this PR) but the original code was still there, I removed it and ...

looks good, Phactor tests are green.

@rlerdorf rlerdorf merged commit 4531726 into phan:main Oct 29, 2025
5 checks passed
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