Added polymorphic many-to-many relationships. by wleona3 · Pull Request #1319 · propelorm/Propel2

wleona3 · 2016-12-30T18:09:04Z

Allows for polymorphic many-to-many relationships. Unfortunately at this time does not allow additional static FKs. The builder almost supports it, but since each polymorphic sibling requires a unique relation, the builder tries to also create a new relation for the other static FKs. Example: I have a table that has two static FK relationships A and B and two polymorphic siblings, C1 and C2. When creating object A the builder has to create properties and methods for the unique C1 and C2 relationships, but then also creates two B relationships, creating duplicate properties and methods. Not sure exactly how to implement that properly. But if that caveat is fixed, this would support any combination of static FKs and Polymorphic FKs.

marcj · 2017-05-15T14:20:54Z

Wow, that's a rather big change, thanks dude! I'd rather want to see that beauty in https://github.com/propelorm/Propel3 :P

wleona3 · 2017-05-15T15:29:17Z

Thanks for the kind words marcj, unfortunately our company will be stuck with PHP 5.6 for the foreseeable future so we'll be sticking with Propel2

src/Propel/Generator/Builder/Om/ObjectBuilder.php

wleona3 · 2017-06-16T13:08:41Z

Whitespace corrected!

Incognito

This branch passes CI when rebased on latest master. The new tests also pass when rebased.

Because it's been open for a while we should give this a proper read-over to check how the feature works and see if there's any code changes needed to meet the new standards on the project.

It looks like a really cool feature to add to Propel.

Incognito · 2020-07-13T15:00:38Z

...Propel/Tests/Generator/Builder/Om/GeneratedObjectM2MRelationPolymorphicWithSelfReference.php

+    /**
+     *
+     *           add     |    remove     |    set     |    get
+     *        +---------------------------------------------------
+     * add    |    1             2             3            4
+     * remove |    5             6             7            8
+     * set    |    9            10            11           12
+     *
+     */
+
+
+
+    /*
+     * ####################################
+     * 1. add, add
+     */


Suggested change

/**

*

* add | remove | set | get

* +---------------------------------------------------

* add | 1 2 3 4

* remove | 5 6 7 8

* set | 9 10 11 12

*

*/

/*

* ####################################

* 1. add, add

*/

/**

* | | Add | Remove | Set | Get |

* |--------|-----|--------|-----|-----|

* | Add | 1 | 2 | 3 | 4 |

* | Remove | 5 | 6 | 7 | 8 |

* | Set | 9 | 10 | 11 | 12 |

*

* Test the intersection of `Add` and `Add`.

*/

I'm not 100% sure if these comments are meaningful or should be removed, I just wanted to improve the presentation of it.

Incognito

This is a really cool feature and I think it will be useful to anyone with polymorphic needs.

Incognito · 2020-07-13T15:02:18Z

...Propel/Tests/Generator/Builder/Om/GeneratedObjectM2MRelationPolymorphicWithSelfReference.php

+     * ####################################
+     * 1. add, add
+     */
+    public function test1()


Suggested change

public function test1()

public function testCombinations()

I suspect this test should be broke into three tests, based on the cyclical appearance of setup + assert code in here.

Incognito · 2020-07-13T15:07:44Z

...Propel/Tests/Generator/Builder/Om/GeneratedObjectM2MRelationPolymorphicWithSelfReference.php

+class GeneratedObjectM2MRelationPolymorphic extends PlatformDatabaseBuildTimeBase
+{
+    /**
+     * Tests for a M2M relation with three pks where each is a FK.


Suggested change

* Tests for a M2M relation with three pks where each is a FK.

* Tests for a Many-to-Many relation with three PKs where each has its own FK.

Incognito · 2020-07-13T15:08:50Z

...Propel/Tests/Generator/Builder/Om/GeneratedObjectM2MRelationPolymorphicWithSelfReference.php

+    /**
+     *
+     *           add     |    remove     |    set     |    get
+     *        +---------------------------------------------------
+     * add    |    1             2             3            4
+     * remove |    5             6             7            8
+     * set    |    9            10            11           12
+     *
+     */
+
+
+
+    /*
+     * ####################################
+     * 1. add, add
+     */


I'm not 100% sure if these comments are meaningful or should be removed, I just wanted to improve the presentation of it.

Incognito · 2020-07-13T15:11:54Z

src/Propel/Common/Util/CartesianProduct.php

@@ -0,0 +1,43 @@
+<?php namespace Propel\Common\Util;
+
+class CartesianProduct


I would like to see some way to limit the size of this. Cartesian products end up being expensive for both CPU and RAM in easily process-crashing-ways.

Incognito · 2020-07-13T15:13:20Z

src/Propel/Common/Util/CartesianProduct.php

+
+class CartesianProduct
+{
+    public static function get($input) {


There's a good opportunity to decrease the logical nesting here. If I get a moment I might propose an alternative that uses generators (since propel is now 7.1+).

Incognito · 2020-07-13T15:17:21Z

src/Propel/Generator/Builder/Om/ObjectBuilder.php

-                    $script .= "
+                    if ($value) {
+                        $script .= "
+                        \$entryPk[$idx] = '$value';";
+                    } else {
+                        $script .= "
                        \$entryPk[$idx] = \$combination[$combinationIdx]->get{$foreign->getPhpName()}();";
+                    }


Because addCrossFkScheduledForDeletion is already quite complex we should do our best to keep reducing it whenever possible. I see an opportunity here to simply put this additional logic into a protected method.

It could also help reduce duplication, as the other two code hunks in this patch look very similar to me.

Incognito · 2020-07-13T15:20:06Z

src/Propel/Generator/Model/ForeignKey.php

        return 0 !== count($cols);
    }
+
+    public function isTheSamePolymorphicSignature(ForeignKey $fk)


There should be a discussion around the names introduced in this model as they're going to become highly visible.

Incognito · 2020-07-13T15:22:39Z

src/Propel/Generator/Model/ForeignKey.php

+        $sortedCols = $this->getLocalColumns();
+        sort($sortedCols);
+
+        return sha1(implode('|||', $sortedCols));


I don't know about everyone else, but any time I've wanted to generate something like a "unique signature" it's always come back to haunt me in 2 ways:

It ends up being slow when called hundreds of times in a loop.

It ends up being really hard to change in a BC way because it isn't abstracted out of the class itself.

It might be a good feature but we should look at improving these two points. Perhaps the signature generation could be even simpler than it is, and/or it could also be moved out of the class.

Incognito · 2020-07-13T15:26:03Z

src/Propel/Generator/Model/Table.php

-                    if ($fk->isAtLeastOneLocalPrimaryKeyIsRequired() &&
-                        $crossFK->isAtLeastOneLocalPrimaryKeyNotCovered($fk)) {
-                        $crossFK->addCrossForeignKey($fk);
+                // group all polymorphic fks


Let's find a way to reduce the complexity of this class. It was already 2 nested foreach loops, both nested inside if statements, this has now increased a fair bit.

I'd also like to understand more about the performance impact of this change, and what cases users of Propel will need to be cautious about getting into.

PhilinTv · 2025-06-24T11:12:37Z

Justification for Beta6.

An advanced ORM feature that is useful in complex data modeling (e.g., tagging, permission systems).
Extensive test coverage is included (including polymorphic + self-referencing cases).
Well-isolated implementation via ForeignKey signature logic and cartesian combinator.
DX complexity is manageable, and performance tradeoffs are acceptable.

wleona3 mentioned this pull request May 15, 2017

Can't use isCrossRef with polymorphic relationships #1369

Closed

marcj reviewed May 26, 2017

View reviewed changes

src/Propel/Generator/Builder/Om/ObjectBuilder.php Outdated Show resolved Hide resolved

marcj added the Investigate label May 26, 2017

wleona3 force-pushed the master branch from bbab48b to faae496 Compare June 16, 2017 13:08

Added polymorphic many-to-many relationships.

7077b85

wleona3 force-pushed the master branch from faae496 to 7077b85 Compare June 16, 2017 13:11

Incognito reviewed Jul 13, 2020

View reviewed changes

Incognito suggested changes Jul 13, 2020

View reviewed changes

PhilinTv added this to the 2.0.0 Beta 6 - Modernization milestone Jun 24, 2025

	* Tests for a M2M relation with three pks where each is a FK.
	* Tests for a Many-to-Many relation with three PKs where each has its own FK.

		@@ -0,0 +1,43 @@
		<?php namespace Propel\Common\Util;

		class CartesianProduct

Conversation

wleona3 commented Dec 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcj commented May 15, 2017

Uh oh!

wleona3 commented May 15, 2017

Uh oh!

Uh oh!

wleona3 commented Jun 16, 2017

Uh oh!

Incognito left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Incognito left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhilinTv commented Jun 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wleona3 commented Dec 30, 2016 •

edited

Loading

Incognito left a comment •

edited

Loading