algolia / laravel-scout-settings

DEPRECATED: Use of this repository is deprecated. Please use Scout Extended - https://github.com/algolia/scout-extended instead.
MIT License
23 stars 12 forks source link

Question: rationale behind using the prefix in file names? #14

Closed qrazi closed 6 years ago

qrazi commented 6 years ago

I would like to pose this as a question, since I am not sure if my use case makes sense or not.

Background I am migrating a Laravel 5.1 + algolia/algoliasearch-laravel application to Laravel 5.5 + Laravel Scout. This application is worked on by multiple developers, and the application is deployed on DTAP-style environments. In the current setup all index settings are properties of respective models. The $perEnvironment setting is used as well. Using this we have achieved a index per environment, with each developer having his own index (because he/she names the environment "jdoe_dev").

Current situation The model contains the index settings. On a deploy (on any of the DTAP environments), the settings as they are at that moment in git history are pushed to Algolia with a custom Artisan command that hooks into the algoliasearch-laravel API.

Laravel Scout Settings Since we're using multiple indexes in the same Algolia app, i.e. for test, acceptance and production, I would have to commit the setting-json files in triple. But I would have to manually update those files, since on my dev environment, the backup command would create another set of json files. Also, if at some point the environment name would change (which we did in the past, when we replaced "staging" with "test" and "acceptance"), it would require some more changes to the setting files as well.

I see no benefit to this. The way the plugin works, it would always only use one set of json files.

Suggested change I think it would be possible to cater to my use case (which I don't think is uncommon/illogical, but correct me if I'm wrong... 😅) whilst keeping the default behavior the same. Instead of directly using the index name to concat the filename, use a mediating function in between, something like AlgoliaCommand::getBaseFileName($indexName). By default this would then just return $indexName, but if set, an alternative callback could be executed to get the base file name to be used.

If you want, I could try and make a PR for this?

julienbourdeau commented 6 years ago

Hi @qrazi,

thanks for the detailed issue. I believe you're right! I have recently redone the Symfony bundle for instance (which is very much inspired by Scout) and I'm not using the prefix, for the same reason you stated: https://github.com/algolia/search-bundle/pull/196

I think it was just a bad idea at the time ^^ I'll release a new version before the weekend :)

Thanks a lot for pointing it out.

qrazi commented 6 years ago

Hi @julienbourdeau,

No problem, I'm glad my use case got some validation... 😅 Your solution for the Symfony bundle would definitely solve my problem... 👍

Since ::searchableAs() is specifically mentioned as override-able in the documentation, there might still be some benefit from allowing custom base file names, but might as well put that on the road map until needed?

Thanks in any case for the quick response! Looking forward to the new version 😄

qrazi commented 6 years ago

I actually have another use case as well, that sort of intersects with functionality provided by this package. The application is multi-language. The index records contain value for all locales, and the localized properties (i.e. name, summary) are suffixed with the locale. A record would thus have the properties name_nl and name_en.

In our current situation we use the index settings from a model, filter it on the current locale, and pass the set of searchable attributes to the view layer, where the Algolia InstantSearch client will do the actual querying of the index, and add a restrictSearchableAttributes to all queries. This prevents the return of matches in other locales.

The benefit of doing this vs suffixing the locale in JS is less JS logic happening, thus lighter. And for me personally makes more sense that the restrictions are supplied by the backend.

Now to my question; in the Laravel 5.5 + Laravel Scout + Laravel Scout Settings setup, those settings are stored in the json files. Would it make sense that the algolia/algoliasearch-laravel package exposes an API to Laravel for getting those settings, or at least the logic for the file name and path to the settings? Right now I am sort copying logic from the algolia/algoliasearch-laravel package to get to those json files, which will break the moment that logic changes... 😄

Again, I'd be willing to help with implementing something like that, just need a few pointer as to what the implementation would need to look like to be accepted... 👍

julienbourdeau commented 6 years ago

I opened a PR that simply remove the prefix from the name. It breaks backward compatibility.

If I understood what you mean, you would like to have a simple way to get the settings for a given model, like the following snippet. Today, all the logic is in the command so it's not very usable.

// Somewhere in a controller
$postSettings = Algolia\Settings\Facade\Ressource::getSettings(Post::class);

I called it Resource, since it should manage settings If you want to open a pull request, I think it will be great to have an AlgoliaSettings class that holds to logic of reading/writing the json files and figuring out the correct name. It would solve the issue for people adding logic to their searchableAs method. And maybe add a nice facade for easy access.

What do you think?

qrazi commented 6 years ago

I think we are thinking along the same line there for sure. I will try to make the PR this weekend, tnx!