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

[1.x] Adds name function #79

Merged
merged 19 commits into from
Aug 15, 2023
Merged

[1.x] Adds name function #79

merged 19 commits into from
Aug 15, 2023

Conversation

nunomaduro
Copy link
Member

@nunomaduro nunomaduro commented Aug 9, 2023

This pull request fixes #48, and it introduces a new name function to Laravel Folio - that allows to basically define a route name for a specific page, and access any Folio's route using the regular route helper. Here is an example:

<?php // pages/dashboard.blade.php

use function Laravel\Folio\{name};

name('dashboard'); // route name defined here...

?>

<x-app>
    <nav>
        <a href="{{ route('dashboard') }}">Dashboard</a> <!-- Route name used here ... -->
    </nav>

    <main>
        <h1>My Dashboard...</h1>
...

And, of course, everything you would expect regarding arguments just works with this function, assuming that these files contain name('users.show') in their content. Here is how the route helper can be used:

- users/[id].blade.php:  route('users.show', ['id' => 1]); // users/1
- users/[user].blade.php: route('users.show', ['user' => User::first()]); // users/1
- users/[user-email].blade.php: route('users.show', ['user' => User::first()]); // users/nuno@laravel.com
- users/[...user].blade.php: route('users.show', ['users' => User::all()]);  // users/1/2/3...

// Note: that the plural `users` was used for this last one...

In addition, in production, when developers use route:cache, Folio will automatically cache all the routes names defined using the name function, avoiding any performance issues. The "cached" file is stored at bootstrap/cache/folio-routes.php.

Finally, if a route is missing parameters or if a parameter contains the null value, a Laravel\Folio\Exceptions\UrlGenerationException is thrown, similar to Illuminate\Routing\Exceptions\UrlGenerationException.

This was referenced Aug 9, 2023
@snellingio
Copy link

snellingio commented Aug 9, 2023

Fantastic work! The use of a name function is spot on.

However, there's one thing we might need to reconsider - the continued use of the Route::fallback method in the 'FolioManager' class.

To get the name, we have to navigate through the directory to run the InlineMetadataInterceptor. But, Laravel doesn't seem to support multiple Route::fallback's. This means that once Folio is installed, 'Route::fallback' may not work properly in your routes/web.php.

From what I understand, the original code was designed to avoid having to go through the directory for each request, and to bypass the need to cache the routes in the same way, leading to the creation of the folio:list command.

But now, we're navigating through the directory again! This raises a question - should we really continue using the Route::fallback method? Do Folio routes still need to be separated, or can they be registered and cached like normal? It might be worth discussing alternatives as this ends up using both traversing the directory, and using the fallback.

@mokhosh
Copy link

mokhosh commented Aug 9, 2023

I'm repeating this here since you said let's continue the discussion here.

Can we generate route names based on a convention if no route name is explicitly declared?

@nunomaduro nunomaduro marked this pull request as ready for review August 10, 2023 13:02
@nunomaduro nunomaduro marked this pull request as draft August 10, 2023 13:09
@nunomaduro nunomaduro marked this pull request as ready for review August 10, 2023 13:20
@taylorotwell
Copy link
Member

taylorotwell commented Aug 10, 2023

@mokhosh anything that generates route names by conventions is inherently tied to the file structure in some way - which defeats the purpose of named routes entirely since the route names would change when you move / rename files.

@taylorotwell
Copy link
Member

@snellingio I agree that in a perfect world Folio would likely register an individual, matching route for each page in each mounted directory. However, careful consideration would have to be given to the order they are registered and the process would be a bit slow in local development.

You are correct that if you are using Folio you can no longer use Route::fallback in your own application. However I think we could offer the appropriate hooks to allow you to still define your own fallback behavior.

@mokhosh
Copy link

mokhosh commented Aug 10, 2023

@mokhosh anything that generates route names by conventions is inherently tied to the file structure in some way - which defeats the purpose of named routes entirely since the route names would change when you move / rename files.

Good point, but i don't think it's a problem we can't solve.

We can have a folio:mv artisan command that puts the explicit named route before moving if it's not there.

@nunomaduro nunomaduro marked this pull request as draft August 14, 2023 22:32
@nunomaduro nunomaduro marked this pull request as ready for review August 15, 2023 13:56
@nunomaduro nunomaduro merged commit ffb7777 into master Aug 15, 2023
6 checks passed
@nunomaduro nunomaduro deleted the feat/name branch August 15, 2023 13:56
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.

Register named routes
4 participants