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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Converting Active Record to the Data Mapper pattern #12317

Merged
merged 3 commits into from Aug 16, 2017

Conversation

@SidRoberts
Copy link
Member

SidRoberts commented Oct 14, 2016

So I thought I'd have a go at converting Phalcon's Active Record models to the Data Mapper pattern (see Doctrine) and I want to hear people's thoughts and opinions before I take it any further.

I've avoided doing to much work just in case it ends up being a dead-end so no code has been refactored and methods in the models manager now call protected methods in Phalcon\Mvc\Model, etc; - it's a bit of a mess. Obviously this will/should all change.

Put succinctly, methods have been moved to the models manager and (currently) operate exactly as they did before:

// Before
$person = People::findFirst($params);

// After
$person = $modelsManager->findFirst(
    People::class,
    $params
);
// Before
$person = People::count($params);

// After
$person = $modelsManager->count(
    People::class,
    $params
);
// Before
$person->delete();

// After
$modelsManager->delete($person);
// Before
$person->save($params);

// After
$modelsManager->save($person, $params);

Phalcon\Mvc\Model is still far too complicated, in my opinion, so there's still a lot of work to do. 馃槢

A few things that I'd like to do is remove the ability to assign properties during create/update/save:

$robot->save(
    [
        "name" => "Bender",
    ]
);

$modelsManager->save(
    $robot,
    [
        "name" => "Bender",
    ]
);

I know people like it because it acts a little shortcut but it doesn't make sense in the data mapper context and looks a little clumsy. A method called save() should save and do nothing else.

I'd also like to decide between only supporting public properties or getters/setters but have no personal preference of either. Alternatively (or additionally), make an effort to vastly simplify how properties are get/set internally.

(It goes without saying but is obviously aimed at version 4 - not version 3)

Thoughts?

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Oct 14, 2016

Introduce please repository class imho as well, because using modelsManager is dirty. So we can still jus t do $personRepostory->find(); for example. Like some abstract class:

abstract class ModelRepository
{
 // all find methods, as well findFirstById, and findFirstBy*, etc, just all finds which are currently in model, as well sum, count etc things
    abstract public funciton setSource(); // which will return Person::class for example in PersonRepository, this will be used for all selections
}

Or eventually this should be used in model like $this->setRepository(new PersonRepository()) in initialize method, and then we can do something like $modelsManager->getRepository(Person::class)->find(); With option to use some alias so we don't need to use Person::class we could just use for example M:Person.

And then totally remove this _invokeFinder imho and only repostiories for selection.

Also we should aim for totally removing Phalcon\Mvc\Model, eventually it can stay there and give ability to like assign, validation and other stuff but extending should not be needed for working with models manager and data mapper.

Also repository classes will be good one place for adding all custom related model find methods, so somewhere in code instead of executing some extensive find or query we can just call proper method - this is pretty main reason im asking for it, just built-in repository class with some generic methods in it + option to add our own.

Don't know which way is better, i guess second.

Also this change is of course for 4.0.0. I thought @andresgutierrez was planning this for 4.0.0

I would need to rewrite whole app then because im using everywhere $model->save($params, $whiteList) because it's really super great.

@sergeyklay sergeyklay added this to the 4.0.0 milestone Oct 14, 2016

@SidRoberts SidRoberts force-pushed the SidRoberts:v4-model-data-mapper branch from ac8916d to ddc3d1f Oct 14, 2016

@sergeyklay
Copy link
Member

sergeyklay left a comment

Looks great to me





This comment has been minimized.

@Green-Cat

Green-Cat Oct 14, 2016

Contributor

Too much spacing.

This comment has been minimized.

@SidRoberts

SidRoberts Oct 14, 2016

Author Member

I was going for speed. 馃槈

This comment has been minimized.

@Jurigag

Jurigag Oct 14, 2016

Member

No need for speed, it will be at least few months until we create 4.0.x repo i guess, and few years to release :D

This comment has been minimized.

@SidRoberts

SidRoberts Oct 14, 2016

Author Member

Fixed in 77acbfe.



/**
* Allows to query the first record that match the specified conditions

This comment has been minimized.

@Green-Cat

Green-Cat Oct 14, 2016

Contributor

Allows to query for the first record that matches the specified conditions

This comment has been minimized.

@SidRoberts

SidRoberts Oct 14, 2016

Author Member

Fixed in 77acbfe.

@SidRoberts

This comment has been minimized.

Copy link
Member Author

SidRoberts commented Oct 14, 2016

@Jurigag, it'd be tedious to have to create repository classes for each model. We could set the model class name in the constructor though:

namespace Phalcon\Mvc\Model;

class Repository
{
    public function __construct(string! modelClass);

    public function find(array params = []) -> <ResultsetInterface>;

    public function findFirst(array params = []) -> <ModelInterface>;

    public function count(array parameters = []) -> int;

    public function sum(array parameters = []);

    public function maximum(array parameters = []);

    public function minimum(array parameters = []);

    public function average(array parameters = []);
}

It would reduce the complexity of the Models Manager a bit but would repositories add complexity for users/developers?:

$robots = $modelsManager->findFirst(
    Robots::class,
    $params
);

// OR ...

$robotsRepository = $modelsManager->getRepository(
    Robots::class
);

$robots = $robotsRepository->findFirst($params);
@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Oct 14, 2016

I mean if you don't want - you don't need to create repository. You can use as well just $modelsManager->findFirst(Robots::class, $params) i guess maybe. Repository will be just some kind of shorthand and some place where you can add addtional methods for finding stuff. I know developer can always create his own class but in most Data Mappers there is Repository class introduced.

By using constructor it's fine. But what if we want to extend phalcon repository and use our own ? Then we need to point it somewhere, in model or in modelsmanager. SO:

We don't need to create repositories for each model and generic internal Repository will be used for each model. But still we will have in model something like $this->setRepository(new PersonRepository(self::class)) and this repository will be used.

Or eventually something like $modelsManager->registerRepository(new PersonRepository(Person::class))

I think second will be better so we can like remove totally need for people to extend Phalcon\Mvc\Model

So in total - i think there should be Repository class which will be used always. No matter if we create it or not. Just phalcon will create proper repository object if none provided by us.

I know that this can mean that there could be possible Phalcon\Mvc\Model\Repository for 10 models in some cases created.

Also models manager is already really complex enough. So introducing Repository will be better i think.

@Jurigag
Copy link
Member

Jurigag left a comment

Phalcon/Mvc/Model/Repository class, add option to use custom Repository class for models.

*/
let related = model->_related;
if typeof related == "array" {
if model->_preSaveRelatedRecords(writeConnection, related) === false {

This comment has been minimized.

@Jurigag

Jurigag Oct 14, 2016

Member

There should be something like Phalcon\Mvc\Model\Worker or Phalcon\Mvc\Model\UnitOfWork which will do this.

/**
* We need to check if the record exists
*/
let exists = model->_exists(metaData, readConnection, table);

This comment has been minimized.

@Jurigag

Jurigag Oct 14, 2016

Member

There should be something like Phalcon\Mvc\Model\Worker or Phalcon\Mvc\Model\UnitOfWork which will do this.

* Depending if the record exists we do an update or an insert operation
*/
if exists {
let success = model->_doLowUpdate(metaData, writeConnection, table);

This comment has been minimized.

@Jurigag

Jurigag Oct 14, 2016

Member

There should be something like Phalcon\Mvc\Model\Worker or Phalcon\Mvc\Model\UnitOfWork which will do this.

/**
* Save the post-related records
*/
let success = model->_postSaveRelatedRecords(writeConnection, related);

This comment has been minimized.

@Jurigag

Jurigag Oct 14, 2016

Member

There should be something like Phalcon\Mvc\Model\Worker or Phalcon\Mvc\Model\UnitOfWork which will do this.

* _postSave() invokes after* events if the operation was successful
*/
if globals_get("orm.events") {
let success = model->_postSave(success, exists);

This comment has been minimized.

@Jurigag

Jurigag Oct 14, 2016

Member

There should be something like Phalcon\Mvc\Model\Worker or Phalcon\Mvc\Model\UnitOfWork which will do this.

if success === false {
model->_cancelOperation();
} else {
model->fireEvent("afterSave");

This comment has been minimized.

@Jurigag

Jurigag Oct 14, 2016

Member

There should be something like Phalcon\Mvc\Model\Worker or Phalcon\Mvc\Model\UnitOfWork which will do this.

}

if success === false {
model->_cancelOperation();

This comment has been minimized.

@Jurigag

Jurigag Oct 14, 2016

Member

There should be something like Phalcon\Mvc\Model\Worker or Phalcon\Mvc\Model\UnitOfWork which will do this.

@Jurigag
Copy link
Member

Jurigag left a comment

Worker class for model methods:

  • _preSaveRelatedRecords
  • _exists
  • _preSave
  • _doLowUpdate
  • _doLowInsert
  • _postSaveRelatedRecords
  • _postSave
  • _cancelOperation
  • fireEvent
  • _checkForeignKeysReverseRestrict
  • _checkForeignKeysReverseCascade

So we can remove all this not needed logic in model. It should impelemnt EvensAwareInterface so we can attach some listener to it. Not sure about which events exactly - i guess preSave, postSave, onCancelOperation, onUpdate, onInsert ? Also as well existing model events so we can watch all model events fired for all models in one listener.

{
var repository;

let repository = new Repository(modelClass, this);

This comment has been minimized.

@Jurigag

Jurigag Oct 14, 2016

Member

There should be property _repositories im Manager which will be array of repostiories where all repositories should be added and their should be checked before creating new one. Also add method registerRepository(<RepositoryInterface> repository) which will add option to use custom repository for models. This will need to add modelClass property to Repository class so we can retrieve it from repository when it's constructed.

@Jurigag
Copy link
Member

Jurigag left a comment

Test cases for class implementing only ModelInterface not extending Model, in future for any class without any interface or extending class.

@SidRoberts

This comment has been minimized.

Copy link
Member Author

SidRoberts commented Oct 14, 2016

What uses could a custom repository have? In my head, I imagine Repository only has the methods listed above (as well as support for findFirstBy..., findBy..., countBy..., etc;).

It might be a good idea to allow the developer to overwrite the repository class via the DI:

$di->set("modelRepository", "Phalcon\\Mvc\\Model\\Repository");

// OR...

$di->set("Phalcon\\Mvc\\Model\\Repository", "MyCustomRepositoryClass");
let repository = <RepositoryInterface> dependencyInjector->get(
    "modelRepository",
    [modelClass, this]
);

// OR...

let repository = <RepositoryInterface> dependencyInjector->get(
    "Phalcon\\Mvc\\Model\\Repository",
    [modelClass, this]
);
@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Oct 14, 2016

Repository also should provide query builder(like createBuilder()) and query method(Phalcon\Mvc\Model\Criteria). With builder we can do extensive queries. So there is example:

class PersonRepository extends Repository
{
    public function findWithProfiles()
    {
        return $this->createBuilder()->from('Person')
            ->columns('*')
            ->leftJoin('Profile')
            ->getQuery()
            ->execute();
    }

    // any other methods we need for finding persons and where generic ones are not enough
}

Just add registerRepository in models manager, and i think it will be good solution. So if someone want to use custom repository for some class with additional methods he can use it. If not and just generic repository is enough for some model then he don't need to register anything.

Your solution is too generic. There should be for 100% option to have each Model his own Repository. Otherwise it's useless Repository pattern.

So in total Repository class:

  • add createBuilder (protected ?) method
  • add query method which will be just equivalent of Model::query()

In Manager:

  • add repositories property - array for all repositories
  • add registerRepository method like:
$manager->registerRepository(new PersonRepository(Person::class));

which will add this repository to repositories, also it could be good idea to maybe lazy load it like registerRepository(PersonRepository::class, Person::class), and when it's resolved in getRepository it will be then created

  • change getRepository for checking first repositories property, if there is already exisiting repository use it, if not then create new one

Also maybe registerRepository is bad idea and better will be something like:

class Person
{
    public static function getRepository()
    {
        return new PersonRepository(self::class);
    }
}

@SidRoberts SidRoberts force-pushed the SidRoberts:v4-model-data-mapper branch from 254beb2 to a3925b8 Oct 14, 2016

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Oct 14, 2016

@SidRoberts

Also this is really great idea. But this definitely need a hard work and implement new classes. It's not only change a few we have right now = and we have data mapper. I just add my ideas what i think should be here and how it should look. How it will be implemented and what's your idea it's up to you and someone who will merge this.

So just don't lose your enthusiasm even i maybe write demotivating comments or something :D

@SidRoberts

This comment has been minimized.

Copy link
Member Author

SidRoberts commented Oct 14, 2016

It's been a long day and after spending so much time looking at code, I can't see straight anymore. I need a good night's sleep before I can do any more serious coding. 馃槤 I agree with your ideas about custom repositories though.

How about copying how the Models Manager deals with custom model sources and schemas (Model::setSource() / Model::setSchema()) and managing custom repositories like this:

class Robots extends \Phalcon\Mvc\Model
{
    public function initialize()
    {
        $this->setRepository(
            RobotsRepository::class
        );
    }
}

I think I'll use this PR as a playground for ideas and redo the entire thing when the 4.0.x branch is created and everyone's opinions are taken into account.

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Oct 14, 2016

Yea This is good way BUT i'm not sure about $this->setRepository. DataMapper is all about NOT EXTENDING any class i think.

I think in result and in final form the only needed thing for our models in phalcon 4.0.0 should be a) implement interface b) use trait c) don't use anything. Im just not sure about solution. For NOW your solution is fine, but i think we should avoid extends \Phalcon\Mvc\Model and in final form it should aim to not having extends \Phalcon\Mvc\Model and somethign like use Model as a trait(as far as i know traits are just classes internally ? Correct me if im wrong. So we can just create some class in zephir and in php do use Class and it should work. Or just only some interface for needed things to work. And repository would be just public static function getRepository()

@SidRoberts

This comment has been minimized.

Copy link
Member Author

SidRoberts commented Oct 14, 2016

I'm glad you're here - you're right and I'm thinking of solutions that require the least amount of effort to implement. 馃槈

I've never used traits but I seem to remember that it's akin to copying and pasting chunks of code, as far as the compiler is concerned.

We'll figure it out tomorrow.

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented Oct 14, 2016

But im not sure, maybe there should be like totally nothing, this is up to people with more knowledge, don't know if DataMapper pattern allows anything like trait or something like this. Well in doctrine there are annotations, and something is parsing them, so well - this is just some kind of "workaround" in doctrine to not having trait or something like this.

@SidRoberts

This comment has been minimized.

Copy link
Member Author

SidRoberts commented Oct 14, 2016

That's kind of the problem. Doctrine stores a lot of metadata in annotations and I don't think annotations will be considered fast enough. We could have metadata stored somewhere else (YAML, XML, ...) but that'd be messy. Or we could just have a few getters in Model.

@sergeyklay sergeyklay closed this Mar 22, 2017

@sergeyklay sergeyklay reopened this Mar 22, 2017

@sjinks sjinks closed this Apr 26, 2017

@sjinks sjinks reopened this Apr 26, 2017

@SidRoberts SidRoberts force-pushed the SidRoberts:v4-model-data-mapper branch 2 times, most recently from 4ed02d1 to 927aa3f May 3, 2017

@SidRoberts SidRoberts changed the base branch from 3.0.x to 4.0.x May 3, 2017

@sergeyklay sergeyklay force-pushed the phalcon:4.0.x branch from cf1abda to 53683c6 Jun 18, 2017

@sergeyklay sergeyklay force-pushed the phalcon:4.0.x branch from 53683c6 to 4a7aa3c Jul 10, 2017

@SidRoberts SidRoberts force-pushed the SidRoberts:v4-model-data-mapper branch 2 times, most recently from a39e043 to d1e6fca Aug 1, 2017

@sergeyklay sergeyklay force-pushed the phalcon:4.0.x branch from 4a7aa3c to 44ce3c6 Aug 13, 2017

@SidRoberts SidRoberts force-pushed the SidRoberts:v4-model-data-mapper branch from d1e6fca to 2c9759e Aug 15, 2017

@SidRoberts

This comment has been minimized.

Copy link
Member Author

SidRoberts commented Aug 15, 2017

I completely messed up the history and there were too many merge conflicts to deal with so I'm recreating these commits manually. The original branch is available at https://github.com/SidRoberts/cphalcon/commits/v4-model-data-mapper-original

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Aug 16, 2017

@SidRoberts Could you please check tests

@SidRoberts SidRoberts force-pushed the SidRoberts:v4-model-data-mapper branch from 2c9759e to 500d894 Aug 16, 2017

@SidRoberts SidRoberts force-pushed the SidRoberts:v4-model-data-mapper branch from 500d894 to da1ac2b Aug 16, 2017

@SidRoberts SidRoberts force-pushed the SidRoberts:v4-model-data-mapper branch from 7b45524 to a994ee1 Aug 16, 2017

@SidRoberts SidRoberts force-pushed the SidRoberts:v4-model-data-mapper branch from 062db92 to 5fc138b Aug 16, 2017

@sergeyklay sergeyklay merged commit e920d83 into phalcon:4.0.x Aug 16, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Aug 16, 2017

Thank you

chilimatic added a commit to chilimatic/cphalcon that referenced this pull request Jan 15, 2018

[RFC] Converting Active Record to the Data Mapper pattern (phalcon#12317
)

* You can no longer assign data to models whilst saving them.

* ModelManager::load() no longer reuses already initialised models.

* Removed Model::reset()
@MufidJamaluddin

This comment has been minimized.

Copy link

MufidJamaluddin commented Nov 21, 2018

I think the better option is move pagination method to this repository / modelsManager or make new class (PagingRepository)

@niden niden added this to Done in 4.0 Release Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment