DB - related() on GroupedSelection #653

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
@juzna
Contributor

juzna commented May 15, 2012

i.e. when you apply related() twice, it won't work.

Suppose two companies, each with one author, each with two books. To output them all, you'd write three foreach loops (see the test case). There, the inner foreach doesn't work properly and will return items only for the first author.

+ $hash = md5($this->getSql() . json_encode($this->parameters));
+ $referencing = & $this->referencing[$hash];
+ if ($referencing === NULL) {
+ // i believe this should never happen

This comment has been minimized.

@juzna

juzna May 15, 2012

Contributor

Need a hint about this situation. Anyone?

@juzna

juzna May 15, 2012

Contributor

Need a hint about this situation. Anyone?

+ "Book 2",
+ "Company 42",
+ "Author 12",
+ "Book 3",

This comment has been minimized.

@juzna

juzna May 15, 2012

Contributor

Without fix, it won't print out these books (or any following)

@juzna

juzna May 15, 2012

Contributor

Without fix, it won't print out these books (or any following)

+// related() on selection where first row has no related items
+$log = array();
+foreach ($connection->table('author')->order('id DESC') as $author) {
+ foreach($author->related('book') as $book) {

This comment has been minimized.

@juzna

juzna May 16, 2012

Contributor

This is lovely! If the first author has no books, then next line will crash.
(i.e. the first item has to have subitems for related() to work properly :D )

Btw error is: Error: Invalid argument supplied for foreach()

@juzna

juzna May 16, 2012

Contributor

This is lovely! If the first author has no books, then next line will crash.
(i.e. the first item has to have subitems for related() to work properly :D )

Btw error is: Error: Invalid argument supplied for foreach()

Db: fixed very strange bug with related()
test included in previous commit

Btw: not sure how good this pach is, but makes some good sense and works
@hrach

This comment has been minimized.

Show comment
Hide comment
@hrach

hrach May 16, 2012

Contributor

WTF? :D so much confused commits? :D
See my dev branch https://github.com/hrach/nette/tree/f-database-refactoring
a this commit should fix it: hrach/nette@46f0fc9

Contributor

hrach commented May 16, 2012

WTF? :D so much confused commits? :D
See my dev branch https://github.com/hrach/nette/tree/f-database-refactoring
a this commit should fix it: hrach/nette@46f0fc9

@juzna

This comment has been minimized.

Show comment
Hide comment
@juzna

juzna May 16, 2012

Contributor

Yours is big refactoring, this was just a quick fix. But yep, not the best solution.

Your branch seems to work, but changes sooo much. Not sure when/if it'll be merged, i.e. useful for everyone.

Contributor

juzna commented May 16, 2012

Yours is big refactoring, this was just a quick fix. But yep, not the best solution.

Your branch seems to work, but changes sooo much. Not sure when/if it'll be merged, i.e. useful for everyone.

@hrach

This comment has been minimized.

Show comment
Hide comment
@hrach

hrach May 16, 2012

Contributor

Must be :D And it's seems weird only in diff. Basically it's just decomposition building sql to another class.
The information it works sounds great, so, both bugs are there fixed?
However, the referenced commit should be easy (4 lines) fix, which should work also on actual master.

From my point of you are doing in your fix some magic, which I dont undestand :D

Contributor

hrach commented May 16, 2012

Must be :D And it's seems weird only in diff. Basically it's just decomposition building sql to another class.
The information it works sounds great, so, both bugs are there fixed?
However, the referenced commit should be easy (4 lines) fix, which should work also on actual master.

From my point of you are doing in your fix some magic, which I dont undestand :D

@juzna

This comment has been minimized.

Show comment
Hide comment
@juzna

juzna May 16, 2012

Contributor

Selection loads data into $rows, which is then used when searching for related entries (both related and relating).
GroupedSelection lazily moves data from $rows into $referencing, which is an array aggregated by a particular column. Here comes the problem, when methods in Selection expect data in $rows, where it is not anymore ;)

hope it's little bit more clear now

Contributor

juzna commented May 16, 2012

Selection loads data into $rows, which is then used when searching for related entries (both related and relating).
GroupedSelection lazily moves data from $rows into $referencing, which is an array aggregated by a particular column. Here comes the problem, when methods in Selection expect data in $rows, where it is not anymore ;)

hope it's little bit more clear now

@hrach

This comment has been minimized.

Show comment
Hide comment
@hrach

hrach May 16, 2012

Contributor

No, you are not right. GroupedSelection dont move data from rows. It only aggregates data by particular column into $data. (cached by referencing).
https://github.com/hrach/nette/blob/f-database-refactoring/Nette/Database/Table/GroupedSelection.php#L165

Contributor

hrach commented May 16, 2012

No, you are not right. GroupedSelection dont move data from rows. It only aggregates data by particular column into $data. (cached by referencing).
https://github.com/hrach/nette/blob/f-database-refactoring/Nette/Database/Table/GroupedSelection.php#L165

@juzna

This comment has been minimized.

Show comment
Hide comment
@juzna

juzna May 16, 2012

Contributor

Your version ;) Nette's master branch behaves differently. Never mind though, Db needs to be refactored because these are only hacks to fix artificial problems.

Contributor

juzna commented May 16, 2012

Your version ;) Nette's master branch behaves differently. Never mind though, Db needs to be refactored because these are only hacks to fix artificial problems.

@juzna juzna closed this May 16, 2012

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