Throw exception when setting association with duplicate alias. #17676
Conversation
ADmad
force-pushed
the
5.1-duplicate-association-alias
branch
from
a43f933
to
ba1d54a
Compare
| @@ -131,6 +131,10 @@ protected function setupAssociations(): void | |||
| $conditions[$name . '.content !='] = ''; | |||
| } | |||
|
|
|||
| if ($this->table->associations()->has($name)) { | |||
There was a problem hiding this comment.
These checks are mainly required because in our test suite we remove and re-add the Translate behavior with different configs but removing the behavior doesn't remove the associations created on the source table.
ADmad
force-pushed
the
5.1-duplicate-association-alias
branch
from
ba1d54a
to
01e78f5
Compare
|
Someone needs to check why MySQL is not accessible for the GA. |
ADmad
force-pushed
the
5.1-duplicate-association-alias
branch
from
1027609
to
e679478
Compare
ADmad
force-pushed
the
5.1-duplicate-association-alias
branch
from
e679478
to
7ab35e0
Compare
Ubuntu 22.04 has MySQL 8, so using docker to get the latest image, to test stuff like window functions, isn't necessary right? |
| @@ -461,7 +461,7 @@ public function testAttachToFormatResultsNoDirtyResults(): void | |||
| { | |||
| $this->setAppNamespace('TestApp'); | |||
| $articles = $this->getTableLocator()->get('Articles'); | |||
| $articles->belongsTo('Authors') | |||
There was a problem hiding this comment.
Not sure users would ever have this code in their table init, but I think the error makes sense in this situation.
I think so. If we supported multiple versions, then we might want to keep the wait loop. |
Co-authored-by: othercorey <corey.taylor.fl@gmail.com>
Let's keep things simple, we can switch back to using docker if/when needed. |
| @@ -78,6 +80,10 @@ public function add(string $alias, Association $association): Association | |||
| { | |||
| [, $alias] = pluginSplit($alias); | |||
|
|
|||
| if (isset($this->_items[$alias])) { | |||
| throw new CakeException(sprintf('Association alias `%s` is already set.', $alias)); | |||
There was a problem hiding this comment.
Generally I'm in favour of this, but we should be prepared for this to impact userland plugins a bit.
There was a problem hiding this comment.
Do plugins stomp each other? Do we need to allow plugins to set if they're stompable? That would depend on order of load so maybe it's a useless concept.
There was a problem hiding this comment.
They could and that likely means it is happening somewhere in the wild. If this becomes a blocker we can always revisit it.
There was a problem hiding this comment.
I am willing to risk a few support tickets, if things get too bad we can always revert :)
There was a problem hiding this comment.
Based on the queries I have seen on our support channels, hidden duplication/overwriting causes a lot of grief to users.
There was a problem hiding this comment.
ok, I'm onboard with the change 👍
This is quite a common mistake done by new users (when creating multiple associations to the same table) and the exception should save them lot of debugging and support requests.