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

feat: add Elasticsearch Engine #3

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rafaucau
Copy link

@rafaucau rafaucau commented Jun 22, 2024

As in the title

@rafaucau
Copy link
Author

rafaucau commented Jun 22, 2024

Currently, I can't get it working.

PHP Fatal error:  Uncaught ArgumentCountError: Too few arguments to function ClarkWinkelmann\Scout\ScoutModelWrapper::__construct(), 0 passed in /var/www/html/vendor/illuminate/database/Eloquent/Model.php on line 2144 and exactly 1 expected in /var/www/workbench/flarum-ext-scout/src/ScoutModelWrapper.php:19
Stack trace:
#0 /var/www/html/vendor/illuminate/database/Eloquent/Model.php(2144): ClarkWinkelmann\Scout\ScoutModelWrapper->__construct()
#1 /var/www/html/vendor/matchish/laravel-scout-elasticsearch/src/ElasticSearch/Params/Bulk.php(44): Illuminate\Database\Eloquent\Model::__callStatic()
#2 /var/www/html/vendor/illuminate/collections/Traits/EnumeratesValues.php(768): Matchish\ScoutElasticSearch\ElasticSearch\Params\Bulk->Matchish\ScoutElasticSearch\ElasticSearch\Params\{closure}()
#3 /var/www/html/vendor/matchish/laravel-scout-elasticsearch/src/ElasticSearch/Params/Bulk.php(42): Illuminate\Support\Collection->reduce()
#4 /var/www/html/vendor/matchish/laravel-scout-elasticsearch/src/Engines/ElasticSearchEngine.php(48): Matchish\ScoutElasticSearch\ElasticSearch\Params\Bulk->toArray()
#5 /var/www/workbench/flarum-ext-scout/src/ScoutServiceProvider.php(103): Matchish\ScoutElasticSearch\Engines\ElasticSearchEngine->update()
#6 /var/www/html/vendor/illuminate/macroable/Traits/Macroable.php(124): Flarum\Database\Eloquent\Collection->ClarkWinkelmann\Scout\{closure}()
#7 /var/www/workbench/flarum-ext-scout/src/FlarumSearchableScope.php(46): Illuminate\Support\Collection->__call()
#8 /var/www/html/vendor/illuminate/database/Concerns/BuildsQueries.php(139): ClarkWinkelmann\Scout\FlarumSearchableScope->ClarkWinkelmann\Scout\{closure}()
#9 /var/www/workbench/flarum-ext-scout/src/FlarumSearchableScope.php(39): Illuminate\Database\Eloquent\Builder->chunkById()
#10 /var/www/html/vendor/illuminate/database/Eloquent/Builder.php(1639): ClarkWinkelmann\Scout\FlarumSearchableScope->ClarkWinkelmann\Scout\{closure}()
#11 /var/www/workbench/flarum-ext-scout/src/ScoutStatic.php(52): Illuminate\Database\Eloquent\Builder->__call()
#12 /var/www/workbench/flarum-ext-scout/src/Console/ModifiedImportTrait.php(22): ClarkWinkelmann\Scout\ScoutStatic::makeAllSearchable()
#13 /var/www/workbench/flarum-ext-scout/src/Console/ImportAllCommand.php(29): ClarkWinkelmann\Scout\Console\ImportAllCommand->handleClass()
#14 /var/www/html/vendor/illuminate/container/BoundMethod.php(36): ClarkWinkelmann\Scout\Console\ImportAllCommand->handle()
#15 /var/www/html/vendor/illuminate/container/Util.php(40): Illuminate\Container\BoundMethod::Illuminate\Container\{closure}()
#16 /var/www/html/vendor/illuminate/container/BoundMethod.php(93): Illuminate\Container\Util::unwrapIfClosure()
#17 /var/www/html/vendor/illuminate/container/BoundMethod.php(35): Illuminate\Container\BoundMethod::callBoundMethod()
#18 /var/www/html/vendor/illuminate/container/Container.php(653): Illuminate\Container\BoundMethod::call()
#19 /var/www/html/vendor/illuminate/console/Command.php(136): Illuminate\Container\Container->call()
#20 /var/www/html/vendor/symfony/console/Command/Command.php(298): Illuminate\Console\Command->execute()
#21 /var/www/html/vendor/illuminate/console/Command.php(120): Symfony\Component\Console\Command\Command->run()
#22 /var/www/html/vendor/symfony/console/Application.php(1058): Illuminate\Console\Command->run()
#23 /var/www/html/vendor/symfony/console/Application.php(301): Symfony\Component\Console\Application->doRunCommand()
#24 /var/www/html/vendor/symfony/console/Application.php(171): Symfony\Component\Console\Application->doRun()
#25 /var/www/html/vendor/flarum/core/src/Console/Server.php(42): Symfony\Component\Console\Application->run()
#26 /var/www/html/flarum(24): Flarum\Console\Server->listen()
#27 {main}
 thrown in /var/www/workbench/flarum-ext-scout/src/ScoutModelWrapper.php on line 19

After changing the constructor of ScoutModelWrapper from:

public function __construct(Model $realModel)
{
    parent::__construct([]);

    $this->realModel = $realModel;
}

to:

public function __construct(Model $realModel = null)
{
    parent::__construct([]);

    if($realModel) {
        $this->realModel = $realModel;
    }
}

I get the following error:

$ php flarum scout:import-all

In ScoutModelWrapper.php line 205:
                                                                       
  Native Laravel soft delete meta not implemented in Scout for Flarum  
                                                                       

scout:import-all [-c|--chunk [CHUNK]]

@clarkwinkelmann
Copy link
Owner

You cannot change the constructor of ScoutModelWrapper, it needs a reference to the model it's wrapping around. Also, it's only supposed to be instantiated by the extension itself.

If I understand correctly, from my quick look at the error trace and the source code of the package on GitHub https://github.com/matchish/laravel-scout-elasticsearch/blob/master/src/ElasticSearch/Params/Bulk.php#L44 it's related to the usesSoftDelete() static method.

In my extension, I actually re-implemented every part of the code from Scout that called that method, hence why I created a no-op version that throws an exception every time in

protected static function usesSoftDelete()
{
throw new \Exception('Native Laravel soft delete meta not implemented in Scout for Flarum');
}
That exception is the one you see in your second problem.

The reason why you don't hit the intended exception on the first attempt seems to be because instead of calling $class::usesSoftDelete(), the package actually calls $model::usesSoftDelete(), which invokes a Laravel magic method I didn't re-implement on the model wrapper.

The most reliable solution would probably to re-implement that Bulk class in the extension itself, replacing everything with the internal calls like I did for the Scout jobs, then map that replacement class where it's needed through container binds.

Otherwise, to make the code works without any override, you could probably add a __callStatic method on ScoutModelWrapper with the same signature as Laravel and if the method name is usesSoftDelete, call the no-op static method on the wrapper itself. Then the no-op should be modified to return false rather than throw an exception. Or you might be able to copy the same code Laravel uses and create a new static class with $this->realModel as constructor parameter. Accepting null in the constructor and changing the no-op method only would achieve the same result, but is much more likely to cause cryptic errors in the future as all the other methods on the wrapper would need to handle that situation with a clear error message.

I hope this gives you some hints. I don't have a lot of time at the moment, but if this works well this looks like an interesting addition and I'll give it a try!

@clarkwinkelmann
Copy link
Owner

Actually, now that I think of it, maybe not all of the code related to soft-deletes was re-implemented.

I would have to check again, but maybe in the Scout original code it might do

config('scout.soft_delete', false) && ::usesSoftDelete()

instead of that elastic package

::usesSoftDelete() && config('scout.soft_delete', false)

The order is important, because the config() helper doesn't exist in Flarum and this extension makes sure it always returns the default value false. This might be why I never expected usesSoftDelete to be called and preferred the always-throw-exception solution at the time.

@rafaucau
Copy link
Author

rafaucau commented Jun 23, 2024

Thanks for the hints!

I would have to check again, but maybe in the Scout original code it might do
config('scout.soft_delete', false) && ::usesSoftDelete()

Yes, I just checked the flipped if ((testing directly in vendor)) and it is working.
So another option will be a PR to matchish/laravel-scout-elasticsearch.

@rafaucau
Copy link
Author

rafaucau commented Jun 23, 2024

Otherwise, to make the code works without any override, you could probably add a __callStatic method on ScoutModelWrapper with the same signature as Laravel and if the method name is usesSoftDelete, call the no-op static method on the wrapper itself. Then the no-op should be modified to return false rather than throw an exception.

I used this method ^
Tested on local installation. Importing and searching are working.

I also created a PR for matchish/laravel-scout-elasticsearch. If and when the author accepts the PR and releases a new version, this workaround will not be necessary.

@rafaucau rafaucau marked this pull request as ready for review June 23, 2024 12:37
@rafaucau
Copy link
Author

@clarkwinkelmann
Copy link
Owner

Sorry, I didn't get any opportunity to check back on this. Would the PR be ready as-is?

@rafaucau
Copy link
Author

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants