Move model to a trait #2580

The properties $primaryKey and $keyType can't be in the trait because PHP forbid declaring a property in a trait that is different (type and value) than the property declared in the parent class.

For Laravel's User model:

namespace App\Models;
use Illuminate\Foundation\Auth\User as Authenticatable;
use MongoDB\Laravel\Eloquent\DocumentModel;
class User extends Authenticatable
{
 protected $connection = 'mongodb';
 protected $collection = 'users';
 protected $primaryKey = '_id';
 protected $keyType = 'string';
}

For Laravel Sanctum (with default model override):

namespace App\Models;
use Laravel\Sanctum\PersonalAccessToken as SanctumToken;
use MongoDB\Laravel\Eloquent\DocumentModel;
class PersonalAccessToken extends SanctumToken
{
 use DocumentModel;
 protected $connection = 'mongodb';
 protected $collection = 'personal_access_tokens';
 protected $primaryKey = '_id';
 protected $keyType = 'string';
}

(削除) I have mixed feelings about this change, as it breaks the link with the overloaded parent methods. It would probably be better to make the change in each community-package when necessary. (削除ここまで)

@GromNaN

Copy link

Member Author

GromNaN commented Aug 29, 2023

Currently, the Model class is a marker for MongoDB models. If a trait is used, an other marker will be necessary.
https://github.com/mongodb-php/laravel-mongodb/blob/f06f7b3dca88cfd5cc6a823ff01051b4a46ca10a/src/Eloquent/HybridRelations.php#L145

@GromNaN GromNaN mentioned this pull request

Jun 3, 2024

[Feature Request] SANCTUM SUPPORT #2991

Closed

@GromNaN GromNaN force-pushed the model-trait branch from a4fb1d3 to 142396f Compare

July 4, 2024 18:42

github-advanced-security[bot]

github-advanced-security bot found potential problems

Jul 4, 2024

View reviewed changes

src/Eloquent/Model.php Fixed Show fixed Hide fixed

@GromNaN GromNaN changed the base branch from 4.1 to 4.5

July 4, 2024 18:52

@GromNaN GromNaN force-pushed the model-trait branch 3 times, most recently from 5d0d693 to 2163745 Compare

July 5, 2024 12:36

@GromNaN


 Create DocumentModel trait to enable MongoDB on any 3rd party model c...

855ff8e

...lass

@GromNaN GromNaN force-pushed the model-trait branch from 2163745 to 855ff8e Compare

July 5, 2024 12:38

@GromNaN GromNaN marked this pull request as ready for review

July 5, 2024 12:39

@GromNaN GromNaN requested a review from a team as a code owner

July 5, 2024 12:39

@GromNaN GromNaN requested a review from alcaeus

July 5, 2024 12:39

@GromNaN


 Use the trait for every test model

c8d6c42

GromNaN

GromNaN commented

Jul 5, 2024

View reviewed changes

Copy link

Member Author

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trait DocumentModel is extracted from the MongoDB\Laravel\Eloquent\Model class, the only changes are commented.

src/Eloquent/Model.php

* @var string

protected $keyType = 'string';

Copy link

Member Author

@GromNaN GromNaN Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the trait is used, the properties $keyType and $primaryKey must be defined, because they can't be redefined by the trait.

src/Eloquent/DocumentModel.php

/** @inheritdoc */

public function getTable()

{

return $this->collection ?? parent::getTable();

Copy link

Member Author

@GromNaN GromNaN Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is not defined in the trait, otherwise it's allowed to redefine in the class that uses the trait.

src/Eloquent/DocumentModel.php

/**

* The parent relation instance.

private Relation $parentRelation;

Copy link

Member Author

@GromNaN GromNaN Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the visibility to private. There is a getter and an setter for this method. It doesn't need to be protected. Since it's a trait, it can still be accessed directly in a class that uses the trait.

src/Eloquent/Model.php Outdated

* @param class-string|object $related

public function unset($columns)

public staticfunction isDocumentModel(string|object$related): bool

Copy link

Member Author

@GromNaN GromNaN Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method replaces $class instanceof MongoDB\Laravel\Eloquent\Model to check if the model class is for a MongoDB connection. It is public and can be used in projects.

tests/Models/User.php

class User extends Eloquent implements AuthenticatableContract, CanResetPasswordContract

class User extends Model implements AuthenticatableContract, CanResetPasswordContract

{

use DocumentModel;

Copy link

Member Author

@GromNaN GromNaN Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the test models have been updated to use the trait, so that we are sure all the features works without extending MongoDB\Laravel\Eloquent\Model.

Copy link

Member

@alcaeus alcaeus Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having one test with a model that extends our document model class instead of using the trait will help us prevent regressions in our model class.

@GromNaN


 Update changelog

ef12043

@GromNaN GromNaN added the feature label

Jul 5, 2024

alcaeus

alcaeus reviewed

Jul 8, 2024

View reviewed changes

src/Eloquent/Model.php Outdated

return $this;

return is_subclass_of($related, BaseModel::class)

&& array_key_exists(DocumentModel::class, class_uses_recursive($related));

Copy link

Member

@alcaeus alcaeus Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class_uses_recursive looks like it has significant performance implications. Using a marker interface and checking using is_subclass_of would probably be more efficient.

Alternatively, checking whether the class is a subclass of this model class first, and only checking for traits in other cases would probably solve the performance issue for most cases.

Copy link

Member Author

@GromNaN GromNaN Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a shortcut to check our Model class + a memory cache for when the same class is checked twice in the same request.

@GromNaN


 Optimize Model::isDocumentModel

114f918

@GromNaN GromNaN requested a review from alcaeus

July 8, 2024 12:10

@GromNaN


 Refactor the User class to extend Laravel's User class

16ff436

GromNaN

GromNaN commented

Jul 8, 2024

View reviewed changes

tests/Models/Casting.php

use MongoDB\Laravel\Eloquent\Model;

class Casting extends Eloquent

class Casting extends Model

Copy link

Member Author

@GromNaN GromNaN Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class continues to extend MongoDB\Laravel\Eloquent\Model

alcaeus

alcaeus approved these changes

Jul 8, 2024

View reviewed changes

@GromNaN


 Model::isDocumentModel don't need to be extensible

8e24ed7

@GromNaN GromNaN force-pushed the model-trait branch from 51d8795 to 8e24ed7 Compare

July 8, 2024 14:56

@GromNaN GromNaN merged commit 89463b0 into mongodb:4.6

Jul 8, 2024

@GromNaN GromNaN deleted the model-trait branch

July 8, 2024 14:59

@masterbater

Copy link

Contributor

masterbater commented Jul 8, 2024

@GromNaN Hopefully in documentation can add a guide for third party packages like spatie's laravel-permissions, bouncer, tenancy etc, sanctum, etc. Or create a community for laravel packages being modified to work with laravel mongodb. This way all popular packages will have counterpart that is made to work with mongodb thanks.

@GromNaN

Copy link

Member Author

GromNaN commented Jul 8, 2024

@masterbater The documentation is going to be written by MongoDB's doc team.

Labels

feature

3 participants

@GromNaN @masterbater @alcaeus

Move model to a trait #2580

Move model to a trait #2580

Uh oh!

Conversation

@GromNaN GromNaN commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GromNaN commented Aug 29, 2023

Uh oh!

Uh oh!

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

Uh oh!

@GromNaN GromNaN Jul 5, 2024

Choose a reason for hiding this comment

Uh oh!

@GromNaN GromNaN Jul 5, 2024

Choose a reason for hiding this comment

Uh oh!

@GromNaN GromNaN Jul 5, 2024

Choose a reason for hiding this comment

Uh oh!

@GromNaN GromNaN Jul 5, 2024

Choose a reason for hiding this comment

Uh oh!

@GromNaN GromNaN Jul 5, 2024

Choose a reason for hiding this comment

Uh oh!

@alcaeus alcaeus Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

@alcaeus alcaeus Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

@GromNaN GromNaN Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

@GromNaN GromNaN Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

masterbater commented Jul 8, 2024

Uh oh!

GromNaN commented Jul 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

@GromNaN GromNaN commented Aug 28, 2023 •

edited

Loading