Skip to content

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

Draft
lukinovec wants to merge 4 commits into
masterfrom
scout-integration
Draft

[4.x] Add integration guide for Laravel Scout#193
lukinovec wants to merge 4 commits into
masterfrom
scout-integration

Conversation

@lukinovec

@lukinovec lukinovec commented Aug 29, 2022

Copy link
Copy Markdown
Collaborator

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

stancl commented Aug 29, 2022

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

stancl commented Aug 30, 2022

Copy link
Copy Markdown
Owner

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

Can you link to the code?

@lukinovec

Copy link
Copy Markdown
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

stancl commented Aug 30, 2022

Copy link
Copy Markdown
Owner

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

@stancl

stancl commented Aug 30, 2022

Copy link
Copy Markdown
Owner

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
Copy Markdown
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

stancl commented Aug 30, 2022

Copy link
Copy Markdown
Owner

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

stancl commented Aug 30, 2022

Copy link
Copy Markdown
Owner

@lukinovec

Copy link
Copy Markdown
Collaborator Author

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

That sounds good

@stancl

stancl commented Sep 1, 2022

Copy link
Copy Markdown
Owner

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
Copy Markdown
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
Copy Markdown
Collaborator Author

@stancl, I've updated the guide

@stancl

stancl commented Sep 8, 2022

Copy link
Copy Markdown
Owner

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
Copy Markdown
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