Route Key Trait to use Optimus in models

[ivanvermeyen]

Hello,

I'm playing around with various Optimus packages for Laravel at the moment.
I really like how yours is set up, with the different connections etc. and your code looks very clean!
I love it :D

I was looking for the easiest way to get the encoded ID's into my models, without having to duplicate route bindings and all. This is what I ended up with... Maybe this can go into a Trait that can be part of your package? What do you think?

/**
 * Get the value of the model's route key.
 *
 * @return mixed
 */
public function getRouteKey()
{
    $id = parent::getRouteKey();

    return $this->getOptimus()->encode($id);
}

/**
 * Retrieve the model for a bound value.
 *
 * @param mixed $value
 *
 * @return \Illuminate\Database\Eloquent\Model|null
 */
public function resolveRouteBinding($value)
{
    $id = $this->getOptimus()->decode($value);

    return $this->where($this->getRouteKeyName(), $id)->first();
}

/**
 * Get the Optimus instance.
 *
 * @return \Cog\Laravel\Optimus\OptimusManager
 */
protected function getOptimus()
{
    $connection = null;

    if (property_exists($this, 'optimusConnection')) {
        $connection = $this->optimusConnection;
    }

    return \Cog\Laravel\Optimus\Facades\Optimus::connection($connection);
}

People configure the primes in your config file, and they can define a connection name on a specific model by setting a $optimusConnection property. If they don't, the default connection is used.

[antonkomarev]

Hello, @ivanvermeyen! Thank you for kind words.

I have thought about route binding but haven't dive deep into it because not using it in any project at this moment. I like an idea but need time to think about best way to implement it. Was planned to look in it in next 2 months. By the way we are always open for Pull Requests.

You could make src/Traits directory and place desired trait there. Do you have suggestions how to name it?

Note: It would be great to have tests for this functionality.

[ivanvermeyen]

Naming things is usually the hardest part :)
I extracted a trait:

use HasEncodedRouteKey:

But if it's part of your package, maybe you want something more "branded'?

use OptimusEncodedRouteKey;

Or just:

use OptimusRouteKey;

Or maybe something else? :)

I'd be happy to see if I can fix some tests for you.
I'll have to figure out the best/cleanest way to do this, as it will require a route + database query to really verify that it works I guess. But I might have an idea. Feel free to discard it if you don't like it though :)

[ivanvermeyen]

Can you take a look here:

https://github.com/ivanvermeyen/laravel-optimus/commit/73842b0523ef4e50974382add4d5441b6a253bd8

I'm testing the 2 methods that I override from the base model. I also do a GET request that triggers implicit route model binding to the User stub.

That dummy User class is added below the test class, as I didn't know where you would prefer to keep this. I could as well save it in a separate file somewhere?

Let me know if should change anything before I do a pull request.

[antonkomarev]

Thank you for your participation, Ivan!

First of all I want to say that your code looks good to me 👍

I'm totally agree that naming is one of the hardest parts in software development.

OptimusEncodedRouteKey is a good name because it's explicitly says that route key is encoded and specified that it's encoded using Optimus.

$this->loadLaravelMigrations(['--database' => 'testing']);

Is there any reason why you don't want to use default :memory: database? It's quicker and don't require any additional configuration.
Personally I'm always prefer to keep all test environment settings in PHPUnit config files. Default test settings are stored in phpunit.xml.dist file. And phpunit.xml file is overwriting config. This file ignored by Git what allows developer to easily switch to own environment settings. In other words when you need to change unit tests configuration you just copying phpunit.xml.dist to phpunit.xml and modify the last one for your personal or Continuous Integrations tools requirements on the fly.

Additionally it would be better to extract User model from tests:

class User extends Model
{
    use OptimusRouteKey;

    protected $guarded = [];
}

Usually I'm placing it here: tests/Stubs/Models/User.php.

• Keep consistency 1 class = 1 file.
• Allows to use same User model in other parts of the unit tests.

[ivanvermeyen]

Testbench provides this method to run the migrations that are included with Laravel out of the box (such as the users table). Then we don't need to make our own migrations just for this test.

$this->loadLaravelMigrations(['--database' => 'testing']);

But as you say this forces a testing database.
I was able to change it to this though:

$this->loadLaravelMigrations(config('database.default'));

Which then corresponds to what is defined in PHPUnit's config (DB_DATABASE) and the config in one of the base test classes: (I'm kinda wondering which config is used)

// \GrahamCampbell\TestBench\AbstractPackageTestCase
$app->config->set('database.default', 'sqlite');
$app->config->set('database.connections.sqlite', [
    'driver'   => 'sqlite',
    'database' => ':memory:',
    'prefix'   => '',
]);

I will update the traits name and move the User stub to its own file 👍

[ivanvermeyen]

One more thing, I'm not sure what to do with this:

To override the Optimus connection in a model, I first wanted to let people add a protected property $optimusConnection to their model and then check if the property exists in getOptimus().

However, then I couldn't easily change it in the tests (unless I created a second User stub), so I added the property and a setOptimusConnection() method to the trait.

But since trait properties can not be overridden, people then need to call setOptimusConnection() in their model's boot method. Which feels less attractive?

I thought adding public properties to a model used to have side effects (Laravel trying to map it to a database) but my earlier tests still passed... So I might be confusing this with something else...

Maybe I should revert to checking if a property exists on the model, and for the tests add a public property to the User stub, so I can directly change it?

How would you go about this?

Edit:

Like this: https://github.com/ivanvermeyen/laravel-optimus/commit/fcc16d5a8d29005405e9bfcf924faa7212ec4e6a

[antonkomarev]

If talk about database connection settings it will be enough to rely on \GrahamCampbell\TestBench\AbstractPackageTestCase for now. If there will be need to have ability to configure it via phpunit.xml file - it will be another one PR. What I've wrote in my previous comment is a recommendation, but not strict requirement.

I agree that calling setOptimusConnection is less attractive and adds complexity to application models. It would be better to revert code to your previous solution and create extra stub.

For example it could be 2 stubs:

• UserWithDefaultOptimusConnection
• UserWithCustomOptimusConnection

This class naming provided as example, you are free to name them as you wish. From my point of view since it's tests so we wouldn't worry about long names. Expressive names would simplify understanding what model do we have.

[antonkomarev]

It's too bad that PHP restricts trait properties overwrite :( The worst part of it - that they could be overwritten in extended model. Such inconsistency hurts my mind.

trait TestTrait
{
    public $test = 'test';
}

class Parent
{
    use TestTrait;
}

class Child extends Parent
{
    public $test = 'demo';
}

$child = (new Child)->test; // demo

By the way this trick is useless for current case.

[ivanvermeyen]

Indeed. Perhaps one day... :)

Should I do a pull request with the latest changes, or is there anything else I should change?

Maybe add this feature to the readme somewhere?

---

Woops, I overlooked your previous reply, will make a second User model for the property test! :)

[antonkomarev]

You could add PR and continue shaping it there. Adding this feature to readme is a good thing for sure. And feel free to add yourself to Credits section 😉

[ivanvermeyen]

I've added howto's to the readme.
Feel free to filter if it's too much info :)

I also added another test to make sure that route('named.route', [$model]) returns the URL with encoded ID.