Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for Issue #222: aggregate_column behavior doesn't work for multiple aggregates #360

Closed
wants to merge 1 commit into from

Conversation

josedsilva
Copy link

Ref: http://github.com/propelorm/Propel/issues/222
Propel does not support multiple behaviors of the same name on the same table. Therefore multiple aggregates are specified as a comma separated string.
e.g.
<behavior name="aggregate_column">
<parameter name="name" value="nb_products, total_quantity" />
<parameter name="foreign_table" value="store_product, store_product" />
<parameter name="expression" value="COUNT(product_id), SUM(quantity)" />
</behavior>

@travisbot
Copy link

This pull request passes (merged 8c67232 into 82372dd).

@fzaninotto
Copy link
Member

Not a big fan of the syntax. Plus, you must add unit tests.

$aggregateColumns = $this->getColumns();
$expressions = $this->getExpressions();
$fullyQualifiedForeignTables = $this->getFullyQualifiedForeignTables();
//var_dump(count($fullyQualifiedForeignTables)); //MARK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to remove

@willdurand
Copy link
Contributor

Too many changes to be able to understand something.. Please add strong unit tests for all these changes.

@willdurand
Copy link
Contributor

No news for a month, no tests, I'm closing that PR :)

@willdurand willdurand closed this Jun 22, 2012
@josedsilva
Copy link
Author

Hi William,

Yes please go ahead. Will take me time to submit the tests. A few bugs
detected too :(

Regards,
Jose

On 23 June 2012 02:30, William Durand <
reply@reply.github.com

wrote:

No news for a month, no tests, I'm closing that PR :)


Reply to this email directly or view it on GitHub:
#360 (comment)

@willdurand
Copy link
Contributor

@josedsilva alright, no problem. Just submit a new PR when everything is ok :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants