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

404 | NOT FOUND #321

Closed
CyberEkklesiaOwner opened this issue Oct 13, 2022 · 11 comments
Closed

404 | NOT FOUND #321

CyberEkklesiaOwner opened this issue Oct 13, 2022 · 11 comments

Comments

@CyberEkklesiaOwner
Copy link

Hello Rick.

The short story:
I'm having issues with your system when integrated with Wave SAAS (https://github.com/thedevdojo/wave). Would you be able to help?

A bit more info:
Devdojo is kindly working with me quite some time after I integrated laravel-forum. But it's getting kind of difficult now since seems that any possible solution might be done at laravel-forum. Here is the post related to this issue: https://devdojo.com/question/404-not-found

Would you be able to do the same effort? I mean, install their software and then your package. If you kindly be able to do so, then here is my installation process:

  1. Install Wave SAAS
  • https://wave.devdojo.com/docs/installation
  • If you get redis error, try composer update --ignore-platform-req=ext-redis
  • If you get migration error copy the below code at config/database php, Ln: 60 (mysql engine)
    --> 'engine' => 'InnoDB ROW_FORMAT=DYNAMIC',
  1. Proof that Wave Blog works before laravel-forum.
  • Visit: Wave Blog link, click any article. You should be able to read any article. Meaning, that there no error.
  1. Install laravel-forum 5.0
    --> You might want to ignore redis if you had trouble before.
    --> composer require riari/laravel-forum:~5.0 --ignore-platform-req=ext-redis

  2. Re-visit Wave Blog Page and click any article. You should get an 404 | NOT FOUND error.

Thank you in advance

@Riari
Copy link
Collaborator

Riari commented Oct 13, 2022

Hi,

I can immediately see what the problem is. Unfortunately, the route model binding feature in Laravel doesn't provide a way of scoping binding logic to specific route groups or prefixes. This is actually a problem for the package itself too, because both the API and web routes use the same set of route model parameters with slightly different logic - so there's a conflict between the bindings for each.

I was considering creating a feature PR for Laravel to introduce some kind of scoping capability to route bindings for this reason, but a simpler solution for now would be to rename the route parameters to something that's more unique to the package. I'll probably prefix them with lf_ or something.

I'll work on a new release with that fix tonight and let you know when it's done. There should be no conflicts with Wave after that.

@CyberEkklesiaOwner
Copy link
Author

CyberEkklesiaOwner commented Oct 13, 2022 via email

@Riari
Copy link
Collaborator

Riari commented Oct 13, 2022

Actually, before I rename the parameters, could you try something for me?

In ForumServiceProvider.php, move $this->registerWebRouteBindings(); (line 62) into the route group where the web.php routes file is loaded. It should look like this:

            $router->group(config('forum.web.router'), function () {
                $this->loadRoutesFrom(__DIR__.'/../routes/web.php');
                $this->registerWebRouteBindings();
            });

For convenience, I would temporarily make this change directly in src/vendor/riari/laravel-forum/src/ForumServiceProvider.php if you can.

The Laravel docs aren't clear on this, but by doing the bindings in the topmost group, it might scope them to the package routes. I just want to verify the following:

  • It doesn't break route caching, i.e. the other issues you reported before are still fixed.
  • It solves the conflict with the Wave category parameter.

If you can confirm it works, I'll do that instead of renaming the parameters as it's a better solution.

@CyberEkklesiaOwner
Copy link
Author

CyberEkklesiaOwner commented Oct 13, 2022 via email

@CyberEkklesiaOwner
Copy link
Author

CyberEkklesiaOwner commented Oct 14, 2022 via email

@Riari
Copy link
Collaborator

Riari commented Oct 14, 2022

No problem, thanks for trying anyway! I'll work on the other solution tonight (UK time).

@Riari
Copy link
Collaborator

Riari commented Oct 14, 2022

Hi,

Just to keep you in the loop, I've encountered a drawback with the parameter rename solution.

I was going to use the prefix forum_api_ for the API route parameters and forum_web_ for the web ones. However, some of the form requests in this package use the route('param') method to retrieve models from route parameters resolved via the bindings. These form requests are shared by both the API and web controllers, so there's no way for them to know which parameter name to use (e.g. forum_api_category or forum_web_category).

At this point, I think route model binding is causing more issues than it's worth, so I'm going to need more time to work on an update that moves the resolution logic out of the Route::bind calls and into some other place that only takes effect for the package routes (maybe in middleware).

Sorry I couldn't get a fix out today - I'll reply here again when I've figured something out :)

@CyberEkklesiaOwner
Copy link
Author

CyberEkklesiaOwner commented Oct 14, 2022 via email

@Riari
Copy link
Collaborator

Riari commented Oct 15, 2022

OK, I've released a fix for this. It moves the binding logic into middleware. Works with the router cache, prevents conflicts, and the parameter names don't need to change :)

See release 5.3.5.

@Riari Riari closed this as completed Oct 15, 2022
@CyberEkklesiaOwner
Copy link
Author

CyberEkklesiaOwner commented Oct 17, 2022

I'm writing to confirm that the fix worked like a charm! Thank you!!!

I let Devdojo know that your package is fully compatible. Great job!

@Riari
Copy link
Collaborator

Riari commented Oct 17, 2022

Great! Glad I was able to help.

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

No branches or pull requests

2 participants