Moved Model methods save(), create(), update(), delete(), and refresh() to Models Manager

[SidRoberts]

Continuation of #12317

[sergeyklay]

@SidRoberts What do you think about removing _ prefixes at all in 4.x ?

[SidRoberts]

Convert everything to protected oldSnapshot = [];?

[sergeyklay]

Yes

[SidRoberts]

I like it. I don't think property names should describe their visibility.

[SidRoberts]

Now ready to merge. 😄

I've had to add a few more public functions to ModelInterface but I've documented it in the CHANGELOG. Once the data mapper stuff is complete, we can refactor and possibly remove these methods.

[sergeyklay]

Could you please rebase. See #13027

[SidRoberts]

Update so far:

• Moved Model::save(), Model::create(), Model::update() and Model::delete() to Models Manager.
• Moved Model::refresh() to Models Manager.
• Added ModelInterface::isSkipped().
• Added ModelInterface::clearMessages().
• Added ModelInterface::cancelOperation() (was previously protected in Model).
• Added ModelInterface::setOperationMade().
• Added ModelInterface::exists() (was previously protected in Model).
• Added ModelInterface::updateSnapshot(), ModelInterface::{get,set}UniqueKey(), ModelInterface::{get,set}UniqueParams(), ModelInterface::{get,set}UniqueTypes(), ModelInterface::resetUniqueParams().
• Moved Model::exists() to Models Manager.
• Moved Model::find() and Model::findFirst() to Model Repository.
• Moved Model::count(), Model::minimum(), Model::maximum(), Model::average(), Model::sum() to Models Repository.
• Moved Model::query() to Model Repository.

Model::find(), Model::findFirst() and Model::count() still exist for a few edge cases

The next stages

Remove Model::find(), Model::findFirst() and Model::count()

There are a few remaining tests that require these methods and I haven't had the time to properly fix them yet.

Allow for custom repositories

This will allow us to set custom find() methods and be able to transition the unit tests that override find() and findFirst().

Should we use annotations or a method?

/**
 * @Repository(MyApp\Repositories\RobotsRepository)
 */
class Robots extends Model
{
}

class Robots extends Model
{
    public static function getRepositoryClass() : string
    {
        return MyApp\Repositories\RobotsRepository::class;
    }
}

Remove model namespaces aliases (eg. Music:Artist)

I think we should use class names instead (eg. Artist::class) to reference models.

Remove the ability to assign data in the constructor

Phalcon currently has too many ways to assign data to a model. We need to simplify this as much as possible.

Move column maps to the model repository class

The model class should be as dumb as possible. Doing the will allow us to completely remove all of the methods that get and set properties within a model.

Update documentation on the website

I've already updated the code examples in docblocks to represent the changes.

Final refactor

Self-explanatory. I've moved a lot of stuff around so there'll be some redundant and/or messy code.

---

@sergeyklay I'd appreciate your feedback before I start any of this work.

[sergeyklay]

I know that it's almost impossible - to keep Active Record and Data Mapper. So I will keep this open for a some time to allow @phalcon/core-team a chance to speak up, and to take a fresh look at it later. But at the moment it seems good to me. 👍

Thank you for contributing!

[niden]

I will have a look at this in the morning.

[SidRoberts]

Currently, a Model Repository may look like this:

<?php

use Phalcon\Mvc\Model\Repository;

class RobotsRepository extends Repository
{
    public function initialize()
    {
        $this->setSource("robots2");

        $this->setConnectionService('dbTwo');

        $this->hasMany(
            "id",
            RobotsParts::class,
            "robots_id",
            [
                "foreignKey" => true,
                "reusable"   => false,
                "alias"      => "parts",
            ]
        );

        $this->useDynamicUpdate(true);
    }
}

How about removing the existing getters/setters and separating these into methods?:

<?php

use Phalcon\Mvc\Model\Repository;

class RobotsRepository extends Repository
{
    public function getSource()
    {
        return "robots2";
    }

    public function getConnectionService()
    {
        return 'dbTwo';
    }

    public function isUsingDynamicUpdate()
    {
        return true;
    }

    public function initialize()
    {
        $this->hasMany(
            "id",
            RobotsParts::class,
            "robots_id",
            [
                "foreignKey" => true,
                "reusable"   => false,
                "alias"      => "parts",
            ]
        );
    }
}

I'm not sure what to do about model relations yet though.

Phalcon\Mvc\Model\Repository would have these methods with the default values:

namespace Phalcon\Mvc\Model;

class Repository
{
    public function getSource() -> string
    {
        var entityName, model;

        let entityName = strtolower(this->_modelClass);

        let model = new {modelClass}();

        return uncamelize(get_class_ns(model));
    }

    public function getConnectionService() -> string
    {
        return 'db';
    }

    public function isUsingDynamicUpdate() -> boolean
    {
        return false;
    }
}

[niden]

@SidRoberts

Currently, a Model Repository may look like this: ....

In your example for the current implementation I believe it is fine but we should return $this on those calls to chain them. This will offer nothing in terms of performance but will allow devs to make one block groups (if that makes sense. Something like this:

<?php

use Phalcon\Mvc\Model\Repository;

class RobotsRepository extends Repository
{
    public function initialize()
    {
        $this
            ->setSource("robots2")
            ->setConnectionService('dbTwo')
            ->useDynamicUpdate(true);
            ->hasMany(
                "id",
                RobotsParts::class,
                "robots_id",
                [
                    "foreignKey" => true,
                    "reusable"   => false,
                    "alias"      => "parts",
                ]
            );
    }
}

As for the getters and setters. It is a different implementation that achieves the same thing really. I am more interested in knowing which approach is faster i.e. create private variables and store those values ( _dynamicUpdate = true) vs. having a method that will return a value on the "client" side i.e. implementation. I have a feeling that the second method will be faster because there are no additional variables set in the master model.

Regarding the model relations, I think we can leave them as they are since they are minimalistic and they get the job done. We definitely need to expand in the documentation regarding the parameters that need to be passed or can be passed in the hasMany, hasOne etc. methods.

An approach however would be to create Relationship objects that one could do something like this:

<?php

$relationship = new Relationship();

$relationship
    ->hasMany()
    ->sourceId('id') // We can hardcode 'id' to allow less code for common ids
    ->target(RobotsParts::class)
    ->targetId('robots_id')
    ->setForeignKey()
    ->isReusable()
    ->setAlias('parts');

// In the model:

$this
    ->setSource("robots2")
    ->setConnectionService('dbTwo')
    ->useDynamicUpdate(true);
    ->addRelationship($relationship);

I think the above example is nice clean and very OO but what would the cost be in performance. Personally I would rather keep things fast than "nice" if that makes sense.

Thoughts?

[SidRoberts]

My reasoning for replacing the getters and setters is to reduce the complexity of the Phalcon code - not so much the experience. 😛

I'll look into the relationships tonight. I've already added a helper in Models Manager to reduce some of the code we have and to simplify adding the new style if we make it.

[Jurigag]

I still think that we need to move all protected Model::_method from current phalcon 3 to separated class, like Worker or something like this, not sure about name, not keep it in Model or Manager.

About custom repos - if they are using annotation adapter for metadata - allow annotation, if not - then method.

Model namespace aliases - no no and no about removing them, this is very nice feature, and imho better than Article::class. But i guess it's only about preference, i use it many time personally, mostly in multi module setup.

[Jurigag]

Can you add 4.0.x milestone? Personally i don't like writing methods like this - like use some method chain in part of code - less methods calls - the better. Method call is heavy in php. It's certainly faster to have getSource in model/repository with return "robots2" than setSource("robots2") beacause we will need to call this getSource anyway.

Also i should model relations should sit in model as they are, so there should still be initialize() method in model, and things like source etc should stay in model.

Repository class should be only for finding stuff, no relations, sources, etc things.

Moved `Model::find()` and `Model::findFirst()` to Model Repository.
Moved `Model::count()`, `Model::minimum()`, `Model::maximum()`, `Model::average()`, `Model::sum()` to Models Repository.
Moved `Model::query()` to Model Repository.

Like only this should be in repository, and it shouldn't do anything more really. Setting source in repository or relations in it seems weird.

And like i already mentioned, now we are changing pretty much Manager class to kind of god-class. Like moving code from Model to Manager and we will have huge manager class. Like i already said - we need to introduce new class which will do all this complex stuff and will have logic for it.

I would move those - _preSave,_checkForeignKeysRestrict,_preSaveRelatedRecords,_postSaveRelatedRecords,_doLowInsert,_doLowUpdate,_checkForeignKeysReverseRestrict,_checkForeignKeysReverseCascade to separated class to reduce complexity of Manager class.

Also why Repository interface has __construct? Interface should never have it.

[SidRoberts]

I've removed the constructor from RepositoryInterface.

For annotations, how about creating something like an AnnotationsRepository class that is able to grab the annotations from the model?:

<?php

namespace Phalcon\Test\Models\Annotations;

use Phalcon\Mvc\Model;

/**
 * @Source("robots")
 * @Schema("another_database")
 * @UseDynamicUpdate(true)
 */
class Robot extends Model
{
    /**
     * @Primary
     * @Identity
     * @Column(type="biginteger")
     */
    protected $id;

    /**
     * @Column(type="varchar", length="70", allow_empty_string=true)
     */
    protected $name;

    /**
     * @Column(type="varchar", length="32", default="mechanical")
     */
    protected $type;

    public static function getRepositoryClass()
    {
        return \Phalcon\Mvc\Model\Repository\Annotations::class;
    }
}

One of the problems I personally have with Phalcon is that there are 5 different ways of accomplishing the same task. It makes it difficult to debug and the internal code is more complicated than it needs to be. The whole point of model aliases was so that you didn't have to write the fully-qualified class name in a query. Regardless of how it was written before, automatic refactoring was difficult because it wasn't able to catch strings in PHQL. If we use the actual aliases in PHP, we still get the benefits of before, the Phalcon code is simpler and refactoring is made easier.

This is still a work in progress but once it is complete, the Model class should be a dumb object. It shouldn't have any knowledge of the Models Manager or the Repositories at all. In fact, it should be something equivalent to the Controller class. Relations need to be moved somewhere but I haven't worked out relations in my head yet.

A Unit Of Work class is planned but I haven't worked out how it will work alongside Transactions yet. For that matter, Repository and models metadata seem to have a lot of overlap and if we use the annotations system above, the models metadata classes should also be overhauled and annotations metadata class removed.

Everything that has been done so far should be viewed as temporary and a work in progress.

[niden]

@SidRoberts I love it so far. I like the direction that this is taking.

• For annotations I agree. An AnnotationsRepository class should do the trick.
• For relations, we will need to move it in its own "space" so to speak that is injectable to the models manager for said model.
• The Tag component that is used here and there, there is another PR to change it from full static methods to non static ones (just a reminder)

Ping me via email or Discord if you want to discuss this further

+1

[david-duncan]

Where is the RFC for this? I see excellent questions raised through this MR and the previous MR that have been ignored. Am I to understand that 4.0 is a complete ORM migration?

[Jurigag]

@SidRoberts

Will you continue this or someone else should?

[SidRoberts]

Someone else can take this over.

…

On Wed, 13 Jun 2018 at 23:23, Wojciech Ślawski ***@***.***> wrote: @SidRoberts <https://github.com/SidRoberts> Will you continue this or someone else should? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13022 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABTQ9uc8ibGVOAQXLNCPWW5n3BBDj3-vks5t8SBYgaJpZM4O5jPZ> .

[ghost]

I would like to ask the phalcon team, will this really merge?

For example, if you retrieve data with Model :: find() or Model :: findFirst(), code completion of property (= column) will work by IDE.

If you transfer all methods to the model manager, such completion will not work. This is a very bad effect on development efficiency.

I think that it is better to withdraw from the 4.0 milestone once and discuss with users with the phalcon team.
How about everyone?

[niden]

This is a significant amount of work and I don't want us to lose it. It moves the framework in the right direction IMO.

However, after putting quite a bit of thought into this, we cannot immediately introduce this into the v4 version as is because it will pretty much force developers to rewrite their applications (at least at the model level) and that is not what I want to do.

We will keep this here and check on whether we can introduce the functionality of this PR while maintaining some good amount of backwards compatibility.

[ghost]

Thank you for your reply.
I'm relieved to hear that.

[niden]

Closing this one: Related: https://github.com/phalcon/cphalcon/issues/14126