ProductFixture overwritten and using a different Factory

[seizan8]

/config/app/services/fixtures.xml

<service
        class="Brille24\SyliusCustomerOptionsPlugin\Fixture\ProductFixture"
        id="sylius.fixture.product"
>
    <argument type="service" id="doctrine.orm.default_entity_manager"/>
    <argument type="service" id="brille24.factory.product"/>
    <tag name="sylius_fixtures.fixture" />
</service>

This causes problems if another bundle would do the same. The ProductFixture would be overwritten twice. For the Factory, I think it would be better if the sylius ProductExampleFactory was decorated. Is there a reason it is not? The Fixture seems a bit trickier to me. I think it would be better to tell the user to overwrite the ProductFixture himself. Ideally there would be a static function or trait he could call for the configureResourceNode of brille24. So he would get later updates automatically. Or I guess, you could also decorate the ProductFixture? Never tried that, though I don't see why that wouldn't work.

Side note: the brille24 ProductFactory also has a useless configureOptions method, it never gets called.

[mamazu]

Thanks for the report. With the factory I totally agree with the fact that we should decorate it. The fixture definition of the product however is extended by extending the class (as it isn't final). One thing that could do is to use a trait for the logic, but I'd rather not do that.

True. That function should be called.

[seizan8]

Maybe I wasn't clear with the Fixture. It's true that the ProductFixture class extends the Sylius Fixture. That was not was I meant. The service id (sylius.fixture.product) is overwritten. If someone would be using this plugin and another plugin which did this as well. One plugin would overwrite the sylius service first and the second would overwrite the first plugins definition afterwards. The sylius service ids should not be overwritten by plugins.

Resources/config/app/services/fixutres.xml

<container xmlns="http://symfony.com/schema/dic/services">
    <services>

        ...

        <service
                class="Brille24\SyliusCustomerOptionsPlugin\Fixture\ProductFixture"
                id="sylius.fixture.product"
        >
            <argument type="service" id="doctrine.orm.default_entity_manager"/>
            <argument type="service" id="brille24.factory.product"/>
            <tag name="sylius_fixtures.fixture" />
        </service>
    </services>
</container>

[mamazu]

I agree, the correct way would be to decorate this service the problem there is however that if someone would want to use another plugin together with ours, then it would need to override the service anyways. Because if plugin A inherits from Sylius and the CustomOptions plugin inherits from Sylius they would both exclude each other no matter in which order they decorate.

[seizan8]

No? That would is happening now with extending as they overwrite each other. If you decorate the Fixture like now, calling the parent/inner method and then adding more, there is no problem. You don't remove anything or overwrite. Decorating doesn't overwrite. If there are multiple decorators for a service, each will be called in sequence. That's why you want to use decorators and not extend. In a way, you extend the base without replacing it with a new class.

https://symfony.com/doc/current/service_container/service_decoration.html#decoration-priority

If all plugins just decorate the service, then all the additional config for the fixture will be added. Example: Sylius Fixture as BaseF, Brille24 Fixture as F1, Some other Plugin Fixture as F2 F2 will receive F1 as inner and F1 will receive the BaseF as inner. The configureResourceNode can be the same as now, as when extending. You call the parent method (or inner when decorating), then add your additional properties. It won't replace or remove anything.

[mamazu]

You are one hundred percent correct, the issue I see here is the fact that we can't extend fixtures with decoration. The concept of decoration relies on the fact that the methods we want to extend or override are public. Sadly the configureResoruceNode function we want to extend is protected. So there is no way we can elegantly decorate the fixtures. The only possibility would be to decorate the getConfigTreeBuilder which I don't see as the right way to do things.

[seizan8]

Oh, I see. Ok, now I get it. Indeed, this is a problem.

I think the only solution is to let the bundle user extend it. Basically add a chapter in the readme on how to extend the ProductFixture and add the necessary properties. Ideally provide a static method or something that the user can call and that adds the properties for him. I would say the user has to extend the product fixture himself. It's the only way I see that prevents future issues with other bundles. And the user is in full control. It requires some work to make the bundle work but should be better overall.

I suggest this should be changed in a new major update as it requires some manual changes from the user.

[mamazu]

That sounds like a solution. I will check with the documentation of Sylius maybe there is a better way to do it.

[seizan8]

Alternatively, the production fixture could be dropped altogether. And a product option added to customer options and group. So when loading the customer options they can be linked to existing products. Instead of adding existing options to products on creation. I think that's more like Sylius intended them to be.

It would be nice to have the Fixtures and Factories updated in a version 3.x

[mamazu]

I don't think we should remove the product fixture as the product customer option value overrides are configured on the product. Which makes a lot of sense but yes the factory situation is not the best (aka. mashing the example factories of Sylius and the "normal" object factories together).