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

[4.x] Add integration guide for Laravel Scout #193

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lukinovec
Copy link
Collaborator

@lukinovec lukinovec commented Aug 29, 2022

In response to #72, I'm adding a short integration guide for Laravel Scout.

image

@lukinovec lukinovec requested a review from stancl August 29, 2022 13:12
@stancl
Copy link
Owner

stancl commented Aug 29, 2022

The page looks great. A few notes:

  • Ideally we'd cover reverting back to the central context as well (this is a common thing in the package). So we should have the opposite listener as well
  • Regarding to the previous point, does changing scout.prefix work after Scout has already read from the config? In most packages the value gets "cached" inside a class property in some singleton
  • Maybe you could turn this into a simple TenancyBootstrapper

@lukinovec
Copy link
Collaborator Author

does changing scout.prefix work after Scout has already read from the config?

It should work, Scout gets the index name via the config() helper (and that happens only during the Scout search operations in the Searchable trait's searchableAs()). So the only way for this not to work would be making the config key change in the middle of a search operation, and I don't think that's going to happen.

Ideally we'd cover reverting back to the central context as well (this is a common thing in the package). So we should have the opposite listener as well

Right. I added that to the page.

Maybe you could turn this into a simple TenancyBootstrapper

I wrote the bootstrapper:

class ScoutTenancyBootstrapper implements TenancyBootstrapper
{
    /** @var Repository */
    protected $config;

    public function __construct(Repository $config)
    {
        $this->config = $config;
    }

    public function bootstrap(Tenant $tenant)
    {
        $this->config->set('scout.prefix', $tenant->getTenantKey());
    }

    public function revert()
    {
        $this->config->set('scout.prefix', '');
    }
}

@stancl
Copy link
Owner

stancl commented Aug 30, 2022

It should work, Scout gets the index name via the config() helper

Can you link to the code?

@lukinovec
Copy link
Collaborator Author

It should work, Scout gets the index name via the config() helper

Can you link to the code?

Sure, here's the searchableAs method

@stancl
Copy link
Owner

stancl commented Aug 30, 2022

I see, and that gets invoked every time, right? The value isn't cached anywhere like I mentioned it could be.

@stancl
Copy link
Owner

stancl commented Aug 30, 2022

Also, looking at the bootstrapper I'm thinking that we could make this a first-party feature rather than something in the docs?

I'd make a few adjustments to the bootstrapper to make it more complete for production. But it could be part of our package directly.

@lukinovec
Copy link
Collaborator Author

I see, and that gets invoked every time, right? The value isn't cached anywhere like I mentioned it could be.

Yeah, that seems to be right. I can't find the value cached anywhere

looking at the bootstrapper I'm thinking that we could make this a first-party feature rather than something in the docs?

I thought about this because the only first-party bootstrappers are for the "core" things (the filesystem, database, etc. – things you need in most apps), so providing a bootstrapper for Scout seemed odd to me. Having the first-party bootstrapper could be helpful, but I think providing the instructions so the user can create it seems better.

@stancl
Copy link
Owner

stancl commented Aug 30, 2022

We have a class that adds Telescope support but it's a feature, not a bootstrapper. The type doesn't matter much though.

I agree that we should somehow keep the repo/namespace clean. Perhaps we could add a folder, like Bootstrappers\Integrations where these would go. With the bootstrappers in the main folder being only for vanilla Laravel features.

@stancl
Copy link
Owner

stancl commented Aug 30, 2022

@lukinovec
Copy link
Collaborator Author

Perhaps we could add a folder, like Bootstrappers\Integrations where these would go.

That sounds good

@stancl
Copy link
Owner

stancl commented Sep 1, 2022

Can you try to open a PR to the Tenancy repo adding this class? Into the Bootstrappers\Integrations namespace.

I wanted to work on this myself, since I wanted to make some changes, but you might be able to do that on your own.

Simply: add a property whose value gets set in bootstrap() if it's not isset() yet (so that it gets set only once), to store the original value of the config key, and then in revert() you'll revert the config to that value.

And you can inject the Config\Repository in __construct() to interact with the config.

@lukinovec
Copy link
Collaborator Author

Can you try to open a PR to the Tenancy repo adding this class? Into the Bootstrappers\Integrations namespace.

I wanted to work on this myself, since I wanted to make some changes, but you might be able to do that on your own.

Simply: add a property whose value gets set in bootstrap() if it's not isset() yet (so that it gets set only once), to store the original value of the config key, and then in revert() you'll revert the config to that value.

And you can inject the Config\Repository in __construct() to interact with the config.

Opened the PR archtechx/tenancy#936

@lukinovec
Copy link
Collaborator Author

@stancl, I've updated the guide

@stancl
Copy link
Owner

stancl commented Sep 8, 2022

The guide now includes v4 syntax, right? So the PR should be changed to a draft and merged when we write v4 docs?

@lukinovec
Copy link
Collaborator Author

The guide now includes v4 syntax, right?

I see, yeah, the bootstrapper is a v4 thing, so I'm changing this to draft now

@lukinovec lukinovec marked this pull request as draft September 9, 2022 05:11
@lukinovec lukinovec changed the title Add integration guide for Laravel Scout [4.x] Add integration guide for Laravel Scout Sep 9, 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.

2 participants