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

[8.x] Custom ‘with’ eager loading definitions for bulk searchable indexing #406

Conversation

serpentblade
Copy link
Contributor

Adds support for $searchableWith as an override to the built-in $with member on models. This allows a custom eager loading strategy specific to indexing requirements

Related Feature Request: #329

@serpentblade serpentblade force-pushed the feature/import-custom-with-param branch 2 times, most recently from 71178cc to 90980e6 Compare June 21, 2020 20:44
Adds support for `$searchableWith` as an override to the built-in `$with` member on models.  This allows a custom eager loading strategy specific to indexing requirements
@serpentblade serpentblade force-pushed the feature/import-custom-with-param branch from 90980e6 to 7acc900 Compare June 21, 2020 20:47
@driesvints driesvints changed the title Custom ‘with’ eager loading definitions for bulk searchable indexing [8.x] Custom ‘with’ eager loading definitions for bulk searchable indexing Jun 22, 2020
@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

@serpentblade
Copy link
Contributor Author

Hi @taylorotwell,

I completely understand and respect the desire to keep the framework minimal due to maintenance requirements. While this does not correct a bug in the framework in the traditional sense, I feel it corrects a performance limitation with regard to how data is loaded by the scout:import command.

As noted in #329, the requirements for eager loading when indexing can often be very different from the requirements for general use of model in the framework. For example, I may want to include a number of additional fields from relations to be used for faceted search, but I would not necessarily want those same relations eagerly loaded in my actual application.

It should also be noted that some of the solutions suggested, such as overriding makeAllSearchable() feels like a generally undesirable solution as it always runs the risk of the original implementation changing in future versions. I would deem that a workaround rather than a solution.

I did note another PR that was rejected with a similar solution (#342), but made sure to avoid the same breaking changes it introduced. My implementation is meant to provide an optional set of eager loading rules, meaning it retains full backwards compatibility with existing usage of $with, and that only when a dev explicitly defines the new $searchableWith parameter will it affect the output from the default implementation of toSearchableArray() and override the existing eager loading rules defined in $with.

Ultimately, I will respect your decision on this either way. But I would be remiss to not attempt to argue for the performance benefits this introduces to the indexing process. Coupled with my other PR that was recently merged (thank you), it provides the other half of the missing pieces to making the scout:import feel feature complete.

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.

None yet

2 participants