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

Update Livewire integration for Tenancy by path #206

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kaisersly
Copy link

I made some minor changes to @FelipeVeiga proposal and added them to the documentation.

See archtechx/tenancy#628 (comment)

kaisersly and others added 2 commits September 26, 2022 10:19
I made some minor changes to @FelipeVeiga proposal and added them to the documentation.

archtechx/tenancy#628 (comment)

(Don't forget to import the middleware class.)

Open the `config/livewire.php` file and add this:
Copy link
Owner

Choose a reason for hiding this comment

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

As @andreasmatu pointed out in archtechx/tenancy#628 (comment), this should be something like routes/tenant.php:

Suggested change
Open the `config/livewire.php` file and add this:
Open the `routes/tenant.php` file and add this:

@stancl
Copy link
Owner

stancl commented Oct 28, 2022

@lukinovec can you test this, check why the build is failing, and see if file uploads work for you: archtechx/tenancy#628 (comment)

Co-authored-by: lukinovec <lukinovec@gmail.com>
@lukinovec
Copy link
Collaborator

see if file uploads work for you

I couldn't make them work yet. When I select a file, LW sends a post request to livewire/upload-file (no tenant ID in the path). I get a response saying InitializeTenancyByPath failed because the route doesn't have any parameters (the check for $route->parameterNames()[0] === PathTenantResolver::tenantParameterName() in the middleware fails). I tried using UniversalRoutes, also didn't work. I noticed that UniversalRoutes has the $identificationMiddlewares array and that InitializeTenancyByPath wasn't there, so I added it, but still, I'm getting the same error.

@stancl
Copy link
Owner

stancl commented Nov 4, 2022

I don't think universal routes would fix anything here. They're a thing that's for domains, and even if they worked here, it would just skip tenant identification which is not what we want.

Instead, the approach should be customizing the path used for file uploads, similar to the other change that was made here.

@lukinovec
Copy link
Collaborator

I see. For me, LW works with the prefixed message path after following the integration guide, but the upload-file path remains the same.

@stancl
Copy link
Owner

stancl commented Nov 4, 2022

Try to focus on finding if this is possible:

Instead, the approach should be customizing the path used for file uploads, similar to the other change that was made here.

@lukinovec
Copy link
Collaborator

lukinovec commented Nov 4, 2022

Adding

Route::post('/livewire/upload-file', [FileUploadHandler::class, 'handle'])->name('livewire.upload-file');

to the tenant routes fixed the file uploads (same place where the guide suggests adding the livewire/message/{name} route). It overrides the livewire.upload-file route used by LW. But that breaks the central app. Having InitializeTenancyByPath::class in the middleware_group array in the LW config also breaks the central app.

Also, we should put window.livewire_app_url = '/{{ tenant()->id }}'; inside a check (@if(tenant())) to not break the central app.

@stancl
Copy link
Owner

stancl commented Nov 22, 2022

@lukinovec So is it possible to make LW work in both tenant & central apps with path identification? I think in our convo on BC it seemed that it's not, but here you're suggesting adding an @if() check that might fix that.

@lukinovec
Copy link
Collaborator

here you're suggesting adding an @if() check that might fix that.

That only fixes a smaller problem this PR has (it assumes that tenant() is not null, and that makes visiting pages on the central domain throw an exception), but it doesn't help with the main problem described in the first paragraph of my previous comment. So making LW work in both tenant & central apps with path identification still doesn't seem possible.

@stancl
Copy link
Owner

stancl commented Nov 23, 2022

I wonder if we could make it work by adding something like "query string identification". That wouldn't require re-defining the route since the path is the same, the ID would be passed as a query parameter and used by a middleware.

@stancl
Copy link
Owner

stancl commented Nov 23, 2022

Netlify should be fixed for the next commit

@lukinovec
Copy link
Collaborator

the ID would be passed as a query parameter and used by a middleware

Sounds like that could work, but I'm not sure

@stancl
Copy link
Owner

stancl commented Nov 24, 2022

@lukinovec Can you make a BC task for that?

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