Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PropertyExistsWithoutAssertRector #158

Closed
wants to merge 1 commit into from

Conversation

jrjohnson
Copy link
Contributor

Removes the now deprecated property/method exists PHPUnit assertions and replaces them with plain PHP equivalents. This is the inverse of AssertPropertyExistsRector.php to account for their removal in PHPUnit v10.

Removes the now deprecated property/method exists PHPUnit assertions and
replaces them with plain PHP equivalents.
@TomasVotruba
Copy link
Member

I'll look into this, thank you 👍

@TomasVotruba
Copy link
Member

Rebased and merged in #202

@jrjohnson jrjohnson deleted the property-exists branch June 26, 2023 05:59
@eerison
Copy link
Contributor

eerison commented Sep 18, 2023

Hey @TomasVotruba

I guess this rule should be introduced in phpunit 9 rules instead of 10, don't you?

https://github.com/rectorphp/rector-phpunit/pull/202/files#diff-1d1786275faa487b46d3179a731083128edc0bb6238271e77c70c5c0ccf1881e

@TomasVotruba
Copy link
Member

Hey, the PHPUnit 10 is fine here as that's where the change happens. See milestone in: sebastianbergmann/phpunit#4601

The idea is that after running PHPUNIT_100 set, the code should run on PHPUnit 10.

@eerison
Copy link
Contributor

eerison commented Sep 18, 2023

But I can not run PHPUNIT_100 using phpunit 9, and it is deprecated in version 9.
IMO it could be in version 9, it is the version where it was deprecated.

my pipelines are failing when I add deprecated methods :(

the same thing for https://github.com/rectorphp/rector-phpunit/blob/main/docs/rector_rules_overview.md#addprophecytraitrector

@eerison
Copy link
Contributor

eerison commented Sep 18, 2023

well I made this work adding

    $rectorConfig->rule(PropertyExistsWithoutAssertRector::class);
    $rectorConfig->rule(AddProphecyTraitRector::class);

But if it's in version 9, I could avoid search about those 2 rules.

@TomasVotruba
Copy link
Member

You can run PHPUNIT_100 set on any PHPUnit version, then upgrade to PHPUnit 10 in your composer.

What exact error are you getting?

@eerison
Copy link
Contributor

eerison commented Sep 18, 2023

I saw that it is applying php 8 code, and I'm still on php 7.4 :'(

                return match ($matcher->numberOfInvocations()) {
                    1 => $expectedSleepValues,
                };

@eerison
Copy link
Contributor

eerison commented Sep 18, 2023

before

    ->withConsecutive(...$expectedSleepValues);

after

            ->willReturnCallback(function () use ($matcher) {
                return match ($matcher->numberOfInvocations()) {
                    1 => $expectedSleepValues,
                };
            });

@eerison
Copy link
Contributor

eerison commented Sep 18, 2023

Ok it's the issue

this rule is applying php 8 code

Should we downgrade this code to php 7 ?

@TomasVotruba
Copy link
Member

TomasVotruba commented Sep 18, 2023

That's the pain point. Perfect! This rule should run only on PHP 8 indeed.

Should we downgrade this code to php 7 ?

That's not necessary. The withConsecutive() still runs on PHPUnit 9, and PHPUnit 10 already requires on PHP 8.1, so it's supported there.

Instead, this rule should implement MinPhpVersionInterface with PHP 8. Could you add it?

@eerison
Copy link
Contributor

eerison commented Sep 18, 2023

That's the pain point. Perfect! This rule should run only on PHP 8 indeed.

Should we downgrade this code to php 7 ?

That's not necessary. The withConsecutive() still runs on PHPUnit 9, and PHPUnit 10 already requires on PHP 8.1, so it's supported there.

Instead, this rule should implement MinPhpVersionInterface with PHP 8. Could you add it?

Yep, I will open a PR ;)

eerison pushed a commit to eerison/rector-phpunit that referenced this pull request Sep 18, 2023
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