studioespresso / craft-scout

Craft Scout provides a simple solution for adding full-text search to your entries. Scout will automatically keep your search indexes in sync with your entries.
MIT License
81 stars 55 forks source link

Unknown method exception `searchable` #257

Closed joshuabaker closed 1 year ago

joshuabaker commented 1 year ago

We’re seeing an error when saving user elements, which are not included in any configured index.

Calling unknown method: craft\elements\User::searchable()

At present, it appears to be caused by recent changes in Freeform, which has been referenced in the past (#105, #199).

Mostly posting here for reference and posterity.

janhenckens commented 1 year ago

Hey @joshuabaker thanks for letting me know. Haven't run into this myself and I can't immediately reproduce it with freeform installed. I'll leave this open for now.

joshuabaker commented 1 year ago

Going to close this. We’ve thankfully not encountered it since.

john-henry commented 1 year ago

Started to run into this lately too for

Calling unknown method: craft\elements\User::searchable()

and also

Calling unknown method: craft\commerce\elements\Order::searchable()

Again both of which are not in any configured index. Will I open a new issue?

No Freeform installed

janhenckens commented 1 year ago

@john-henry can you check that you're on the latest version of scout? Which Craft version are you seeing this on?

john-henry commented 1 year ago

Craft 3.7.33 and Scout 2.4.0

Then same behaviour on Craft 3.7.33 and Scout 2.7.2

joshuabaker commented 1 year ago

@john-henry I found the Freeform issue by toggling off each plugin manually and checking. Seems to be a bunch of plugins that don’t properly register behaviours, which is how searchable is attached to elements.

john-henry commented 1 year ago

OK Ill give that a go although nothing has changed really in about a year. no new updates on Craft or plugins

john-henry commented 1 year ago

OK found the bugger. Its the Guide plugin. Feel free to close this again

joshuabaker commented 1 year ago

@john-henry Congrats on locating it. You might file an issue on the Guide repo.

@janhenckens This issue can be a real nightmare to track down. It might be prudent to run further checks around behaviour calls, or not use behaviours at all; Is there a specific reason for using them?

janhenckens commented 1 year ago

Should have it fixed in Craft 4 but I should check if the Craft function I'm using there was backported to 3.x.

As to using behaviours, that was done before I took over maintenance of the plug-in so can't really speak to that. Refactoring that could be on option but only as a last resort I think.

john-henry commented 1 year ago

We will get to Craft 4 one of these days. Ill file an issue on Guide repo. Thanks @joshuabaker

janhenckens commented 1 year ago

@john-henry Feel free to tag this issue there, so I can keep track of things. Also, Which version of Guide were you running here?

joshuabaker commented 1 year ago

As to using behaviours, that was done before I took over maintenance of the plug-in so can't really speak to that. Refactoring that could be on option but only as a last resort I think.

Tend to agree, @janhenckens, especially as the code in here isn’t actually the culprit. One of those unfortunate if-only-everyone-else-drove-safely issues. 🤦🏻

janhenckens commented 1 year ago

@john-henry Can I ask when or where you're seeing the error? I added Guide to my Craft 3 test install but can't reproduce it.

joshuabaker commented 1 year ago

This has returned for us unexpectedly and is causing issues for users trying to register again. Once again, it looks to be caused by Freeform.

qrazi commented 1 year ago

So I ran into this same issue. And the comments here were very helpful to chase down why, in my project, this occurs.

Now, the criteria callback gets executed upon loading this plugin, so in our case the getIdentity() actually gets executed before this plugin registers the SearchableBehavior on the Elements. Later on in a Controller we call getIdentity() to grab the logged in User, to allow modifications - changing an avatar in our case. The User instance doesn't have the SearchableBehavior loaded, but does get send through the EVENT_AFTER_SAVE_ELEMENT etc event listeners and those event listeners all expect SearchableBehavior to be attached - i.e. to be able to call searchable() etc.

For now, I fixed this in our project by simply querying again for the current logged in user - $reloaded_user = User::findIdentity($user->getId());. $reloaded_user now does have the SearchableBehavior attached and the rest of our code works as expected.

It also made me realize that this plugin always executes the callbacks defined in the config, even on requests that don't necessarily are going to be using this plugin - i.e. most site requests.

So, would it make sense, and I could then try to PR it, to refactor this plugin to lazy-call the callbacks? Rough draft and assuming magic methods (mostly for BC):

public function criteria(callable $criteria): self
{
    $this->_criteria = $criteria;

    return $this;
}

public function getCriteria(): ElementQuery
{
    if (is_callable($this->_criteria)) {
        $elementQuery = call_user_func($this->_criteria, $this->elementType::find());

        if (!$elementQuery instanceof ElementQuery) {
            throw new Exception('You must return a valid ElementQuery from the criteria function.');
        }

        if (is_null($elementQuery->siteId)) {
            $elementQuery->siteId = Craft::$app->getSites()->getPrimarySite()->id;
        }

        $this->_criteria = $elementQuery;
    }

    return $this->_criteria;
}
joshuabaker commented 1 year ago

@qrazi That's not widely dissimilar to what I was trying to do.

I'm still thinking it might make sense to switch this away from behaviours completely. There's lots of potential conflicts that seem to surface using them with Craft.

joshuabaker commented 1 year ago

@janhenckens Did you ship a version of #262 for Craft 3.x?

qrazi commented 1 year ago

@qrazi That's not widely dissimilar to what I was trying to do.

I'm still thinking it might make sense to switch this away from behaviors completely. There's lots of potential conflicts that seem to surface using them with Craft.

If I understand your PR correctly, my suggestion could still be complementary, I think. It's not tied per se to using the Behaviors, that's just how I showed the suggested improvement :sweat_smile:

I think using Behaviors would make it easiest to have no BC, which could be a big pro? What I mean is that during configuration criteria() (the method) is called, and when the Query is needed, it is accessed as ->criteria (the property). Extending BaseObject - or implementing magic __get etc ourselves - it is possible to keep the API; it could remain ->criteria, but the callable would be lazy-called (not sure what the technical call for that would be, haha).

I'll try to create an actually PR for this and let's see :sweat_smile:

janhenckens commented 1 year ago

Thanks for the input and the PR @joshuabaker @qrazi! I would preferably not have any BC for now - we'll re-evaluate that when Craft 5 comes around.

Would do you up for testing this is a beta release for Craft 4? I have a couple of sites I can test in on but the more the merrier :)

qrazi commented 1 year ago

I have a Craft 3 site that's using Algolia, I can definitely run a beta there.

And I think I saw failing builds on the PRs, I'll look into it tomorrow!

janhenckens commented 1 year ago

3 works too! I think your PR's started from branches that didn't include the fixed tests from your other PR - I've merged those in develop and craft3 earlier today so rebasing from those again might do the trick @qrazi

janhenckens commented 1 year ago

@qrazi @joshuabaker I've combined #262 and #269 in a Craft 3 beta release - which already included a feature that moved the "should this be indexed to a queue job". This is the version you'll want to install:

composer require studioespresso/craft-scout:2.8.0-beta.3

qrazi commented 1 year ago

I've added the beta version to our develop branch. It's running currently on our (low traffic) staging server. I'll update this issue when we're running it in production.

qrazi commented 1 year ago

Forgot to update, since last week Friday this is also running in production for us. Multi-site Craft 3 with ~10k / ~15k visitors according to GA.

janhenckens commented 1 year ago

Thanks for the update @qrazi, sounds like we're good to go. I'll carve out some time over the weekend to get this released for both 3 & 4.

janhenckens commented 1 year ago

This fix is now out for both 3 & 4.