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

[9.x] Use scout key when mapping keys from search results #652

Merged
merged 6 commits into from
Sep 27, 2022
Merged

[9.x] Use scout key when mapping keys from search results #652

merged 6 commits into from
Sep 27, 2022

Conversation

flexchar
Copy link
Contributor

@flexchar flexchar commented Sep 23, 2022

Trying a fix against #651

This PR overloads the keys() method in the base Engine class providing MeilisearchEngine the exact Scout key to be used when mapping search results.

Previously the key was determined by taking the first key of the array in the search response. However as recently discovered in the aforementioned issue - Meilisearch will return random array order depending on its index configuration.

@taylorotwell
Copy link
Member

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

@driesvints
Copy link
Member

@flexchar please see the pr template when submitting pr's and add a thorough description to your PR

@flexchar
Copy link
Contributor Author

Fair enough. Unfortunately I'm not available today but I'd love to try submitting a proper PR for the experience. I could totally do it this weekend @driesvints. :)

@driesvints driesvints reopened this Sep 26, 2022
@driesvints driesvints marked this pull request as draft September 26, 2022 08:43
@driesvints
Copy link
Member

@flexchar since this is only happening in the meilisearch engine maybe we should revert the changes to the base engine class and do them in the mapIds method of the meilisearch engine?

@flexchar
Copy link
Contributor Author

I'm back online! Let me try to tackle this right now. Can you link to the commit of changes you're talking about, @driesvints?

@driesvints
Copy link
Member

There's only a few changes in this PR so not sure what you want me to link to?

@flexchar
Copy link
Contributor Author

Perhaps! What do you mean by this part?

...maybe we should revert the changes to the base engine class and do them in the mapIds...

@driesvints
Copy link
Member

You added changes to src/Engines/Engine.php and I feel we should only add them to src/Engines/MeiliSearchEngine.php because it only affects Meilisearch.

@flexchar
Copy link
Contributor Author

Ah... gotcha. I thought you were referring to a change made in the past.

To be honest as much as I'm looking at the abstract base Engine class has no implementation to providing models' data to the driver class. As such, I cannot foresee any other way than going hacky and using debug_backtrace() function which would make the function unpure.

On the contrary, adding using mapIdsFrom instead of mapIds would give drivers more concrete requirements on what to map and avoid pushing developers into ambiguous implementations. I'd vote for this. Also backwards compatible because base method does not actually use key:

public function mapIdsFrom($results, $key)

OR. Now that I think there is nothing stopping us from overloading a keys() method from MeilisearchEngine driver. :)

@flexchar
Copy link
Contributor Author

I have the proper commit ready if I may force push one last time - somehow things got messed up this time.

@driesvints
Copy link
Member

Sure. Was just trying to fix stuff

@flexchar
Copy link
Contributor Author

@driesvints should be good & ready for the review now.

Thank you for the opportunity to practice.

@flexchar flexchar marked this pull request as ready for review September 26, 2022 11:51
Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay to me but would like a review from @mmachatschek as well if possible.

@driesvints
Copy link
Member

I think I agree with @mmachatschek. @flexchar let's send this to master instead and maybe look forward to prepare Scout v10.

@driesvints driesvints closed this Sep 27, 2022
@flexchar
Copy link
Contributor Author

flexchar commented Sep 27, 2022

I'm working on several projects that are soon to receive custom searchable attributes thus causing this issue arise. Is v10 close down the road? :)

And I can send one directly to v10. It seems like the change with less code! Should I sent PR against master branch?

@driesvints
Copy link
Member

Actually, on second thought the overwriting of the behavior of the keys method might not be such a breaking change as it's technically a bug fix.

@driesvints driesvints reopened this Sep 27, 2022
Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taylorotwell this one is ready for review now.

@flexchar flexchar changed the title Use scout key when mapping keys from search results [9.x] Use scout key when mapping keys from search results Sep 27, 2022
@taylorotwell taylorotwell merged commit ce0fd9a into laravel:9.x Sep 27, 2022
@driesvints
Copy link
Member

Thanks @flexchar

@flexchar flexchar deleted the patch-1 branch September 28, 2022 09:31
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.

Scout won't return correct model ->keys() when using custom searchableAttributes from Meilisearch
4 participants