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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow PHP ^8.0, drop PHP 7.3 #96

Merged
merged 1 commit into from Dec 14, 2020
Merged

Allow PHP ^8.0, drop PHP 7.3 #96

merged 1 commit into from Dec 14, 2020

Conversation

fezfez
Copy link
Contributor

@fezfez fezfez commented Dec 9, 2020

Note :

roave/backward-compatibility-check has been drop because it does not support PHP:^8.0 and he his not used in CI (probably only in local)

Thanks for your awesome work 馃殌 馃寛 !

@fezfez
Copy link
Contributor Author

fezfez commented Dec 9, 2020

@Ocramius : 8.0 job seem to fail with locked deps because of infection/infection 0.15.3, there are two solution, drop locked tests or drop php 7.3. i believe your policy is to drop php version that are not in active support so i drop php 7.3.

@fezfez
Copy link
Contributor Author

fezfez commented Dec 9, 2020

@Ocramius : infection does not like phpunit deprecation "at" and i dont find a clean solution, i replaced at by "once" ....

@fezfez
Copy link
Contributor Author

fezfez commented Dec 9, 2020

i got this error with infection.

PHPUnit 9.5.0 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.12 with PCOV 1.0.6
Configuration: /tmp/infection/phpunitConfiguration.initial.infection.xml
Random seed:   1607512606

...............R..............                                    30 / 30 (100%)

Time: 00:00.110, Memory: 14.00 MB

There was 1 risky test:

1) DoctrineBatchUtilsTest\BatchProcessing\SimpleBatchIteratorAggregateTest::testIterationRollsBackOnMissingItems
This test executed code that is not listed as code to be covered or used:
- DoctrineBatchUtils\BatchProcessing\Exception\MissingBatchItemException::fromInvalidReference

OK, but incomplete, skipped, or risky tests!
Tests: 30, Assertions: 57, Risky: 1.

Have you an idea ?

@fezfez fezfez changed the title Allow php8 Allow php ^8.0 drop 7.3 Dec 9, 2020
@Ocramius
Copy link
Owner

Ocramius commented Dec 9, 2020

Hmm, the exception should probably be listed under @uses.

Without running infection, you can reproduce these warnings with vendor/bin/phpunit --coverage-text

@fezfez
Copy link
Contributor Author

fezfez commented Dec 9, 2020

@Ocramius thanks 馃憤

So now tests are green, PR contains :

  • allow PHP ^8.0
  • drop PHP 7.3
  • allow doctrine/orm ^2.8
  • move to phpunit ^9.5
  • move phpunit.xml schema to 9.3
  • remove deprecated "at" in phpunit
  • move to infection ^0.20.2
  • typed properties 馃
  • drop roave/backward-compatibility-check 馃憥

Copy link
Owner

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

@fezfez excellent work, but I did indeed find multiple things that do need adjustment :S

composer.json Outdated Show resolved Hide resolved
composer.json Show resolved Hide resolved
phpunit.xml.dist Outdated Show resolved Hide resolved
Copy link
Owner

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Besides some minor changes needed in tests and file permissions, this looks very close to what is needed to get us to PHP 8 :D

@@ -115,7 +115,7 @@ public function testIterationWithSuccessfulReFetches(): void
['Yadda', ['id' => 123], $freshObjects['foo']],
['Yadda', ['id' => 456], $freshObjects['bar']],
]);
$this->entityManager->expects(self::at(2))->method('clear');
$this->entityManager->expects(self::once())->method('clear');
Copy link
Owner

Choose a reason for hiding this comment

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

I've mentioned this previously: this clear() must be called after the other invocations

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 have fixed this test but i think it is really messy, have you a more clean solution ?

Copy link
Owner

Choose a reason for hiding this comment

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

It's messy, but it is indeed required. The best we can do is isolating the stub creation to a private method.

Further refactoring can be deferred to a later point in time: for now, this (IMO) works well enough.

composer.lock Show resolved Hide resolved
Copy link
Owner

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

@fezfez for the purposes of this patch, I'd say:

  1. clean up the last CS checks
  2. open a new issue to clean up UoW/EntityManager stubs in a separate task that somebody else can take on

@Ocramius Ocramius added this to the 2.2.0 milestone Dec 14, 2020
@Ocramius Ocramius added dependencies Pull requests that update a dependency file enhancement labels Dec 14, 2020
@fezfez
Copy link
Contributor Author

fezfez commented Dec 14, 2020

@Ocramius : done. curious to see a more clean solution.

Copy link
Owner

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Excellent, thanks @fezfez!

@Ocramius Ocramius self-assigned this Dec 14, 2020
@Ocramius Ocramius changed the title Allow php ^8.0 drop 7.3 Allow PHP ^8.0, drop PHP 7.3 Dec 14, 2020
@Ocramius Ocramius merged commit 8affefd into Ocramius:2.2.x Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants