Unify array structure for containing and marshaling associated data#18988
Unify array structure for containing and marshaling associated data#18988dereuromark wants to merge 3 commits into6.xfrom
Conversation
| } | ||
|
|
||
| /** | ||
| * Returns the list of known option keys that should not be treated as associations. |
There was a problem hiding this comment.
This is why I actually prefer the use of the associated key, you don't have to maintain this list of known keys.
How? It seems like only one of them provides the full feature set. The other forms are for 'simple' cases. Maybe we could remove the 'dot notation' form, but I think that is also supported by |
|
Yeah, now they are now equal in support with this. |
|
My issue isn't really with the dot notation. It's with array structure like this where the aliases and config keys can be mixed at the same level: $usersQuery->contain([
'Articles' => [
'conditions' => [.....], // option key
'Comments', // model alias
],
]);I would prefer if the 2nd level associations were also under the config key: $usersQuery->contain([
'Articles' => [
'conditions' => [.....],
'associated' => ['Comments']
],
]); |
|
I can agree that $usersQuery->contain([
'Articles' => [
'conditions' => [.....], // option key
'Comments', // model alias
],
]);is awkward. I'm on-board for not having that anymore. Would it suffice to deprecate only this call style in 5.x? That would allow us to remove the problematic scenario with a graceful upgrade path. It looks like this call style can be detected by having a mix of string and int keys which should be simple enough to detect. I think this also sets us up better for contain options to use a builder pattern more easily in the future. |
|
If aliases were always UpperCaseCamel (upper case first letter), there would also be no collisions. |
I would be happy with just that.
Not always, I think you can have this form too: $usersQuery->contain([
'Articles' => [
'conditions' => [.....], // option as key
'Comments' => [..options for this inner model...] // model alias as key
],
]);
Yes, adding builders in future would be nice. I believe that will also make it easier for AI agents to write CakePHP code.
Yes when naming the aliases as per convention the chances of collision are negligible but it's also about readability. I personally find that this mix of options and alias as same level adds unnecessary cognitive load. |
Resolved conflict in src/ORM/Marshaller.php by keeping both changes: - Documentation for new nested array format (from PR) - Template type annotations (from 6.x) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
So what course do we go? |
|
Is the proposal to check if the isn't isn't one of the allowed option keys and if so suggest using |
|
My main concern would be that this verbose form is super hard to type for default case when u just want list them nested. |
|
Is is annoying. I'm not sure how to improve it with fluid getters other than with a closure - but we already do that to modify the query. |
|
The few times you need to set options, I would in this syntax prefer underscores. We could throw deprecations in 5.x. |
|
So with what approach do we want to go moving forward? $usersQuery->contain([
'Articles' => [
'conditions' => [.....], // option key
'Comments', // model alias
],
]);Seems fine to me as long as you stick to conventions (lower vs upper should never conflict) tbh $usersQuery->contain([
'Articles' => [
'_conditions' => [.....], // option key
'Comments', // model alias
],
]);Prefixing options with underscore would be an option to separate. Using associated key always is quite verbose and not sure if that's so user-friendly in the end. Another idea: $usersQuery->contain(Contain::build()->...) Not sure how the syntax for such a nesting would look like though. |
|
We could even go as far as using This was its easy to parse, in those few cases those are applied. So: Current options are
// Clean separation:
protected function extractAssociations(array $options, Table $table): array
{
$associations = [];
$actualOptions = [];
$associationNames = array_keys($table->associations()->keys());
foreach ($options as $key => $value) {
if (is_int($key)) {
$associations[] = $value;
} elseif (in_array($key, $associationNames, true) || $table->hasAssociation($key)) {
$associations[$key] = $value;
} else {
$actualOptions[$key] = $value;
}
}
return [$associations, $actualOptions];
}This flips the logic: instead of "known options list", anything matching an association name is an association, everything else is an option. No maintenance needed. I think 1. would be an option for Cake 6 if we wanted to, "options" and "associations" being the only special keyword. |
Replace getKnownOptions() with convention-based detection: - Association names start with uppercase (CamelCase): Users, Articles - Option keys start with lowercase (camelCase): onlyIds, conditions - Special data keys start with underscore: _joinData, _ids This eliminates the need to maintain a list of known keys that must be updated whenever new marshalling/contain options are added.
|
Pushed a commit that addresses the concern about maintaining the known options list. Solution: Use CakePHP naming conventions instead of a hardcoded list:
This eliminates The convention is already enforced by CakePHP's existing patterns (model names are always CamelCase, option keys are always camelCase), so this is fully backward compatible. Maybe this should target 5.next then.. |
Resolves comments and 6.x roadmap of
#17547
Supported Formats:
But this PR could actually go into 5.next, since it seems "BC".
In 6.x we could then think about removing one of the different declarations.