Improvements to behavior handling, and specifically, AggregateColumnBehavior #482

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

VanTanev commented Oct 6, 2012

This is related to a very old ticket of mine:
http://trac.propelorm.org/ticket/1055

I decided to tackle it and even expand on the original idea. Now multiple aggregate_column behaviors can be defined on the same table, and they can even point to the same foreign table.

I came across this use case when we had an object which could be in one of two main states, but they were dependent on multiple conditions. I wanted to have aggregate columns to count the instances of the two separate states, and came accross this Propel limitation (again). This is my attempt at fixing it.

To be honest, I have no good idea how to write good tests for this. Please advice.

VanTanev added some commits Oct 6, 2012

@VanTanev VanTanev Allow the same behavior to be added multiple times to one table
Based on patch by Francois: http://trac.propelorm.org/ticket/1055
5948f9d
@VanTanev VanTanev Improve AggregateColumnBehavior
Now you can have two different tables using aggregate_column, that point
to the same foreign table, as well as define multiple aggregate_columns
on the same table, and even multiple aggregate_columns on the same table
to the same foreign table! Crazyness! :)
ebb1e73
Contributor

VanTanev commented Oct 6, 2012

Related to this, currently sfPropelORMPlugin does not allow for multiple definitions of the same behavior on one table in yaml - here is a discussion on the subject. This should be fixed as well.

Member

fzaninotto commented Oct 7, 2012

Interesting approach. Do the unit test pass on your side? Because Travis says that your PR breaks the tests.

Contributor

VanTanev commented Oct 7, 2012

@fzaninotto The old testsuite passes when manually ran through console. The new "test" I have written passes as well, though it's really insufficient for these changes.

Running tests on a windows machine is.. challenging, as well as having a password on root for mysql, had to setup a VM to run the tests.

Member

fzaninotto commented Oct 8, 2012

Then I don't understand why Travis woul'nt successfully build your branch.

Owner

willdurand commented Oct 8, 2012

I'm experiencing errors on my laptop too:

1) AggregateColumnBehaviorTest::testCompute
PropelException: Undefined method ModelCriteria::findRelatedAggregatePosts()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:32
/usr/local/php5-20120919-075914/bin/phpunit:46

2) AggregateColumnBehaviorTest::testUpdate
PropelException: Undefined method ModelCriteria::findRelatedAggregatePosts()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:51
/usr/local/php5-20120919-075914/bin/phpunit:46

3) AggregateColumnBehaviorTest::testCreateRelated
PropelException: Undefined method ModelCriteria::findRelatedAggregatePosts()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:69
/usr/local/php5-20120919-075914/bin/phpunit:46

4) AggregateColumnBehaviorTest::testUpdateRelated
PropelException: Undefined method ModelCriteria::findRelatedAggregatePolls()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateItemQuery.php:437
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateItemQuery.php:437
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:248
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:88
/usr/local/php5-20120919-075914/bin/phpunit:46

5) AggregateColumnBehaviorTest::testDeleteRelated
PropelException: Undefined method ModelCriteria::findRelatedAggregatePolls()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateItemQuery.php:437
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateItemQuery.php:437
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:248
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:97
/usr/local/php5-20120919-075914/bin/phpunit:46

6) AggregateColumnBehaviorTest::testUpdateRelatedWithQuery
PropelException: Undefined method ModelCriteria::findRelatedAggregatePolls()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateItemQuery.php:437
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateItemQuery.php:437
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:248
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:107
/usr/local/php5-20120919-075914/bin/phpunit:46

7) AggregateColumnBehaviorTest::testUpdateRelatedWithQueryUsingAlias
PropelException: Undefined method ModelCriteria::findRelatedAggregatePolls()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateItemQuery.php:437
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateItemQuery.php:437
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:248
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:116
/usr/local/php5-20120919-075914/bin/phpunit:46

8) AggregateColumnBehaviorTest::testDeleteRelatedWithQuery
PropelException: Undefined method ModelCriteria::findRelatedAggregatePolls()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateItemQuery.php:437
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateItemQuery.php:437
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:248
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:126
/usr/local/php5-20120919-075914/bin/phpunit:46

9) AggregateColumnBehaviorTest::testDeleteRelatedWithQueryUsingAlias
PropelException: Undefined method ModelCriteria::findRelatedAggregatePolls()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateItemQuery.php:437
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateItemQuery.php:437
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:248
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:135
/usr/local/php5-20120919-075914/bin/phpunit:46

10) AggregateColumnBehaviorTest::testRemoveRelation
PropelException: Undefined method ModelCriteria::findRelatedAggregatePosts()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:146
/usr/local/php5-20120919-075914/bin/phpunit:46

11) AggregateColumnBehaviorTest::testReplaceRelation
PropelException: Undefined method ModelCriteria::findRelatedAggregatePosts()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:164
/usr/local/php5-20120919-075914/bin/phpunit:46

12) AggregateColumnBehaviorTest::testAddMultipleComments
PropelException: Undefined method ModelCriteria::findRelatedAggregatePosts()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:183
/usr/local/php5-20120919-075914/bin/phpunit:46

13) AggregateColumnBehaviorTest::testQueryCountOnUpdate
PropelException: Undefined method ModelCriteria::findRelatedAggregatePosts()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/test/fixtures/bookstore/build/classes/behavior/aggregate/om/BaseAggregateCommentQuery.php:392
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorTest.php:207
/usr/local/php5-20120919-075914/bin/phpunit:46

14) AggregateColumnBehaviorWithSchemaTest::testComputeWithSchema
PropelException: Undefined method ModelCriteria::findRelatedBookstoreSchemasBookstores()

/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:2203
/Users/william/projects/Propel/Propel/test/fixtures/schemas/build/classes/bookstore/om/BaseContestBookstoreContestEntryQuery.php:623
/Users/william/projects/Propel/Propel/test/fixtures/schemas/build/classes/bookstore/om/BaseContestBookstoreContestEntryQuery.php:623
/Users/william/projects/Propel/Propel/runtime/lib/query/ModelCriteria.php:1617
/Users/william/projects/Propel/Propel/test/testsuite/generator/behavior/aggregate_column/AggregateColumnBehaviorWithSchemaTest.php:45
/usr/local/php5-20120919-075914/bin/phpunit:46

willdurand closed this Oct 8, 2012

willdurand reopened this Oct 8, 2012

Owner

willdurand commented Nov 23, 2012

Hey @craftyshadow I tried to fix your changes, but I can't get a working version. IMO you missed to rename generated method calls in the hooks. But then, I ended with an infinite loop calling save().

Contributor

VanTanev commented Nov 23, 2012

I have set aside the whole of Saturday to work on this issue and some other, will have something to show for it then.

Basically, method names should be generated similar to FK methods, the getObjectRelatedByColumn() methods; Also, it that should be done only when multiple aggregate columns are used on the same foreign table, so quite a few changes will be necessary.

Owner

willdurand commented Nov 23, 2012

Oh ok, thank you for your time.

Owner

willdurand commented Dec 28, 2012

ping @craftyshadow

Contributor

VanTanev commented Dec 28, 2012

Ping acknowledged; I'm kind of caught between jobs now, but will get back to this as soon as possible. Sorry for the delay.

Owner

willdurand commented Dec 29, 2012

Alright, then reopen the PR when it's ok :)

willdurand closed this Dec 29, 2012

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