[RFC] [3.3] Add support for associations in custom datasource #9002
Conversation
| @@ -15,6 +15,7 @@ | |||
| namespace Cake\ORM; | |||
|
|
|||
| use ArrayIterator; | |||
| use Cake\Datasource\AssociationsNormalizerTrait; | |||
There was a problem hiding this comment.
Doesn't this leave us with a lot of duplicated code between ORM and Datasour e packages?
There was a problem hiding this comment.
I moved AssociationsNormalizerTrait to the datasource package because its not very specific to the ORM.
There was a problem hiding this comment.
I was trying to comment on the AssociationCollection as a whole. The ORM version is still there.
There was a problem hiding this comment.
Thanks, I didn't see that. This has been resolved 👍
|
Is thee an alternative implementation of associations already available? I fear that we may be abstracting something without thinking if this is the right abstraction |
|
I'm currently working on one for the Webservices plugin. I opened the PR so that the interface could be discussed 😄 |
Marlinc
changed the title
| * required joins and decorating the results so that those associations can be | ||
| * part of the result set. | ||
| */ | ||
| interface EagerLoaderInterface |
There was a problem hiding this comment.
A good start would be not having this as an interface. This class is pretty much knowledgable of how joins work in SQL.
|
I think most of it makes sense, except for the functions I pointed out |
|
Closing as this can't be merged. If you have time @Marlinc to get this into a mergable state, we can discuss it further. |
| public function add($alias, AssociationInterface $association) | ||
| { | ||
| list(, $alias) = pluginSplit($alias); | ||
| return $this->_items[strtolower($alias)] = $association; |
There was a problem hiding this comment.
Missing blank line before return statement
Marlinc
force-pushed
the
datasource-associations
branch
from
8657742
to
b258856
Compare
Marlinc
force-pushed
the
datasource-associations
branch
3 times, most recently
from
02d9e0f
to
9f526ac
Compare
|
|
||
| $out = array_filter($this->_items, function ($assoc) use ($class) { | ||
| list(, $name) = namespaceSplit(get_class($assoc)); | ||
|
|
||
| return in_array(strtolower($name), $class, true); | ||
| return in_array($name, $class, true); |
There was a problem hiding this comment.
Missing blank line before return statement
| * @param string $name The Association name. | ||
| * @param array $config The list of properties to set. | ||
| */ | ||
| public function __construct($name, array $config = []) |
There was a problem hiding this comment.
Codecov says there's no unit test coverage for this class.
There was a problem hiding this comment.
Thank you, I'll look at this!
Marlinc
force-pushed
the
datasource-associations
branch
from
fc697f8
to
2ea9c7a
Compare
|
This needs another rebase :( |
|
Is this BC if you move classes without class_aliasing them? etc |
|
Closing as this can't be merged anymore. If it can be rebased we can discuss it further. |
|
This is gonna be picked up again, im reopening this PR. Reopening before rebasing is needed since there is a bug in github. See: isaacs/github#361 |
|
ping @Marlinc |
Marlinc
force-pushed
the
datasource-associations
branch
2 times, most recently
from
c378b67
to
68a273c
Compare
|
Closing again for the time being, rebase has been done, can be reopened later when @Marlinc pushes more changes to further discuss |
|
reopening again as requested by @Marlinc |
Marlinc
force-pushed
the
datasource-associations
branch
from
68a273c
to
f1872eb
Compare
Marlinc
force-pushed
the
datasource-associations
branch
from
f1872eb
to
c72670d
Compare
|
Closing until this can be put into a state where it can be merged. |
These changes make it possible to use associations in custom datasources. It also moves some logic that is not very database ORM specific to the datasource component.
This is still a work in progress and comments are appreciated!