Doctrine class metadata not found for extended interface about syliusresourcebundle HOT 27 OPEN

I've been thinking about a long-term solution for this issue. The interface passed in the configuration is used only for resolving Doctrine relationships.

Defining multiple interfaces sounds like an easy way to work around the current limitation, but still requires manual changes whenever changing the interface and the knowledge that the change is possible.

Please let me know what you think, thanks for your patience. @teohhanhui @vvasiloi

Proposed solution

I'd go for autodiscovering interfaces based on model class and using the unique ones for resolving target entities. Let's assume the following scenario from Sylius:

interface ResourceInterface {}

interface UserInterface extends ResourceInterface {}

interface ShopUserInterface extends UserInterface {}
interface AdminUserInterface extends UserInterface{}

class ShopUser implements ShopUserInterface {}
class AdminUser implements AdminUserInterface {}

The first step would be to gather all the interfaces implemented by each resource:

• AdminUser: AdminUserInterface, UserInterface, ResourceInterface
• ShopUser: ShopUserInterface, UserInterface, ResourceInterface

We can always filter out ResourceInterface as it's too generic. The interfaces that are implemented by more than one resource are also filtered out. This leaves us with:

• AdminUser: AdminUserInterface, UserInterface, ResourceInterface
• ShopUser: ShopUserInterface, UserInterface, ResourceInterface

Extending entities

Going further, let's follow a case in which someone decides to extend AdminUser:

interface MyAdminUserInterface extends AdminUserInterface {}
class MyAdminUser implements MyAdminUserInterface { use SomeTraitFromPlugin; }

The following configuration will be needed (same as now):

sylius_user:
  resources:
    admin:
      user:
        classes:
          model: MyAdminUser

No need to change interface value (configuration merging for multiple interfaces could be tricky), interfaces are autodiscovered.

• AdminUser: MyAdminUserInterface, AdminUserInterface, UserInterface, ResourceInterface
• ShopUser: ShopUserInterface, UserInterface, ResourceInterface

Backwards compatibility

If interface configuration option is passed, trigger a deprecation notice and add it to the list of autodiscovered interfaces after filtering it.

Possible improvements outside of MVP

If an unresolved interface is used in the mapping and it has been filtered out from the list earlier, throw an exception containing all matching models and their interfaces.

from syliusresourcebundle.

To get rid of the deprecation we have to remove the default value, but not the key as it will result in errors when it's defined in end-projects.

from syliusresourcebundle.

I think going up or down the inheritance chain isn't a good idea.
What if an interface is extended/implemented by multiple interfaces/classes?
Like Sylius\Component\Core\Model\ProductInterface is implemented by App\Entity\Product
and also extended by App\Model\FooProductInterface, which is then implemented by App\Entity\FooProduct.
You will end up having either App\Model\FooProductInterface mapped to App\Entity\Product or Sylius\Component\Core\Model\ProductInterface mapped to App\Entity\FooProduct, depending on the order they're processed.

Maybe it could be done by having a way to explicitely define multiple/additional interfaces for a class.
WDYT?

from syliusresourcebundle.

Yes, that's what I've mentioned. We cannot go up / down the inheritance chain as that's unsafe. So we must provide a way to map multiple interfaces. 😄

from syliusresourcebundle.

Oh, sorry, I read that poorly. 😄
So, maybe something like that:

// Sylius\Bundle\ResourceBundle\DependencyInjection\Configuration::addResourcesSection
->arrayNode('interfaces')
    ->cannotBeEmpty()
    ->scalarPrototype()
    ->beforeNormalization()->castToArray()
 ->end()

and

// Sylius\Bundle\ResourceBundle\DependencyInjection\Compiler\DoctrineTargetEntitiesResolverPass::getInterfacesMapping

private function getInterfacesMapping(array $resources): array
{
    $interfaces = [];
    foreach ($resources as $alias => $configuration) {
        // current code
        if (isset($configuration['classes']['interfaces'])) {
            foreach ($configuration['classes']['interfaces'] as $interface) {
                $interfaces[$interface] = sprintf('%s.model.%s.class', $alias[0], $alias[1]);
            }
        }
    }

    return $interfaces;
}

Looks too easy to be true though.

from syliusresourcebundle.

Transferred to the ResourceBundle repository as the development would involve it now.

from syliusresourcebundle.

@pamil sounds good to me.
This way any conflict could be solved by creating and implementing a unique interface.

from syliusresourcebundle.

Please take a look at #97 for the implementation.

from syliusresourcebundle.

autodiscovering interfaces based on model class and using the unique ones for resolving target entities

I think it's dangerous as it might inadvertently map some other interfaces that are not desired, e.g. TimestampableInterface, especially considering the use of the ResourceBundle standalone.

More importantly, it means the user loses control completely. If the resource class happens to implement some interfaces, they'll get mapped, and there's (currently) no way to change that. A blacklist approach is not practical either. I still favour switching to a whitelist approach. Let the user map the interfaces explicitly, especially considering this interface mapping should not only be for resolving target entity in Doctrine ORM.

from syliusresourcebundle.

@teohhanhui right now the interface config value is only used for Doctrine's target entity resolving.
Interfaces like TimestampableInterface are unlikely to be used as target entities and that's something the developer is in control of.
Let's say there's a Foo entity which is a resource and is implementing TimestampableInterface and ResourceInterface respectively.
The later is ignored so the only choice is to use TimestampableInterface as target entity which doesn't make sense and is extremely unlikely to happen.
Even if it happens, it has to be implemented by only one resource entity and will work perfectly fine unless another one will implement it and the developer will have to create an intermediate FooInterface which had to be done intitially.
I might be missing something though, so maybe you can provide an example scenario when this will cause issues or will be a bad DX.
I think the developer has enough control over the process by being able to choose what interfaces to implement and use as target entities.

from syliusresourcebundle.

Consider the very simple case of:

class A implements B, C
{
}

The user does not have any control on which interfaces will be mapped. Maybe the user wants to map B, or C, or both, or neither. We cannot tell and we must not assume.

Sure, the problem might not manifest itself in the Sylius project / distribution, but this bundle is supposed to be a general purpose resource bundle. Also, true, the interface mapping is currently not used for anything else in Sylius, but that's not an assumption we should be making going forward (and the same consideration for the standalone use of this bundle). So, to me this is a design smell.

from syliusresourcebundle.

The user is in control of what interfaces to implement and by being aware of how the autodiscovery works it's easy to make sure the right interface is used.
In case of class A implements B, C, if both interfaces are only implemented by class A, then interface B will be used for mapping. If the user used interface C in mappings, then the code will break.
It might be a good idea to throw an exception in that case.
This can be easely mitigated by class A implements D where interface D extends B, C and D is a dedicated interface which is not implemented by another resource entity.

from syliusresourcebundle.

@vvasiloi I disagree. The user should be free to implement whichever interfaces they want. The resource bundle should not force them to code in a certain way. Also, you don't have control over third party classes, so that wouldn't work in practice.

from syliusresourcebundle.

Well, every piece of reused code forces some rules, that's a contract the user has to sign.
If classes from a 3rd party library are not designed to work within the Resource Bundle, then the user will have to take care of their integration anyway, at least to implement the ResourceInterface.
Also, the user can always define explicit mapping:

doctrine:
    orm:
        resolve_target_entities:

from syliusresourcebundle.

I think I've already made my case above. We're talking past each other so I'll stop.

from syliusresourcebundle.

I don't see any issues with your example.
Let's get back to class A implements B, C.
Either B or C , or even both, can be used to map to A.
The exclusivity condition is obvious and is forced by the fact that this is a mapping, so you can't have B or C resolve to multiple targets.
Whichever is used in mapping will be added to ResolveTargetEntityListener, the other one will only be added if it's also exclusively implemented by A.
Where's the issue?

from syliusresourcebundle.

@teohhanhui @vvasiloi what if we would drop using interfaces and go for the resource identifier (app.book, sylius.product) instead? It is unique and does not need to be changed if the model or its interfaces change.

from syliusresourcebundle.

what if we would drop using interfaces and go for the resource identifier (app.book, sylius.product) instead?

I'm not sure I understand. Do you mean using that as the entity alias? ResolveTargetEntityResolver works with class names. And it wouldn't work for the WIP migration of ShopApiPlugin to API Platform, at least (see for example https://github.com/dunglas/ShopApiPlugin/blob/api-platform/src/Resources/config/api_resources/product/ProductInterface.xml).

from syliusresourcebundle.

Actually, what's the problem with having to change the interface(s) mapping when the implementation changes? I think it makes perfect sense.

from syliusresourcebundle.

If the error is Cannot autowire service "RelaisChateaux\Gifts\Doctrine\ORM\RetailStoresRepository": argument "$class" of method "Doctrine\ORM\EntityRepository::__construct()" references class "Doctrine\ORM\Mapping\ClassMetadata" but no such service exists.
this -> https://stackoverflow.com/questions/48024235/cannot-autowire-service-argument-references-class-but-no-such-service-exists seems like the solution, no ?

from syliusresourcebundle.

@vasilvestre I don't know what's the relevance of your comment to this issue. It seems completely unrelated.

from syliusresourcebundle.

so why is this deprecation triggered? at this moment sylius throws this deprecation for over 4 years without a way to silence it properly.

from syliusresourcebundle.

@vvasiloi do you think you could bring to this issue a new life ?

from syliusresourcebundle.

@loic425 you have to be more specific. I have to re-read everything to see what's left to be done, but generallyI'm quite happy with the autodiscovery made by Kamil and never had any issues with it.

from syliusresourcebundle.

So maybe just have a look on the triggered deprecation

from syliusresourcebundle.

The deprecation comes from Sylius codebase, as it still uses the interface key in resource configuration. Those should be removed. I think it's mostly in the bundles' Configuration classes. Here's an example: https://github.com/Sylius/Sylius/blob/c4bacdfb4e492af4360deaeace466ad694d51380/src/Sylius/Bundle/AddressingBundle/DependencyInjection/Configuration.php#L77

from syliusresourcebundle.

Hum ok, I never use that interface Key. That's easy to remove for 1.13. thanks @vvasiloi

from syliusresourcebundle.