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

Route binding #18

Merged
merged 4 commits into from
Jun 16, 2021
Merged

Route binding #18

merged 4 commits into from
Jun 16, 2021

Conversation

tabuna
Copy link
Member

@tabuna tabuna commented Jun 10, 2021

Make it possible to receive parameters directly in the method:

For example, let's take the following simple procedure, where we pass two values for which we want to get the subtract:

{
    "jsonrpc": "2.0",
    "method": "math@subtract",
    "params": {
        "a": 4,
        "b": 2
    },
    "id": 1
}

In this case, we need to write something like the following in the handler:

public function subtract(Request $request) {
    return $request->get('a') - $request->get('b');
}

It is rather strange when we operate with a small number of parameters and without specifying their type. So let us make it possible to pass these arguments on-demand automatically. That could be written as follows:

public function subtract(int $a, int $b): int
{
    return $a - $b;
}

It will also work for deep things, via camelCase:

{
    "jsonrpc": "2.0",
    "method": "....",
    "params": {
        "user": {
            "name": "Alex"
        }
    },
    "id": 1
}

for argument:

public function handler(string $userName): string
{
    return $userName;
}

Specifying the same names when working with code and documentation

We can use different variable names in the code and expected JSON parameters in the proposed implementation. However, this can lead to confusion over time. For example, one command is called $user_id, and the other $client_id. It seems like an absolutely odd thing that hurts rather than adds value.

Binding declaration

I think we should rely on how laravel does it. Take our substract example:

RPC::bind('a', function () {
    return 100;
});

This will automatically replace the substituted value in our method:

public function subtract(int $a, int $b): int
{
    return $a - $b; // $a = 100
}

But at the same time, the request will contain the original value that was passed.

As for the binding to the models, here I am on the side of the sequence, as I mentioned earlier. However, it is extraordinary that the API would accept different values for the same value in other places. For example, the $user parameter in one place was defined by id, in another by email.

Therefore, you should rely on what is indicated in the model. By default, it looks like this. (Every Eloquent model has this)

/**
 * Retrieve the model for a bound value.
 *
 * @param  mixed  $value
 * @param  string|null  $field
 * @return \Illuminate\Database\Eloquent\Model|null
 */
public function resolveRouteBinding($value, $field = null)
{
    return $this->where($field ?? $this->getRouteKeyName(), $value)->first();
}

If this needs to be changed, and accept more email or something else. You can change this condition by adding ->orWhere. This will be consistent, since in all procedures, the user will be the same, and not depend on how the FormRequest is written:

And this is how we declare the model binding in a straightforward way:

RPC::model('user', User::class);

It will work for anyone where the $user argument is expected, and this value is passed.

As with automatic substitution, bindings RPC::bind and RPC::model can be set for nested elements. To do this, you need to use the dot notation in the declaration. For example:

RPC::model('user.order', ...);
RPC::bind('user.order', ...;

In turn, this will expect camelCase in the arguments:

public function handler($userOrder)

@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #18 (b75a927) into master (ea4f409) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##              master       #18   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       115       120    +5     
===========================================
  Files             20        22    +2     
  Lines            383       403   +20     
===========================================
+ Hits             383       403   +20     
Impacted Files Coverage Δ
src/Binding.php 100.00% <100.00%> (ø)
src/Facades/RPC.php 100.00% <100.00%> (ø)
src/Guide.php 100.00% <100.00%> (ø)
src/HandleProcedure.php 100.00% <100.00%> (ø)
src/ServerServiceProvider.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea4f409...b75a927. Read the comment docs.

@tabuna tabuna merged commit 08938cd into master Jun 16, 2021
@tabuna tabuna deleted the alternative-binding branch June 16, 2021 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant