-
Notifications
You must be signed in to change notification settings - Fork 4
CFM-511/CMF-274 Tab navigation resolves wrong ID for plugin model index links/Breadcrumb fixes #385
CFM-511/CMF-274 Tab navigation resolves wrong ID for plugin model index links/Breadcrumb fixes #385
Conversation
c962f40 to
6c7ac5a
Compare
arlen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
892ed7c to
de934df
Compare
de934df to
7182820
Compare
app/src/Lib/Util/StringUtilities.php
Outdated
| * Convert a foreign key into the full qualified (plugin) model name (eg: Reports.Report). | ||
| * | ||
| * @param string $foreignKey Foreign key name (eg: report_id) | ||
| * @return string Pluralized plugin model name. | ||
| * @since COmanage Registry v5.2.0 | ||
| */ | ||
| public static function foreignKeyToQualifiedModelName(string $foreignKey): string { | ||
| $primaryLinkModelName = StringUtilities::foreignKeyToClassName($foreignKey); | ||
|
|
||
| return self::modelNameToQualifiedModelName($primaryLinkModelName); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to reconcile the comment to what the code actually does. There is no a priori way to extrapolate from a foreign key (eg: report_id) to a fully qualified plugin name. Reports.Report would not be a valid name, it would be something like CoreReporter.Report, though this is a bad example because (the unfinished) Report model is a Pluggable Model, and therefore part of the core code.
It seems like modelNameToQualifiedModelName is trying to find any plugin that implements a model that inflects from this primary key. ie, if I have the key ssh_key_id, then the first plugin that implements SshKeysTable is returned. This will probably work most of the time, but it's not guaranteed. (What if two plugins happen to implement the same foreign key?)
Since this is in the context of Breadcrumbs, I assume what you're trying to do here is map from (eg) MatchServerAttributes (primary key match_server_id) to MatchServers. In this example, if you have access to MatchServerAttributesTable you can examine the primary link on the table, which is CoreServer.match_server_id, which will canonically tell you the correct parent model. But maybe I'm misunderstanding what you're trying to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to reconcile the comment to what the code actually does. There is no a priori way to extrapolate from a foreign key (eg:
report_id) to a fully qualified plugin name.Reports.Reportwould not be a valid name, it would be something likeCoreReporter.Report, though this is a bad example because (the unfinished) Report model is a Pluggable Model, and therefore part of the core code.It seems like
modelNameToQualifiedModelNameis trying to find any plugin that implements a model that inflects from this primary key. ie, if I have the keyssh_key_id, then the first plugin that implementsSshKeysTableis returned. This will probably work most of the time, but it's not guaranteed. (What if two plugins happen to implement the same foreign key?)Since this is in the context of Breadcrumbs, I assume what you're trying to do here is map from (eg) MatchServerAttributes (primary key
match_server_id) to MatchServers. In this example, if you have access toMatchServerAttributesTableyou can examine the primary link on the table, which isCoreServer.match_server_id, which will canonically tell you the correct parent model. But maybe I'm misunderstanding what you're trying to do.
@benno
The original code assumed a model name would always map to a single table, but different plugins can define the same model name and map to different tables.
The updated modelNameToQualifiedModelName() removes the ambiguity by looking at the requester model’s associations to determine the correct (possibly plugin-qualified) table name. If the requester has exactly one primary link, it uses that as the canonical parent and returns it immediately.
This fixes issues surfaced by tabs and nested tabs that rely on context (rather than a single direct association), such as the People View, the Enrollment Flows View, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above
7182820 to
a926451
Compare
@benno pull request updated |
…he context of person_id.
… associations and using physical table names and avoid plugin-scanning guesses
…irst matching and BFS traversal fallback
…le PrimaryLink parent alias
2780bcb to
76d0819
Compare
No description provided.