Skip to content

Conversation

mttsch
Copy link
Contributor

@mttsch mttsch commented Aug 11, 2025

No description provided.

@mttsch
Copy link
Contributor Author

mttsch commented Aug 11, 2025

After wanting to continue with the contains deprecations from https://github.com/sebastianbergmann/phpunit/blob/11.5/DEPRECATIONS.md#soft-deprecations, I am wondering whether to also consider them in this rector to have rector for "generic method with type parameter to specialized type-specific method" that covers isType, assertContainsOnly, assertNotContainsOnly and containsOnly (this rector would be renamed, of course). Otherwise I would add a separate AssertContainsMethodCallRector.

@samsonasik Do you have a preference?

$this->assertThat(1, $this->isType('int'));
$this->assertThat(1, $this->isType('integer'));
$this->assertThat([], $this->isType('iterable'));
$this->assertThat(null, $this->isType('null'));
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can have another rule that would simply use exact assert method:

-$this->assertThat($value, $this->isType('null'));
+$this->assertNull($value);

E.g. in phpunit code quality set. Much more readable and direct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get your point but for this specific rector rule fixture, it makes sense to keep it like this to keep the fixture code somewhat "useful" instead of just calling the relevant methods and doing nothing with the return type.

Also see PHPUnit's test cases for these methods which are the exact same https://github.com/sebastianbergmann/phpunit/blob/a561bbf7dd850c1bef4f8db7230ffa68ca6797af/tests/unit/Framework/Assert/assertThatTest.php#L423-L426

But I guess you are talking more about a different rule from this one.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is job for next Rector rule :)

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.

3 participants