Remove prophecy usage #6421

jordisala1991 · 2020-09-27T08:34:55Z

Subject

I am targeting this branch, because this is a changes on tests plus a patch on src

Changelog

### Fixed
- Call to deprecated `getTargetEntity`

This will allow us to have the php 8 build running without errors. Also it will allow to remove prophecy usage and to upgrade all builds to use phpunit 9 (Except php 7.2)
This is the first step, there are other changes needed on dev-kit and on other repositories

jordisala1991 · 2020-09-27T08:36:01Z

src/Form/DataTransformerResolver.php

-        return (
-            method_exists($fieldDescription, 'getTargetModel') && $class === $fieldDescription->getTargetModel()
-        ) || $class === $fieldDescription->getTargetEntity();
+        return method_exists($fieldDescription, 'getTargetModel') ?


I found this while changing all the tests, this can be done on a separate PR if you want.

This PR could take some times before being merged. So maybe it's indeed a good idea to create a separate PR

Let's see how this goes, if we see a lot of review process I will revert it from this PR and open a new one. If not, I will change it to patch and add a changelog

Yes, but at least a separate commit seems needed.

jordisala1991 · 2020-09-27T08:47:30Z

I spent a lot of time trying to fix the BreadcrumbsBuilderTest, this one testUnitChildGetBreadCrumbs I dont have idea on how to fix it.

There was 1 failure:

1) Sonata\AdminBundle\Tests\Admin\BreadcrumbsBuilderTest::testUnitChildGetBreadCrumbs
Failed asserting that actual size 0 matches expected size 5.

Error: /home/runner/work/SonataAdminBundle/SonataAdminBundle/tests/Admin/BreadcrumbsBuilderTest.php:403

@greg0ire maybe you can help me? I need to see what I broke with the change from prophecy to phpunit mocks.

jordisala1991 · 2020-09-27T08:48:59Z

I leave it as a draft, but the only thing missing before reviews is fixing that one test. The rest should be okay.

tests/Action/DashboardActionTest.php

greg0ire · 2020-09-27T09:36:19Z

@greg0ire maybe you can help me? I need to see what I broke with the change from prophecy to phpunit mocks.

Not sure what you broke, but when I wrote this test, I wasn't yet aware that mocking what you don't own is bad it seems. I pushed a new commit that greatly simplifies this test by relying on real classes instead of stubs.

tests/Bridge/Exporter/AdminExporterTest.php

greg0ire · 2020-09-27T09:50:19Z

My commit removed many expectations though, I will force push equivalent assertions soon.

jordisala1991 · 2020-09-27T09:54:45Z

My commit removed many expectations though, I will force push equivalent assertions soon.

I pushed two more commits to fix builds and the var thing

greg0ire · 2020-09-27T09:59:16Z

I just force pushed (with lease)

jordisala1991 · 2020-09-27T10:00:58Z

Aaaand the php8 is green again! Thank you @greg0ire .

Please review, @sonata-project/contributors

VincentLanglet

Maybe some clean up is necessary in the commits ?

For instance the fix of src/Form/DataTransformerResolver.php should be in a separate commit.

This will allow us to use `assertMatchesRegularExpression` on the lowest dependencies build

The previous check will call it anyway since the second part of the comparison can return false, and that would lead to calling deprecated code.

jordisala1991 · 2020-09-27T10:09:28Z

I squashed some commits, separate the code changed on src. It should be better now.

franmomu

I would replace:

- $this->container = $this->createStub(ContainerInterface::class);
+ $this->container = new Container();

But we could do that in another PR.

Edit: Another PR is fine 🤣

tests/Action/AppendFormFieldElementActionTest.php

tests/Action/RetrieveAutocompleteItemsActionTest.php

tests/Action/RetrieveFormFieldElementActionTest.php

tests/Admin/AdminTest.php

tests/Controller/HelperControllerTest.php

tests/Filter/Persister/SessionFilterPersisterTest.php

tests/Twig/Extension/SonataAdminExtensionTest.php

tests/Util/ObjectAclManipulatorTest.php

greg0ire · 2020-09-27T10:51:05Z

Oh sorry @franmomu I merged right when you published your review 😅

franmomu · 2020-09-27T10:52:52Z

No worries, that can be change with the Container mock in another one.

VincentLanglet · 2020-09-27T11:09:54Z

@jordisala1991 Big thanks for this PR.
Do you want to also make a merge into master ? There should be some conflict.

jordisala1991 · 2020-09-27T11:10:54Z

Yes, I will do it. I expect big conflict fight

jordisala1991 · 2020-09-27T11:12:06Z

No worries, that can be change with the Container mock in another one.

Can you make another PR to fix those things? I wont have time today to do it

Edit: wait, I have time to fix them now

jordisala1991 commented Sep 27, 2020

View reviewed changes

jordisala1991 added the pedantic label Sep 27, 2020

jordisala1991 changed the title ~~[WIP] Remove prophecy usage~~ Remove prophecy usage Sep 27, 2020

jordisala1991 requested a review from VincentLanglet September 27, 2020 08:49

greg0ire reviewed Sep 27, 2020

View reviewed changes

tests/Action/DashboardActionTest.php Outdated Show resolved Hide resolved

greg0ire reviewed Sep 27, 2020

View reviewed changes

tests/Bridge/Exporter/AdminExporterTest.php Show resolved Hide resolved

greg0ire force-pushed the pedantic/remove-prophecy branch from 1dada05 to c1cd54b Compare September 27, 2020 09:59

jordisala1991 marked this pull request as ready for review September 27, 2020 10:00

jordisala1991 requested a review from a team September 27, 2020 10:01

VincentLanglet previously approved these changes Sep 27, 2020

View reviewed changes

VincentLanglet requested a review from a team September 27, 2020 10:03

jordisala1991 added 4 commits September 27, 2020 12:05

Remove prophecy usage

e3f7f79

Fix tests that trigger a risky test

8ed4d6d

Bump phpunit-bridge to 5.1.4 as minimum

88f5fee

This will allow us to use `assertMatchesRegularExpression` on the lowest dependencies build

Ensure deprecated getTargetEntity is not called

7e578d1

The previous check will call it anyway since the second part of the comparison can return false, and that would lead to calling deprecated code.

jordisala1991 dismissed VincentLanglet’s stale review via 7e578d1 September 27, 2020 10:07

jordisala1991 force-pushed the pedantic/remove-prophecy branch from c1cd54b to 7e578d1 Compare September 27, 2020 10:07

jordisala1991 requested review from VincentLanglet and greg0ire September 27, 2020 10:08

VincentLanglet approved these changes Sep 27, 2020

View reviewed changes

jordisala1991 added patch and removed pedantic labels Sep 27, 2020

franmomu reviewed Sep 27, 2020

View reviewed changes

greg0ire approved these changes Sep 27, 2020

View reviewed changes

greg0ire merged commit 8380d06 into sonata-project:3.x Sep 27, 2020

franmomu mentioned this pull request Sep 27, 2020

Remove some Prophecy usage #6416

Closed

jordisala1991 deleted the pedantic/remove-prophecy branch September 27, 2020 11:17

jordisala1991 mentioned this pull request Sep 27, 2020

Improve mock usage #6422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove prophecy usage #6421

Remove prophecy usage #6421

jordisala1991 commented Sep 27, 2020 •

edited

Loading

jordisala1991 Sep 27, 2020

VincentLanglet Sep 27, 2020

jordisala1991 Sep 27, 2020

VincentLanglet Sep 27, 2020

jordisala1991 commented Sep 27, 2020

jordisala1991 commented Sep 27, 2020

greg0ire commented Sep 27, 2020

greg0ire commented Sep 27, 2020

jordisala1991 commented Sep 27, 2020

greg0ire commented Sep 27, 2020

jordisala1991 commented Sep 27, 2020

VincentLanglet left a comment

jordisala1991 commented Sep 27, 2020

franmomu left a comment •

edited

Loading

greg0ire commented Sep 27, 2020

franmomu commented Sep 27, 2020 •

edited

Loading

VincentLanglet commented Sep 27, 2020 •

edited

Loading

jordisala1991 commented Sep 27, 2020

jordisala1991 commented Sep 27, 2020 •

edited

Loading

Remove prophecy usage #6421

Remove prophecy usage #6421

Conversation

jordisala1991 commented Sep 27, 2020 • edited Loading

Subject

Changelog

jordisala1991 Sep 27, 2020

Choose a reason for hiding this comment

VincentLanglet Sep 27, 2020

Choose a reason for hiding this comment

jordisala1991 Sep 27, 2020

Choose a reason for hiding this comment

VincentLanglet Sep 27, 2020

Choose a reason for hiding this comment

jordisala1991 commented Sep 27, 2020

jordisala1991 commented Sep 27, 2020

greg0ire commented Sep 27, 2020

greg0ire commented Sep 27, 2020

jordisala1991 commented Sep 27, 2020

greg0ire commented Sep 27, 2020

jordisala1991 commented Sep 27, 2020

VincentLanglet left a comment

Choose a reason for hiding this comment

jordisala1991 commented Sep 27, 2020

franmomu left a comment • edited Loading

Choose a reason for hiding this comment

greg0ire commented Sep 27, 2020

franmomu commented Sep 27, 2020 • edited Loading

VincentLanglet commented Sep 27, 2020 • edited Loading

jordisala1991 commented Sep 27, 2020

jordisala1991 commented Sep 27, 2020 • edited Loading

jordisala1991 commented Sep 27, 2020 •

edited

Loading

franmomu left a comment •

edited

Loading

franmomu commented Sep 27, 2020 •

edited

Loading

VincentLanglet commented Sep 27, 2020 •

edited

Loading

jordisala1991 commented Sep 27, 2020 •

edited

Loading