nova-framework / framework

Demo application using Nova - this is an extensive playground, use "app" repository for real applications.
http://novaframework.com/
MIT License
418 stars 210 forks source link

About implementation of a (small) ORM native into Nova Framework #365

Closed LuckyCyborg closed 8 years ago

LuckyCyborg commented 8 years ago

Sure, there are plenty of powerful ORMs... Some are "indoctrinated", other are "illuminated". :smirk:

But sometimes, while still you want to use an ORM, from habits or even from necessity, those "indoctrinated" or "illuminated" ORMs are overwhelming for your current project.

Then, I and @tomvlk , we decided to write a small one for Nova Framework.

But, in your opinion, what you expect from a small ORM?

I for one, I see an small ORM essentially as a DataMapper who manage Entities.

I even find one which looks right on as the Design I have in mind, it is called Spot:

http://phpdatamapper.com/

Sure, make no sense to integrate directly it, using yet another Events Manager (and we already have three), and wanting its own dedicated Connection; still, it use for Database Connection, guess what? Doctrine DBAL.

In other hand, it have nice Events, very similar with our BaseModel, known about Relations and other nice things. I'm about opinion that if we reuse its code to write our ORM, will rise something good.

Your opinion? You will like something like/similar with Spot included into Nova Framework?

tomvlk commented 8 years ago

save,insert and finds by the orm is required, and is currently the aim I think. Using annotations for the table and column names, types etc.

LuckyCyborg commented 8 years ago

How about Relations ?

I.e. An User hasMany Posts, but hasOne Profile.

tomvlk commented 8 years ago

Maybe, Dunno.

LuckyCyborg commented 8 years ago

Study that Spot, from the given link, is small, works on top of Doctrine DBAL and have lots of shiny things, including Relations and Events, similar with our BaseModel.

But I guess we can't integrate it directly in Nova Framework...

tomvlk commented 8 years ago

Well, it uses return arrays, not really entity in my opinion. It's just a define of structure and not a real object class.

LuckyCyborg commented 8 years ago

@tomvlk Study better the thing. It use Entities as in Entities. :smirk:

tomvlk commented 8 years ago

The method of defining isnt nice. You just return an array that defines the columns, it's better to just make fields in a class. Like I'm making.

LuckyCyborg commented 8 years ago

@tomvlk It is his own vision, works even that way... :smirk:

In other hand, I do not said to import literally that ORM in our tree.

I said to reuse its concepts and its code, to develop our ORM.

tomvlk commented 8 years ago

I think annotations (what I'm working on) are better for this. But I don't have that much time today, will have more tomorrow.

LuckyCyborg commented 8 years ago

@tomvlk Okay, let's see...

tomvlk commented 8 years ago

@LuckyCyborg Could you please look into https://github.com/tomvlk/php-framework/commits/3.0-db-orm this commit history for some implementations.

Currently I only have the Car::get($id) method up and working. But you can see the way to define the entity table and columns.

tomvlk commented 8 years ago

@LuckyCyborg Btw, I'm in love with the QueryBuilder. It's pretty cool, and we should use it in the ORM Entity stuff to support all drivers.

LuckyCyborg commented 8 years ago

@tomvlk Then, I think that you understand that choosing to migrate to Doctrine DBAL was a well thought strategic move... :smirk:

LuckyCyborg commented 8 years ago

@tomvlk Looking at your ORM, looks good for a starting point. Congratulation! :+1:

But to remember that ORM stands for Object-Relational Mapper and the Relations between objects, also the lazy loading of related objects, are essentials characteristics for an ORM.

LuckyCyborg commented 8 years ago

@tomvlk For naming consistency you can rename:

    public static function find()
    {
        return self::getLink()->createQueryBuilder();
    }

AS

    public static function getQueryBuilder()
    {
        return self::getLink()->createQueryBuilder();
    }

Eventually, if you want to expose this method to end-user, name it query, like following... :smirk:

    public static function query()
    {
        return self::getLink()->createQueryBuilder();
    }
LuckyCyborg commented 8 years ago

@tomvlk BTW, Big attention at QueryBuilder!

Never pass directly the Data got from user input to QueryBuilder (string) fields, because nothing is (and can't be) escaped by, automatically. You should always use parameter bindings or to build expressions, like you already made.

LuckyCyborg commented 8 years ago

@tomvlk Another thing,

Please learn to use:

use PDO;

And stop using ROOT \, as in \PDO::FETCH_CLASS, for example. It is more simple. :smirk:

tomvlk commented 8 years ago

@LuckyCyborg Look once again, I've made the save() method, that inserts or update the entity in the database. (Warning, the mysql db will not be cleared when executing tests yet).

tomvlk commented 8 years ago

@LuckyCyborg Stuff that isn't done yet is simple finding methods. (only get with the primary key(s) is done).

tomvlk commented 8 years ago

@LuckyCyborg And indeed, I read about the QueryBuilder, it's very dangerous, nothing is auto escaped by default. You have to do it indeed. Like I did already in the Car::get()

LuckyCyborg commented 8 years ago

@tomvlk In other hand, I wonder why you use internally the QueryBuilder; sure, for fun is OK, but I suggest you to pass on 'orthodox' methods, being slightly faster.

And, BTW, I prepared into \Nova\DBAL\Connection some methods specially for you, to simplify your work into ORM. Go a look.

tomvlk commented 8 years ago

@LuckyCyborg I'm using it for the different dialects of SQL for selecting and inserting/updating. This will have maximum support for all drivers.

LuckyCyborg commented 8 years ago

Worry not! Under Doctrine DBAL you should not care about SQL Dialects... :smirk:

Or you write right commands and they are accepted, or the DBAL will nuke the fridge.

Just remember to not ask for shiny MySQL-isme... :smirk:

tomvlk commented 8 years ago

@LuckyCyborg Theres an error in public function fetchClass($statement, array $params = array(), array $paramTypes = array(), $className = null)

Look at line 112, It's refering to the undefined $sql. (DBAL\Connection)

You mean to use the SELECT on the get($id); method? (And then fetchClass you prepared :tada: ?)

LuckyCyborg commented 8 years ago

@tomvlk A minute to take a look!

tomvlk commented 8 years ago

@LuckyCyborg I can fix it but it won't be included for now.

LuckyCyborg commented 8 years ago

@tomvlk In few seconds you have a fix into repo.

LuckyCyborg commented 8 years ago

@tomvlk Done, please update your fork! :smile:

tomvlk commented 8 years ago

@LuckyCyborg Done

tomvlk commented 8 years ago

@LuckyCyborg :laughing: Nova is fast, you see :laughing: :rocket:

LuckyCyborg commented 8 years ago

@tomvlk He he! Let's say that this is working in team. When every one know its part of code. :smirk:

LuckyCyborg commented 8 years ago

@tomvlk What is autoIncredimental ? In what Language?

tomvlk commented 8 years ago

@LuckyCyborg Thats for auto incredimental columns, Will be filled with the lastInsertId() value when inserting.

Btw, something is wrong with your fetchClass, I get exception No valid Entity Name is given: , line 62 of connection.php. I think your not giving the class to the connection.

tomvlk commented 8 years ago

@LuckyCyborg A I see , parameter order is not good in the return $this->select(...).

LuckyCyborg commented 8 years ago

Yep, sorry! Fixed now, PR incoming.

LuckyCyborg commented 8 years ago

@tomvlk Please update your fork!

tomvlk commented 8 years ago

@LuckyCyborg Already done :rocket:

tomvlk commented 8 years ago

@LuckyCyborg Next error :laughing:

PDOStatement::fetch() expects parameter 2 to be integer, string given

The fetch isn't having a second parameter, instead, use this before the fetch() on the statement:

$statement->setFetchMode(PDO::FETCH_CLASS, $className);
LuckyCyborg commented 8 years ago

@tomvlk Update your fork, please!

LuckyCyborg commented 8 years ago

@tomvlk But something is strange...

setFetchMode(PDO::FETCH_CLASS);

have a single parameter

Later: Wrong, It's about Connection, not Statement. Sorry!

tomvlk commented 8 years ago

@LuckyCyborg For PDO maybe, but for DBAL's (doctrine/dbal/lib/Doctrine/DBAL/Driver/ResultStatement.php) it has a few optional args.

tomvlk commented 8 years ago

@LuckyCyborg Look at my last commit btw.

LuckyCyborg commented 8 years ago

@tomvlk Looks good.

And BTW, in the second array you can specify the binding types for parameters, that's improve the security and you can obtain them from Entity

In other hand, there is a last minute change on \Nova\DBAL\Connection

LuckyCyborg commented 8 years ago

@tomvlk Same thing for your insert and update. You can prepare the parameter types from Entity Annotations.

And is more safe.

tomvlk commented 8 years ago

@LuckyCyborg Indeed, Will implement that once DBAL is stable (or is it already?).

tomvlk commented 8 years ago

@LuckyCyborg And the delete is incomming!

LuckyCyborg commented 8 years ago

@tomvlk I supose that is in RC, at minimum.

Was just about to add some convenience method on Connection.

And the last (untested) method was fetchClass, thing happened now. You have all the shiny things from \Doctrine\DBAL\Connection there. :smirk:

tomvlk commented 8 years ago

@LuckyCyborg This is going to look sexy! :laughing:

LuckyCyborg commented 8 years ago

@tomvlk To remember to prepare the paramTypes for second array!

You should be able to get them from Annotations.

LuckyCyborg commented 8 years ago

@tomvlk

This is going to look sexy! :laughing:

Yep, that's a sexy DataMapper. To arrive to call it ORM, will be need also for Relations...