Skip to content
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

Support Doctrine ORM inheritance #64

Open
mattattui opened this issue Mar 15, 2021 · 2 comments
Open

Support Doctrine ORM inheritance #64

mattattui opened this issue Mar 15, 2021 · 2 comments
Labels
needs opinion Ask for the community opinion on a topic/enhancement

Comments

@mattattui
Copy link

mattattui commented Mar 15, 2021

I'm excited to try Meilisearch, so I'm happy there's a symfony bundle! Sadly the project I picked to try it out on uses Doctrine's Single Table Inheritance, and this confuses the meili:import command, or more specifically Services\MeiliSearchService::isSearchable().

Under Doctrine's single table inheritance method, we create multiple entity classes that inherit from a root entity in the normal PHP way (class MyEntityType extends BaseEntity…), and then a so-called discriminator column saves which type an entity really is, and Doctrine takes care of hydrating the right entity type when you load things from the parent type's repository. To give an example, I've got an app where I've got a Card entity, and its two subtypes are Article and Link. Both Article and Link have all the (searchable) properties of Card, but Articles have a content blob and Links have a URL to a site. With my previous search solution, I indexed Card so that I could search all cards together, which makes sense in this app -- the differences between the entities are in how they're displayed, not in what we browse and search for.

The problem seems to be that isSearchable() does a string comparison of entity class names against a list of configured classes, rather than using PHP's instanceof operator. This means that although all my entities are descendants of Card, they're actually Link or Article entities so isSearchable returns false and nothing ever gets into my index. If I configure each entity to have its own index, then… well they'd be in separate indices and not searchable as one thing, which is what I'd like to continue doing.

If I were to try to resolve this myself, I'd change the isSearchable method to do something like this

    public function isSearchable($className): bool
    {
        if (is_object($className)) {
            $className = ClassUtils::getClass($className);
        }

        foreach ($this->searchableEntities as $searchableEntity) {
            if ($className instanceof $searchableEntity) {
                return true;
            }
        }

        return false;
    }

However I've not tested this, and I'm not familiar enough with the bundle or its uses to know if this could cause problems for other users, so I'm reluctant to submit a PR. If you think it's an acceptable change, I could try my hand at it though!

@curquiza
Copy link
Member

Hello @inanimatt!
Thanks for your feedback and sorry for the delay!

I'm from the Meili team and I'm responsible for the integration repositories like this one. However, I'm not a Symfony user, so I'm not able to tell you how it could impact the usage of the other users, I mean if your suggestion is what they would like or if it's already what they expect.
This repository was built by an external contributor and he copied the Algolia's search bundle to adapt it to MeiliSearch. So the isSearchable method has currently the same behavior as the one in Algolia's search bundle.

But we are opensource, and it does not mean we cannot change it, that's why I particularly like this kind of issue 🙂
You can understand I cannot change it soon since you are the only one (for the moment) asking for this change, but I let this issue opened so that every user can come and share their opinion about it. We could make a decision together about this behavior 🙂

@curquiza curquiza added help wanted Extra attention is needed question Further information is requested needs opinion Ask for the community opinion on a topic/enhancement and removed help wanted Extra attention is needed question Further information is requested labels Mar 20, 2021
@revenkroz
Copy link
Contributor

I was faced with the same question. Now it can be solved quite easily (based on your example):

meili_search:
    url: '%env(MEILISEARCH_URL)%'
    api_key: '%env(MEILISEARCH_API_KEY)%'
    indices:
        - name: cards
          class: App\Entity\Article # extends Card
        - name: cards
          class: App\Entity\Link # extends Card

Just don't set base class as a source, set only its children and use the same name of index for both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs opinion Ask for the community opinion on a topic/enhancement
Projects
None yet
Development

No branches or pull requests

4 participants