Skip to content

Make the class parameter mandatory for references #464

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

Merged
merged 2 commits into from
Mar 8, 2024
Merged

Make the class parameter mandatory for references #464

merged 2 commits into from
Mar 8, 2024

Conversation

VincentLanglet
Copy link
Contributor

No description provided.

@VincentLanglet VincentLanglet marked this pull request as ready for review December 19, 2023 10:02
@@ -96,7 +98,7 @@ public function testReferenceReconstruction(): void

$roleFixture->load($em);
// first test against managed state
$ref = $referenceRepository->getReference('admin-role');
$ref = $referenceRepository->getReference('admin-role', Role::class);
Copy link
Member

Choose a reason for hiding this comment

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

Should we have made these adjustments to the tests on 1.x already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and no, depends on the test strategy.
The "old way" to call getReference must be tested on 1.x too.

On 1.x test should have been duplicated with

  • The old way, working but triggering deprecation, and a @group legacy
  • The new way working without deprecation.

Seems like no test were added/duplicated in #409, so only the old way were tested. Now on 2.x tests are updated.

Copy link
Member

Choose a reason for hiding this comment

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

So, we're missing coverage for the non-deprecated way in 1.x currently? We should fix that first, I suppose.

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 did #465

@derrabus
Copy link
Member

Please remember to update the UPGRADE.md file.

@VincentLanglet
Copy link
Contributor Author

Friendly ping @derrabus @greg0ire ; can it be merged ?

@derrabus derrabus changed the title Solve some 2.0 deprecations. Solve some 2.0 deprecations Mar 8, 2024
@derrabus derrabus changed the title Solve some 2.0 deprecations Make the class parameter mandatory for references Mar 8, 2024
@derrabus derrabus added this to the 2.0.0 milestone Mar 8, 2024
@derrabus derrabus merged commit ecb0e82 into doctrine:2.0.x Mar 8, 2024
@ThomasLandauer
Copy link

This change is not documented at https://symfony.com/bundles/DoctrineFixturesBundle/current/index.html

@VincentLanglet
Copy link
Contributor Author

This change is not documented at symfony.com/bundles/DoctrineFixturesBundle/current/index.html

DoctrineFixtureBundle only support data-fixture v2 since few hours.
PR need to be done to update https://github.com/doctrine/DoctrineFixturesBundle/blob/3.7.x/docs/index.rst

@derrabus
Copy link
Member

Please open an issue (or a PR) on the bundle's repository.

javiereguiluz added a commit to symfony/demo that referenced this pull request Dec 10, 2024
This PR was squashed before being merged into the main branch.

Discussion
----------

Update the data fixtures

The class parameter is now mandatory for references.

Fixes exception when running `bin/console doctrine:fixtures:load`:
```
[ArgumentCountError]
  Too few arguments to function Doctrine\Common\DataFixtures\AbstractFixture::getReference(), 1 passed in /workspace/project/src/DataFi
  xtures/AppFixtures.php on line 143 and exactly 2 expected
```

Related to: doctrine/data-fixtures#464

Commits
-------

0ad830b Update the data fixtures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants