Handle anonymous class attribute

[romm]

Hi, first of all thanks for your work, I didn't know about this library and it could be of great use for a test suite in a project of mine.

I ran into the following issue: when an anonymous class uses a PHP 8 attribute, Patchwork crashes.

I'm adding a failing test to this PR, I currently have no idea where to look to fix this, I'd be glad to help if someone gives me a hint. 🙂

Have a good day!

[anomiex]

I currently have no idea where to look to fix this, I'd be glad to help if someone gives me a hint. 🙂

Looks like the code in injectCodeAfterClassDefinitions skips over whitespace and comments (see Source::junk()), but to handle the new class case it also needs to skip over attributes.

patchwork/src/CodeManipulation/Actions/Generic.php

Lines 140 to 143 in 27e8ab9

	if ($s->is([T_DOUBLE_COLON, T_NEW], $s->skipBack(Source::junk(), $match))) {
	# Not a proper class definition: either ::class syntax or anonymous class
	continue;
	}

Also, I find that php -r 'require "./Patchwork.php"; echo Patchwork\CodeManipulation\transformString( file_get_contents( "tests/includes/AnonymousClassAttribute.php" ) );' can show what Patchwork is turning the code in your test into, which may help in working on it.

[jrfnl]

Re: the tests - probably a good idea to add some more variations, like multi-line attributes with array parameters + a class multiple attributes attached, as those cases will need to be handled correctly and are, based on experience elsewhere, the ones which will break any "simple" solution.

Another thing which should probably be taken into account (but solved separately) is PHP 8.3 readonly anonymous classes, where the readonly keyword between new and class could possibly trip Patchwork up ?

[romm]

@anomiex @jrfnl thanks for your feedback, I've just pushed some changes and will be waiting for further comments or approval. 🙂

[anomiex]

This incorrectly excludes injecting code after

patchwork/tests/includes/AnonymousClassAttribute.php

Lines 3 to 6 in 28c1464

	#[\Attribute]
	class SomeAttribute {
	public function __construct($value) {}
	}

A test for that might have to actually call Patchwork\CodeManipulation\transformString() and examine the output rather than just seeing that there's no fatal error raised.

[romm]

Well observed. To be honest I have no clue how to fix the issue, how to detect that a class with attribute is anonymous or not. Do you have any idea on how to do it?

[antecedent]

I think that all named classes have a T_STRING token (the name) following the class token. This, however, only comes from my intuition for the time being, and could be wrong. But if it is correct, then we could get around this issue by inspecting the token on the right side of the class token, not the left side.

[jrfnl]

I think that all named classes have a T_STRING token (the name) following the class token. This, however, only comes from my intuition for the time being, and could be wrong. But if it is correct, then we could get around this issue by inspecting the token on the right side of the class token, not the left side.

Depends on how the tokenizer is run. Looks like it is run with the TOKEN_PARSE $flag, in which case probably yes, though results from running the tokenizer with that flag vary depending on the PHP version (then again, that is the case for token streams anyway).

In PHPCS, to determine whether the class keyword is an anonymous class or a named class, we check if the next non-empty token is one of the following, if it is, the class keyword is marked as an anonymous class:

• ( (open parenthesis)
• { (open curly)
• T_EXTENDS
• T_IMPLEMENTS

Hope that helps.

[anomiex]

Personally what I was thinking of doing was to $pos = $s->skipBack(Source::junk(), $match), then if it's pointing at a RIGHT_SQUARE and $s->match() for that points to a General\ATTRIBUTE, skipBack from that General\ATTRIBUTE and repeat as necessary for more RIGHT_SQUAREs. Once it finds a non-bracket (or a RIGHT_SQUARE not matching with a General\ATTRIBUTE), then look for the T_DOUBLE_COLON or T_NEW.

Although I like @jrfnl 's idea too.

[antecedent]

I took @jrfnl 's suggestion. Both the last one, and the previous one about readonly. T_READONLY really seems to be the only token that could intervene between new and class, beside attributes:
https://github.com/nikic/PHP-Parser/blob/master/grammar/php.y#L501-L515

[antecedent]

@romm, could you run this PR's branch with your intended code base, as an end-to-end test of sorts, and report here? Just to make sure that no other bugs show up.

[romm]

Hi @antecedent, just tried the patch on my code base and it works like a charm. I detected another issue regarding private constructors, but it seems to be a completely different problem, I'll investigate on it.

Thanks!

EDIT: that private constructor thing was an issue on my end, so no worries!