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

add custom type hint #2

Merged
merged 1 commit into from
May 26, 2021
Merged

add custom type hint #2

merged 1 commit into from
May 26, 2021

Conversation

MarceloSantosCorrea
Copy link
Contributor

If there is another way, I apologize for this PR, but this contribution is to be able to add Model as type hint:
file auto-route.php

'patterns'     => [
    'slug'     => '([\w\-_]+)',
    'uuid'     => '([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})',
    'date'     => '([0-9]{4}-(0[1-9]|1[0-2])-(0[1-9]|[1-2][0-9]|3[0-1]))',
    "customer" => "App\Models\Customer",
],

Captura de Tela 2021-05-23 às 12 54 34

in controller

public function getShow(Customer $customer){}

@izniburak
Copy link
Owner

Hi @MarceloSantosCorrea ,
This is great! We will be able to add model binding functionality for Route parameters thanks to this PR.
Thank you so much for your effort! 💯

I also have a suggestion about that:
We have to define a pattern for Model parameters right now. But actually, we shouldn't need to do this. I think we should handle all parameters related with any class that extends from Model. According to this, we can change your codes like that:

if ($typeHint !== null && class_exists($typeHint)) {
    if (!in_array($typeHint, ['int', 'float', 'string', 'bool']) && !in_array($typeHint, $patterns) 
        && !is_subclass_of($typeHint, 'Illuminate\Database\Eloquent\Model')) {
            continue;
    }
}

What do you think about that?

@MarceloSantosCorrea
Copy link
Contributor Author

I thought about that too, but I wanted to make it broader in case you want to inject other types of classes, by injecting Laravel's dependencies.

@izniburak
Copy link
Owner

When do we use other types of classes for the Route params? As far as I know, for the Route params can be Model or primitives types like int, string, etc. What could we use else?

@MarceloSantosCorrea
Copy link
Contributor Author

Laravel, here is an example.

Another example that can be passed are Interfaces that can be solved with Binding

@izniburak
Copy link
Owner

I know that, but question is can we use Route param binding for all classes/interfaces types? According to documentation of the Laravel Routing, we can solve Model binding because Routing package supports that. But I couldn't find anything else for specific class binding in Router. You can check this documentation.

Could you please share any example for any other class like Service, Library, Helper for solving Route param binding? I'm trying to sure about that.

@MarceloSantosCorrea
Copy link
Contributor Author

'can we use route parameter binding for all types of classes/interfaces?', yes, I already made use of it, in the example I used was to implement the Socialite package, where I pass the network name by string parameter and in the controller it is returned to provider instance.

in routes/web.php

Route::get('login-social/{network}', [\App\Http\Controllers\LoginSocialController::class, 'login']);

in controller

class LoginSocialController extends Controller
{
    public function login(LoginSocialInterface $network)
    {
        return redirect($network->redirect());
    }
}

in App\Providers\AppServiceProvider

public function register()
{
    $this->app->singleton(LoginSocialInterface::class, function (\Illuminate\Foundation\Application $app) {
        $network = $app->router->current()->parameter('network');

        return \Socialite::driver($network);
    });
}

@izniburak
Copy link
Owner

Okay, I understand clearly. So, we can merge the your PR!
Maybe later we can add Model binding support automatically not need to add any pattern. But now, these changes will be great for Auto-Router.

Thank you so much again!

@izniburak izniburak closed this May 26, 2021
@izniburak izniburak reopened this May 26, 2021
@izniburak izniburak merged commit 58835a4 into izniburak:main May 26, 2021
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