Skip to content

Always use getOMClass in findPkSimple and peer's populateObject #592

Open
wants to merge 10 commits into from

6 participants

@glorpen
glorpen commented Feb 3, 2013

It was bugging me for a while - shouldn't getOMClass be always used, disregarding table inheritance?

And another question if i may:
if getOMClass is a way to go with extending models, is it safe to assume that in namespaced project substr($modelClass::PEER,0,-4).'Query' will always yield proper Query classpath?

@glorpen
glorpen commented Feb 4, 2013

found another, hope that it is the last place

@staabm staabm and 1 other commented on an outdated diff Feb 4, 2013
generator/lib/builder/om/PHP5PeerBuilder.php
$script .= "
+ \$cls = ".$this->getPeerClassname()."::getOMClass(\$row, \$startcol);
@staabm
Propel member
staabm added a note Feb 4, 2013

when no behaviours are used the method getOMClass looks like

    public static function getOMClass()
    {
        return AddressPeer::OM_CLASS;
    }

This makes me feel that the signature this code relies on getOMClass($row, $startcol); will not be availbale and lead at least to warnings...?

Seems the behaviour which you are using here modifies the signature of the getOMClass method...?

@glorpen
glorpen added a note Feb 4, 2013

Yes, it modifies the signature but then it is already not consistent through all model classes because depends on table behaviors (inheritance would be my guess). Don't know is this behavior is ok from project api/coding rules standpoint.

Since php4 php supports variable-length argument list so you can do

function vaa(){ $vars=func_get_args(); }
vaa(1, "arg2", 3);
@staabm
Propel member
staabm added a note Feb 4, 2013

ok, so IMO the code current code makes sense the way it is coded.
Also changing the code to something, which only makes sense when inheritance is used will make the code harder to understand and maintain for the usual case...

What benefit do you get by your change and what your motivation to change the call here?

If there are not performance benefits or some other aspects which make your life easier, I would feel like not changing this, because there always a certain risk to get new bugs..

@glorpen
glorpen added a note Feb 4, 2013

point taken :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@staabm
Propel member
staabm commented Feb 4, 2013

And now you could add uni test, which supports your change (fixed behaviour) :-)

@glorpen
glorpen commented Feb 4, 2013

hope tests are good and well indented ;)

@staabm
Propel member
staabm commented Feb 4, 2013

@willdurand the stage is yours :-)

@willdurand
Propel member

I don't fully understand why you need this change, but it breaks everything.

@glorpen
glorpen commented Feb 5, 2013

It only breaks the tests, i didn't notice that there should be another schema in another package even for not real behavior. Fixed now :)

Without this change, overriding peer's getOMClass will not work as expected - in some cases there will be old class returned/populated.

@staabm
Propel member
staabm commented Feb 16, 2013

could you describe the use-case?

@glorpen
glorpen commented Feb 16, 2013

The original use case:
When using AdmingeneratorGeneratorBundle + FOSUserBundle with their schema.xml i had to do some fancy Constraints/Callback validation on (FOS)User model. I didn’t want to copy whole schema.xml and simply extending model didn’t work (because of typed arguments and query returning original class).
I thought the best way to deal with this would be creating MyUser class extending User, overwriting getOMClass in some way (i’ve done it as propel behavior in custom bundle) to point at MyClass and then do validation. And it worked :)
(well, i’ve found 2 assumptions in other bundles about namespace of query class but i have working patches and will pull-request that should this patch be merged )

The "should it work that way?" use case :) - for code:

\Propel::disableInstancePooling();

$t=array();
$t[] = TenderCpvQuery::create()->findOne()->getCpv();
$t[] = CpvQuery::create()->limit(1)->find()->getFirst();
$t[] = CpvQuery::create()->findOne();
$t[] = CpvQuery::create()->findPk(1);
$t[] = CpvPeer::retrieveByPK(1);

foreach($t as $q){
      $output->writeln(get_class($q));
}

when getOMClass is overwrited before patch:

...\BaseBundle\Propel\Cpv
...\BaseBundle\Propel\Cpv
...\BaseBundle\Propel\Cpv
...\BaseBundle\Propel\Cpv
...\BaseBundle\Propel\MyCpv

after patch:

...\BaseBundle\Propel\MyCpv
...\BaseBundle\Propel\MyCpv
...\BaseBundle\Propel\MyCpv
...\BaseBundle\Propel\MyCpv
...\BaseBundle\Propel\MyCpv
@glorpen
glorpen commented Mar 3, 2013

Out of curiosity i've applied change_om_class behavior to all bookstore/behaviors models in tests, it didn't seem to break anything except classname based asserts.

Is getOMClass deemed for internal use only or is there something else i could do to help?

@staabm staabm commented on an outdated diff Mar 3, 2013
test/testsuite/generator/builder/om/QueryBuilderTest.php
@@ -1083,6 +1083,18 @@ public function testPruneCompositeKey()
$nbBookListRel = BookListRelQuery::create()->prune($testBookListRel)->count();
$this->assertEquals(1, $nbBookListRel, 'prune() removes an object from the result');
}
+
+ public function testfindPkSimpleUsesGetOmClass()
+ {
+ $b = new BookExtended();
+ $b->save();
+
+ require_once dirname(__FILE__) . '/fixtures/MyBookExtended.php';
+ Propel::disableInstancePooling(); // need to be disabled to test the hydrate() method
@staabm
Propel member
staabm added a note Mar 3, 2013

You need to re-enable the instance pool.. Could't you use setup/teardown for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@staabm
Propel member
staabm commented Mar 3, 2013

I don't know the reason why we have a OM_CLASS constant and a method, so I am not able to decide whether this makes sense or not...

Any performance implications?

@willdurand
Propel member

Part of the plain old black magic :)

@glorpen
glorpen commented Mar 9, 2013

so, i think i've found answer for my second question - PropelQuery :) also i've updated patch for current master.

@staabm

$col is is never read

@staabm

The additonal class loading could have a perf impact...?

Hm, if first $queryClass is right then second class_exists($queryClass) shouldn't hit autoloader. If its wrong, then normally you would get an exception. So i believe that only performance loss would be when searching for something nonexistent

Propel member

You got me wrong. I mean the added path which uses getOMClass() will need another class loading call..
Also this could make problems in case you would like to change the OMClass but there is a class named $class. 'Query' from the user space. This would make your defined OMClass rendered useless...

@staabm staabm commented on the diff Mar 9, 2013
test/testsuite/generator/builder/om/QueryBuilderTest.php
@@ -1083,6 +1083,18 @@ public function testPruneCompositeKey()
$nbBookListRel = BookListRelQuery::create()->prune($testBookListRel)->count();
$this->assertEquals(1, $nbBookListRel, 'prune() removes an object from the result');
}
+
+ public function testfindPkSimpleUsesGetOmClass()
+ {
+ $b = new BookExtended();
+ $b->save();
+
+ require_once dirname(__FILE__) . '/fixtures/MyBookExtended.php';
+ BookExtendedPeer::clearInstancePool(); //clear saved object
@staabm
Propel member
staabm added a note Mar 9, 2013

Better comment: "force hydration"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@staabm staabm commented on the diff Mar 9, 2013
.../helpers/bookstore/behavior/ChangeOmClassBehavior.php
+/**
+ * This file is part of the Propel package.
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ *
+ * @license MIT License
+ */
+
+class ChangeOmClassBehavior extends Behavior
+{
+
+ public function extensionPeerFilter(&$script)
+ {
+ $getOMClass = <<<EOF
+\\1
+ static public function getOMClass(\$row = 0, \$colnum = 0)
@staabm
Propel member
staabm added a note Mar 9, 2013

$row is an array when fetched from the statement, therefore 0 is not a good default IMO, maybe add a typehint?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@staabm staabm commented on the diff Mar 9, 2013
generator/lib/builder/om/QueryBuilder.php
} else {
$script .="
- \$obj = new $ARClassname();";
@staabm
Propel member
staabm added a note Mar 9, 2013

ARClassname was also used in the phpdoc, therefore you should adjust this also. The var is never used afterwards and can be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@staabm
Propel member
staabm commented Mar 9, 2013

Added some comments about the actual implementation, but I cannot decide whether this PR will be merged. Have no clear opinion on this.

@glorpen
glorpen commented Mar 11, 2013

I’ve moved my coding efforts to own bundle for Symfony2, so i’ll try explain the pros again, maybe more clearly :) if you are still not convinced i think we can close this PR.

When using third party library which has own model (om/map) + own user classes you often cannot add own methods - you have to hijack whole model and copy it to your app.
i realize that user classes are classes generated for editing by developer and i guess that the right way to add vendor code would be by behaviors but i didn’t see it done that way too often.
PR could make it possible to override getOMClass methods and with changes in PropelQuery the whole “process” could be using user extended classes.
I guess its something like in old days - 5 years ago ;) - http://trac.symfony-project.org/wiki/HowToExtendPropelPluginModel

@jaugustin
Propel member

@glorpen Have you tried to use delegate behavior ? maybe that could solve your problem.

@glorpen
glorpen commented Mar 11, 2013

almost, but since it uses __call it will fail when using introspection by reflection - so whats left is going through delegated model (could not be supported in every case) or writing empty delegated methods.
And in case there aren't any changes in schema it would be overkill to create another table just for code.

@jaugustin
Propel member

@glorpen if you mean issue with property path I wrote a PR for that maybe it will be included in SF 2.3,
Otherwise you could use a workaround with __get __set (that call getter and setter)

@glorpen
glorpen commented Mar 11, 2013

@jaugustin ok, with that PR accepted it should solve my current problem. But i'm still not content about creating new db table with delegate behavior when you don't need schema modifications - just for eg. callback validator in S2

@alanbem
alanbem commented Apr 28, 2013

I think my question here propelorm/PropelBundle#212 is somewhat associated with yours.

@glorpen
glorpen commented Apr 28, 2013

Sounds about the same :) At the end i've created own bundle - https://bitbucket.org/glorpen/glorpenpropelbundle#model-extending that allows extending model classes and adds proper support for it in query/peer methods.

@alanbem
alanbem commented Apr 28, 2013

I would love to see PropelBundle being able to generate userland models into e.g. src/Application/{BundleName}/Model/* or src/Application/Model/{BundleName}/*.

@glorpen
glorpen commented Apr 28, 2013

You can do that by changing namespace attribute in bundle's schema.xml (or app/Resources/...) to eg. Application\Model\{BundleName} but if it is in context of this PR you would have to copy/paste chunks of code from old model.

@alanbem
alanbem commented Apr 28, 2013

but if it is in context of this PR you would have to copy/paste chunks of code from old model.

What is on my mind is that Propel would generate all models into src/Application but allow to easily define symfony1-style proxy objects or generate them by default.

@hakre
hakre commented Aug 2, 2013

It was bugging me for a while

Really? Why? Where? What is your concrete motivation? It seems that this is not really needed. What is the use-case?

Edit: Original use-case is here: #592 (comment)

@glorpen
glorpen commented Aug 2, 2013

Use case is described in comments above :)
Since then there even was question in http://stackoverflow.com/questions/16585957/fosuserbundle-extend-propel-user.

@staabm , I think we can close PR if there wasn't change in mind on your side.

@hakre
hakre commented Aug 2, 2013

Use case is described in comments above :)

There are many comments, can you give a summary or point to a specific one? Or should I read the ":)" that you don't want to share (again)? I was asking because of interest, stumbling over your code and this PR here: http://stackoverflow.com/a/18010353/367456 which might be related - at least via your symfony2 extension.

@glorpen
glorpen commented Aug 2, 2013

It is comment 13677611 and starts with "The original use case:". It was about how to modify model classes when using third party bundle with own model. There a few other solutions in this thread which can be applied to some use cases.

@hakre
hakre commented Aug 2, 2013

@glorpen: Thanks for the details and explanation, that was what I was looking for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.