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

[10.x] Fix custom scout keys not being utilized when deleting from queue #657

Merged

Conversation

stevebauman
Copy link
Contributor

Related #656

Sending this in for tests to run and ensure everything passes with the recent changes of the above PR. 👍

@stevebauman stevebauman marked this pull request as draft September 30, 2022 16:15
@driesvints
Copy link
Member

Thanks @stevebauman. I'll use this PR as a reference once 9.x is updated and I merge it into master.

@driesvints
Copy link
Member

@stevebauman @mmachatschek I've merged 9.x into master now but unsure if everything is now okay. If you have the time, can you maybe check if everything looks in order? Looks like this PR is also conflicting now.

Here's my merge commit: 0337e4c

@stevebauman
Copy link
Contributor Author

Hey @driesvints, thanks for the ping! I'll review in a couple hours today and comment back 👍

@stevebauman stevebauman marked this pull request as ready for review December 7, 2022 23:33
@stevebauman
Copy link
Contributor Author

stevebauman commented Dec 8, 2022

@driesvints Okay I've completed reviewing and have updated my PR with master.

I have removed the getUnqualifiedScoutKeyName() inside the Searchable trait. We no longer need it, because in v9, getScoutKeyName() was returning the fully qualified key name for the Eloquent model (i,e. {table}.id):

scout/src/Searchable.php

Lines 388 to 391 in 8489fac

public function getScoutKeyName()
{
return $this->getQualifiedKeyName();
}

In v10, this was adjusted to return only the key name instead (i.e. id):

scout/src/Searchable.php

Lines 390 to 393 in 0337e4c

public function getScoutKeyName()
{
return $this->getKeyName();
}

This should not cause any side-effects for those upgrading to v10. getScoutKeyName() was only used in one location in v9:

scout/src/Searchable.php

Lines 247 to 249 in 8489fac

return $query->{$whereIn}(
$this->getScoutKeyName(), $ids
);

However, if a developer has overrode getScoutKeyName() on their Searchable model with a hard-coded string matching the previous format for some reason (the fully qualified key naming structure), such as:

// app/Models/Post.php

public function getScoutKeyName()
{
    return 'posts.id';
}

Then they must update their formatting to return only the DB column itself, excluding the DB table to maintain compatibility with the v10 update:

public function getScoutKeyName()
{
-    return 'posts.id';
+    return 'id';
}

Let me know if there's anything else I can help with! 🙏

@driesvints driesvints marked this pull request as draft December 8, 2022 14:12
@driesvints driesvints marked this pull request as draft December 8, 2022 14:12
@driesvints driesvints marked this pull request as draft December 8, 2022 14:12
@driesvints driesvints marked this pull request as ready for review December 9, 2022 09:47
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.

This looks good @stevebauman. Thank you for your work on this!

@taylorotwell
Copy link
Member

Hi @stevebauman - I see this is sent to master branch. Can you elaborate on any breaking changes or notes we would need to make in the upgrade guide about this? Basically any relevant notes to what this changes would be helpful to document in this thread.

@stevebauman
Copy link
Contributor Author

Hey @taylorotwell, I can definitely do that, but I'm leaving my home for the weekend. I'll be able to write something up on Monday! 👍

@taylorotwell
Copy link
Member

OK - mark as ready for review when that is done. Thanks!

@taylorotwell taylorotwell marked this pull request as draft December 9, 2022 21:50
@stevebauman
Copy link
Contributor Author

Okay I've done a run though. There are two breaking changes here:

  1. Removal of the Searchable::getUnqualifiedScoutKeyName() method.

This method was introduced in v9 in a PR I wrote (#656) to prevent logic duplication of retrieving the key name from the fully qualified key name that was being returned from the getScoutKeyName() method.

  1. Change of the Searchable::getScoutKeyName() return value.

The getScoutKeyName() method now returns the unqualified key name from the model by default, instead of the fully qualified name that includes the model's database table. As mentioned above, developers who have overridden the method will need to update their definition to return the unqualified key name to maintain compatibility with v10:

public function getScoutKeyName()
{
-    return 'posts.id';
+    return 'id';
}

This is because this method is now being used in the built-in search engine implementations (such as to store the models primary key in the indexed record), where it was not being used previously. Ex:

MeiliSearch Before:

[$model->getKeyName() => $model->getScoutKey()],

MeiliSearch After:

[$model->getScoutKeyName() => $model->getScoutKey()],

Developers that have not overridden the getScoutKeyName() method, or have not utilized the getUnqualifiedScoutKeyName() method, will not have to adjust anything.

Let me know if you have any further questions or would like me to tackle additional related tasks!

@stevebauman stevebauman marked this pull request as ready for review December 12, 2022 02:55
@taylorotwell taylorotwell merged commit 1eacfd9 into laravel:master Dec 14, 2022
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.

3 participants