Make ReindexProvider getIndex none static

[alexander-schranz]

The idea behind that we did make the method of the ReindexProvider static:

    public static function getIndex(): string;

was that static methods are able to be called without instantiate the class trigger lazy service to be loaded.

The problem is if we want reuse a ReindexProvider for example when creating a Index per language we would require to create a Reindex class also per language. As languages are in Sulu depending on other service it would be best to make it none static so something like:

    public function getIndex(): string
    {
        return 'website' . $this->locale;
    }

Can be implemented.

Sadly this would be kind of BC break: https://3v4l.org/nfFFJ not sure why but PHP does not allow that a implementation does make a method of interface static.

@Toflar think you also had in contao once request similar change? But as the name != real index name you could workaround it.

[Toflar]

I think this is a design issue of the current SEAL implementation. Istead of having a getIndex(): string I would probably implement this as a supportsIndex(Index $index): bool. Then it can be dynamic 😉

[alexander-schranz]

Interesting idea, let me think about it :)

[Toflar]

There's of course the disadvantage that if you have 300 providers, you will have to ask all the 300 every single time you run a reindex. It's the same logic as e.g. Symfony's security voters use. You could optimize this by asking them for some cache information (in Symfony Security that would be the CacheableVoterInterface) but honestly: Will you ever have an application with 300 reindex providers? You'll probably have 10 at most - so who cares.

If you want to implement such a logic anway, I suggest that you don't go the Symfony way with the CacheableVoterInterface. I feel this is too complex for what it should be and the architecture clearly shows that it was introduced later on so it had to be its own interface and its own methods. But if you consider this from the very beginning, you could do it like this:

public function supportsIndex(Index $index): SupportsIndexResult

class SupportsIndexResult
{
    public function supportsIndex(): bool;

    public function getCacheKey(): string|null
}

Then the reindex providers can return the information whether they support this index or not and if this information can be cached (and if so, with what cache key) in the same method call.

[alexander-schranz]

@Toflar I'm unsure about the usecases of supportsIndex(Index $index)? Do you know a usecase where one service does support multiple indexes, which could not be handled by register the same class multiple times as a service?

As forwarding the provide(ReindexConfig $reindexConfig) would be not enough as it would maybe require also then provide(Index $index, ReindexConfig $reindexConfig) or a modified ReindexConfig, else the service would not know for which indexes the returned data should be if reindex over all indexes is done.

/cc @wachterjohannes can you think of usecases?

[Toflar]

Sure, for example if you have an index per user role to ensure proper access permissions.

class MyProvider implements ReindexProviderInterface
{
    public function supportsIndex(Index $index): bool
    {
        return null !== $this->getRole($index->name);
    }

    public function provide(Index $index, ReindexConfig $reindexConfig): \Generator
    {
        $role = $this->getRole($index->name);

        // Do the heavy computing which is exactly the same except for access checks based on the role

        yield ...
    }

    private function getRole(string $indexName): ?string
    {
        if (preg_match('/^index_([a-zA-Z]+)$/', $indexName, $matches)) {
            return $matches[1];
        }

        return null;
    }
}

[alexander-schranz]

@Toflar make sense. I think about handling the Dynamic case via early returns instead of separate supports method:

class MyProvider implements DynamicReindexProviderInterface
{
    public function total(Index $index): int|null;
    {
         if ($this->indexSupported($index)) { // private method call
             return 0;
         }

         // return total
    }

    public function provide(Index $index, ReindexConfig $reindexConfig): \Generator
    {
        if ($this->indexSupported($index)) { // private method call
            return;
        }

        yield ...
    }
}

As it feels less prone to errors as not every call to the index provider need call a support method before.

[Toflar]

Also an option, yes.

[alexander-schranz]

Will go with that, think also Dynamic way is something I can reuse for ODM (#81) where own reindex interface will exist which work with the ODM Object and have a Dynamic which bridge convert it into the array structure.