Skip to content

Conversation

chr-hertel
Copy link
Contributor

Rector did the work and I cleaned up. Not sure if you're open for this kind of PRs, but was my sunday meditation anyways :)

@greg0ire
Copy link
Member

greg0ire commented Mar 5, 2023

By all means, we're open to such PRs. Can you take a look at phpcs?

@derrabus
Copy link
Member

derrabus commented Mar 5, 2023

Looks good so far. One thing: Doesn't PHPUnit 10 have an attribute equivalent for @requires or did Rector just not catch those?

Can you take a look at phpcs?

PHPCS fails because a test has been commented out and because the following test does not have a PHPDoc block anymore, it assumes that the commented-put test is a badly formatted PHPDoc block.

Here's my take at fixing the issue on 2.14.x by skipping the test instead of commenting it out: #10560. Alternatively, we could just delete that test.

@derrabus derrabus added this to the 3.0.0 milestone Mar 5, 2023
@chr-hertel
Copy link
Contributor Author

Doesn't PHPUnit 10 have an attribute equivalent for @requires or did Rector just not catch those?

It does, Rector missed it or wasn't able. I will have another look 👍

@derrabus
Copy link
Member

derrabus commented Mar 6, 2023

The PHPCS failure should be gone after a rebase.

@greg0ire
Copy link
Member

greg0ire commented Mar 6, 2023

The PHPCS failure should be gone after a rebase.

You mean one failure out of three I think. @chr-hertel please do check it works before force-pushing.

@@ -25,8 +25,10 @@
use Doctrine\Tests\Models\Company\CompanyManager;
use Doctrine\Tests\Models\Company\CompanyOrganization;
use Doctrine\Tests\Models\Company\CompanyPerson;
use Doctrine\Tests\ORM\Functional\Ticket\MyEntity;
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be reverted

@greg0ire
Copy link
Member

greg0ire commented Mar 6, 2023

This should help: #10563

@derrabus
Copy link
Member

derrabus commented Mar 6, 2023

Ah, I missed the other test. Thanks, @greg0ire.

@derrabus
Copy link
Member

derrabus commented Mar 6, 2023

Browsing through all of the RequiresPhp attributes, I realized that they could actually all be removed because we don't run the test suite on PHP < 8.1 on this branch. I did remove a lot of those @requires annotations previously, but merge-ups keep reintroducing them. 😓

But we can remove those in a follow-up, no need to block a merge because of that.

Great work. 👍🏻

@derrabus derrabus merged commit b904f44 into doctrine:3.0.x Mar 6, 2023
@derrabus derrabus changed the title Converting PHPUnit tags to attributes Convert PHPUnit annotations to attributes Mar 6, 2023
@chr-hertel chr-hertel deleted the phpunit-attributes branch March 6, 2023 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants