-
Notifications
You must be signed in to change notification settings - Fork 3
CFM-474_Pipeline_Toolkit_problems #343
base: develop
Are you sure you want to change the base?
Changes from 4 commits
99520df
eba8522
029f1c3
dca9676
5122aba
f79960f
cf95750
6f4bf5d
f9c2ab7
dfd0121
7790c80
55a48d0
520eedd
de17b3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,12 +36,12 @@ | |
| use App\Lib\Util\StringUtilities; | ||
| use Cake\Controller\Controller; | ||
| use Cake\Datasource\Exception\RecordNotFoundException; | ||
| use Cake\Http\Exception\UnauthorizedException; | ||
| use Cake\Event\EventManager; | ||
| use Cake\Http\Exception\UnauthorizedException; | ||
| use Cake\ORM\TableRegistry; | ||
| use Cake\Routing\Router; | ||
| use Cake\Utility\Hash; | ||
|
|
||
|
|
||
| /** | ||
| * @property \App\Controller\Component\RegistryAuthComponent $RegistryAuth | ||
| * @property \App\Controller\Component\BreadcrumbComponent $Breadcrumb | ||
|
|
@@ -273,16 +273,95 @@ public function getCOID(): ?int { | |
| */ | ||
| public function getCurrentTable(): \Cake\ORM\Table | ||
| { | ||
| /** @var string $modelsName */ | ||
| $modelsName = $this->getName(); | ||
| return $this->fetchTable($this->getQualifiedName()); | ||
| } | ||
|
|
||
| $alias = $this->getPlugin() !== null | ||
| ? $this->getPlugin() . '.' . $modelsName | ||
| : $modelsName; | ||
| /** | ||
| * Get the fully-qualified controller/model name with plugin prefix when present. | ||
| * | ||
| * Examples: | ||
| * - Core controller "Users" => "Users" | ||
| * - Plugin controller "MyPlugin.Users" => "MyPlugin.Users" | ||
| * | ||
| * @since COmanage Registry v5.2.0 | ||
| * @return string | ||
| */ | ||
| public function getQualifiedName(): string | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this belong in AppController? It feels more like it belongs in StringUtilities or something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think getQualifiedName belongs in AppController because it returns the fully qualified name of the controller handling the current request. It’s also used by getCurrentTable to resolve the correct table. I understand your point and how it relates to your next comment. To that end, I added the Qualified Name calculation to StringUtilities, but kept this method here to avoid repeatedly accessing the plugin and name values. I find this approach cleaner. |
||
| { | ||
| $name = $this->getName(); | ||
| $plugin = $this->getPlugin(); | ||
|
|
||
| return $plugin !== null && $plugin !== '' | ||
| ? $plugin . '.' . $name | ||
| : $name; | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Check if a given route/URL target is valid and can be resolved. | ||
| * | ||
| * @param array $target Route parameters to validate | ||
| * @return bool True if route is valid and can be resolved | ||
| * @since COmanage Registry v5.2.0 | ||
| */ | ||
| public function isValidRoute(array $target): bool | ||
Ioannis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| try { | ||
| // Normalize plugin nulls and query args | ||
| $params = $target; | ||
| if (array_key_exists('plugin', $params) && $params['plugin'] === null) { | ||
| unset($params['plugin']); | ||
| } | ||
| if (isset($params['?'])) { | ||
| $params['query'] = $params['?']; | ||
| unset($params['?']); | ||
| } | ||
|
|
||
| return $this->fetchTable($alias); | ||
| Router::url($params, true); // will throw if unresolvable | ||
| return true; | ||
| } catch (\Throwable $e) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Check if target corresponds to an active application route (is routable). | ||
| * | ||
| * @param array $target e.g. ['plugin'=>'PipelineToolkit','controller'=>'IdentifierMappers','action'=>'index','?'=>['flange_id'=>3]] | ||
| * @return bool | ||
| * @since COmanage Registry v5.2.0 | ||
| */ | ||
| public function isActiveRoute(array $target): bool | ||
| { | ||
| try { | ||
| // Normalize params for Router::match | ||
| $params = $target; | ||
|
|
||
| // Query params should not affect route matching | ||
| if (isset($params['?'])) { | ||
| unset($params['?']); | ||
| } | ||
| if (isset($params['query'])) { | ||
| unset($params['query']); | ||
| } | ||
|
|
||
| Router::url($target, false); | ||
|
|
||
| $params += [ | ||
| 'plugin' => $params['plugin'] ?? null, | ||
| 'prefix' => $params['prefix'] ?? null, | ||
| 'controller' => $params['controller'] ?? null, | ||
| 'action' => $params['action'] ?? null, | ||
| 'pass' => array_values(array_filter($params, 'is_int', ARRAY_FILTER_USE_KEY)) ?: [], | ||
| ]; | ||
|
|
||
| // If a matching route exists, match() returns an array; otherwise it throws | ||
| Router::getRouteCollection()->match($params, []); | ||
| return true; | ||
| } catch (\Throwable $e) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @param string $potentialPrimaryLink | ||
|
|
||
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.
IdentifierMappers have neither names nor descriptions, so why are we testing for them?
Also we should fail gracefully (ie: return null) if
$entity->flangeis not set.More generally, this seems to be a fairly common pattern, so maybe the parent code in StringUtilities should detect the situation?
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.
Fixed the first. As for the second, you are correct there is a pattern for the models that have no name or description. In general, we probably want to inherit from their parent. I am not sure how we should generalize this yet. @arlen suggested in the past that each model should have a name or description field to facilitate the title construction, etc. I do not think that the StringUtilities is the appropriate place for this.