Skip to content

Conversation

kshtompel
Copy link

  • Allow fixtures loader custom implementation

@greg0ire
Copy link
Member

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

@kshtompel
Copy link
Author

@greg0ire please suggest me what to do here, because I see no problem in code.
Interface is declared and imported.
image

Copy link
Member

@greg0ire greg0ire Apr 22, 2025

Choose a reason for hiding this comment

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

Oh I don't know... Have you tried putting this in a file that ends with .php? Might work slightly better...

Copy link
Author

Choose a reason for hiding this comment

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

Oh, thank you. didn't get it

@greg0ire
Copy link
Member

Let me insist on this:

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

@kshtompel
Copy link
Author

@greg0ire thanks for the guidance. All checks are passed

@kshtompel kshtompel requested a review from greg0ire April 24, 2025 08:40
<argument type="service" id="service_container" />
</service>
<service id="doctrine.fixtures.provider" alias="doctrine.fixtures.loader" />
<service id="doctrine.fixtures.loader" class="Doctrine\Bundle\FixturesBundle\Loader\SymfonyFixturesLoader" public="false" />
Copy link
Member

@greg0ire greg0ire Apr 24, 2025

Choose a reason for hiding this comment

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

(why) is this change necessary? Or maybe is it just a best practice?

Copy link
Author

@kshtompel kshtompel Apr 24, 2025

Choose a reason for hiding this comment

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

As LoadDataFixturesDoctrineCommand class right now receives FixturesProvider interface not SymfonyFixturesLoader we should define it as separate service to allow it to be overridden and as it the same service as SymfonyFixturesLoader I just put alias on it for simplicity reason only

Copy link
Member

Choose a reason for hiding this comment

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

OK, but isn't it possible for you to override doctrine.fixtures.loader just by defining a service with that id in your application? I think I remember doing something like that in the past.

Copy link
Author

Choose a reason for hiding this comment

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

it can be overridden only if it was defined otherwise it should be a required part of bundle configuration to be set up

Copy link
Member

Choose a reason for hiding this comment

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

According to what you just wrote, since doctrine.fixtures.loader is defined, it can be overridden, so this is useless, but I feel this isn't what you mean to say.

Copy link
Author

@kshtompel kshtompel Apr 25, 2025

Choose a reason for hiding this comment

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

SymfonyFixturesLoader class is wider implementation then FixturesProvider interface and it is used to configure bundle, see FixturesCompilerPass and \Doctrine\Common\DataFixtures\Loader. So If you want to override doctrine.fixtures.loader you need implement additional methods in your implementation which are not defined in new interface. So as I mentioned I defined new interface and new service which is just alias for current fixtures loader implementation for simplicity.

Copy link
Member

Choose a reason for hiding this comment

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

and it is used to configure bundle, see FixturesCompilerPass and \Doctrine\Common\DataFixtures\Loader.

I'm AFK until Monday, and can only access this on my phone. Please provide GitHub permalinks

Copy link
Author

Choose a reason for hiding this comment

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

updated previous comment with links to repos

Copy link
Member

Choose a reason for hiding this comment

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

Because you changed this line, you can remove public="false" because afaik are Symfony services private by default.

<argument type="service" id="service_container" />
</service>
<service id="doctrine.fixtures.provider" alias="doctrine.fixtures.loader" />
<service id="doctrine.fixtures.loader" class="Doctrine\Bundle\FixturesBundle\Loader\SymfonyFixturesLoader" public="false" />
Copy link
Member

Choose a reason for hiding this comment

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

Because you changed this line, you can remove public="false" because afaik are Symfony services private by default.

@greg0ire greg0ire changed the base branch from 4.1.x to 4.2.x April 27, 2025 22:45
@greg0ire greg0ire added this to the 4.2.0 milestone Apr 27, 2025
@greg0ire greg0ire merged commit aaa20e3 into doctrine:4.2.x Apr 27, 2025
11 checks passed
@greg0ire
Copy link
Member

Thanks @kshtompel

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