Refactoring hydrator

[bakura10]

@davidwindell, @Ocramius

This is the beginning of the refactoring of the hydrator, using the byref/byval logic.

Basically, we get rid of the ClassMethods hydrator, people will have to follow some conventions on the naming.

The extracting phase is done (I hope) correctly for both byref/byval.

I still want ocramius to explain exactly why a user would need byref or byval (I mean, I know what by value and by reference means) but WHY it would chooose one over another).

For the hydrating phase, and to answer to davidwindell answer, I think that the cleanest way is to introduce strategy that are called when hydrating collections.

For instance, the intersectionUnion would become a "AllowRemove" strategy, so that if elements are not present in the posted collections, the strategy allow to remove them from the original collection, or a DisallowRemove that would do what david wanted (does not remove elements from original collection even if they are not posted).

The AllowRemove would need the user to define a removeSomethings, as well as a addSomethings that will be used to insert new elements.

What we need :

• List all the strategies we want by default
• What we need to take care for the byref thing in hydrating (especially about the inverse side).

[bakura10]

I added some strategies example so that you can see the logic. Do you think this is good ?

Not sure about the naming or namespaces (should it be DoctrineModule\Stdlib\Hydrator\Strategy\Collection...) ?

[bakura10]

Question : for by value, we clone when it's a toOne relationship. However, what happen when a proxy is cloned ? Can this be a problem ?

[davidwindell]

Thanks @bakura10, I'm interested to hear @Ocramius comments on this. With regards to;

TODO: should we ignore this or throw an exception ?

My answer would be to just skip silently in line with the ClassMethods hydrator.

[bakura10]

Ok I will change that tomorrow. I don't have a lot of time (I need to prepare my tomorrow's webinar). I'll work more on it tomorrow and try to implement the first strategy as an example. Then I'll wait ocramius feedback to check if this is the best way to do.

[bakura10]

@Ocramius , could you starting to review this so we can work more on it ? I have had several people pinging me on IRC and on my blog for issues with the current hydrator when used with strategies. The current hydrator uses the ClassMethods hydrator inside and it makes things not intuitive at all, so it would be cool if we could work on this one, as it solves this issue by not relying on the ClassMethods hydrator.

[bakura10]

@Ocramius , @davidwindell , I worked on it. I think it's finished. I've implemented the two default strategies for Collections (AllowRemove and DisallowRemove).

The hydrator code is now simpler to the end-user to as it does not rely on ClassMethods hydrator, so strategy attached to fields are directly executed. It also add the by-value and by-reference logic (although, to be honest, I still don't understand why people would need the by reference method).

I just need to write tests.

[Ocramius]

Owwwww, my fault :( Here and on #119 (which I'm thinking of rewriting personally, but at filter level)

Let me get drunk in a couple of hours and then I'll be back and ready =D

[bakura10]

No problem. Have fun tonight :).

Envoyé de mon iPhone

Le 31 déc. 2012 à 17:50, Marco Pivetta notifications@github.com a écrit :

Owwwww, my fault :( Here and on #119 (which I'm thinking of rewriting personally, but at filter level)

Let me get drunk in a couple of hours and then I'll be back and ready =D

—
Reply to this email directly or view it on GitHub.

[bakura10]

Need to tweek performacne a little bit I suppose...

[bakura10]

Hi everyone,

First of all, I wish all of you a happy new year, and lot of fun with Doctrine this year !

Concerning the hydrator refactoring, I added a tons of tests (and rewrite all of them) so now the coverage is pretty solid. You can completely review it @Ocramius .

Some notes:

• It's a major BC, so this will need to be tagged as a new version (I think we can tag it as 1.0 ?).
• I don't know if it is a problem for you @Ocramius , but I've changed the way the hydrator work. I removed completely the dependency onto the ClassMethods hydrator so now strategies attached to the DoctrineHydrator will really work (this was the most annoying thing from the previous hydrator). Furthermore, the hydrator now takes not an ObjectManager but an ObjectRepository + Metadata. This means that the hydrator can be used with only one class type. I made it this way because when constructed, the hydrator automatically creates strategies for every collections. It would have been strange if the hydrator could handle many types of objects as the strategies array would have been cluttered with lots of different strategies. Plus, it's better for performance as we can cache some things.

EDIT : I figured out that we can't get a Reference to an object using an ObjectRepository (don't know why...), and we can't access to the ObjectManager from an ObjectRepository too... However the getReference is needed in the specialization of the hydrator in Doctrine ORM to avoid to fetch the object from database (but just get a reference). That's why I moved to the object manager + class of the entity.

• Yesterday I added a support for Collections when they are used in Select. This is a common use case that a lot of people struggle too and don't understand why it's not automatically handled. Basically, you have a OneToMany relationship (user => city). When you have a form, people would like that it extracts automatically the id of the entity so that the good select element can be "enabled". However I was not satisfied by my solution...

OKEY, now I'm going to write documentation :).

[davidwindell]

Thanks for your hard work @bakura10 looking forward to giving this one a spin.

[bakura10]

The doc is updated: https://github.com/bakura10/DoctrineModule/blob/a76195401d8f9fbc2c32e4435edf889793a1d6c8/docs/hydrator.md

[Ocramius]

Same here, yoda + it is not clear why you skip on null (may want to add a comment)

[bakura10]

iirc it has because there was a test case about not hydrating null values. Someone added it. Furthermore, if you typehint your setter like this:

public function setFoo(Foo $foo);

It won't work, people need to write Foo $foo = null.

Don't know if we should allow null ?

[Ocramius]

@bakura10 coverage is awesome and this can be merged after addressing my comments.

You should also use @since 0.7. When this is in, I'll tag a new version.

You should probably spawn a few issues about:

• accessing collections byRef in write mode (instead of the adder/remover)
• type conversion (the typical "datetime" mapping problem)

[bakura10]

So, I change the since to since 0.7, I added the two strategies that work by reference, and I updated tests and doc.

As I told you in IRC:

Don't forget to merge this one in ORM too: doctrine/DoctrineORMModule#156
We'll likely need to add it in DoctrineMongoDB too

Furthermore, when you'll tag DoctrineORMModule, I think you need to change https://github.com/doctrine/DoctrineORMModule/blob/master/composer.json#L36 to "doctrine/doctrine-module": "=0.5.*",

Otherwise I suppose that DoctrineORMModule will download latest hydrator and will break everything.

[davidwindell]

Can we still set a relation to null or have you now removed that and the
test?
On 13 Jan 2013 11:18, "Michaël Gallego" notifications@github.com wrote:

So, I change the since to since 0.7, I added the two strategies that work
by reference, and I updated tests and doc.

As I told you in IRC:

Don't forget to merge this one in ORM too: doctrine/DoctrineORMModule#156doctrine/DoctrineORMModule#156
We'll likely need to add it in DoctrineMongoDB too

Furthermore, when you'll tag DoctrineORMModule, I think you need to change
https://github.com/doctrine/DoctrineORMModule/blob/master/composer.json#L36to "doctrine/doctrine-module": "=0.5.*",

Otherwise I suppose that DoctrineORMModule will download latest hydrator
and will break everything.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/106#issuecomment-12192393.

[bakura10]

For now no I didn't change that davidwindell. I've tried to port all the old tests to the new hydraotr.

[davidwindell]

Fantastic!
On 13 Jan 2013 12:37, "Michaël Gallego" notifications@github.com wrote:

For now no I didn't change that davidwindell. I've tried to port all the
old tests to the new hydraotr.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/106#issuecomment-12193116.

[Ocramius]

@davidwindell Please see #139 for what you just commented. I asked bakura to remove the checks on null values, since if I explicitly set something to null, that null should be passed to our entity.

This is a validation problem IMO

[coveralls]

Changes Unknown when pulling e56a897 on bakura10:form/hydrator-refactoring into * on doctrine:master*.