pkp/pkp-lib#1550 Controlled vocabulary support #10833
Conversation
jyhein
commented
Jan 22, 2025
jyhein
commented
- Use external source to add new keywords etc. using plugins
- Save extra data (identifier, source) with the term to db
There was a problem hiding this comment.
@jyhein I have added few reviews, most of them are questions/suggestions . Please take a look .
api/v1/vocabs/PKPVocabController.php
Outdated
| foreach ($entries as $entry) { | ||
| $data[] = $entry->getLocalizedData('name', $locale); | ||
| } | ||
| $data = $entries |
There was a problem hiding this comment.
As we are directly attaching the map assuming it's always a Collection but what if it's not a collection ? perhaps better to wrap this to Collection::wrap .
| @@ -27,6 +27,10 @@ | |||
| class ControlledVocabEntry extends Model | |||
| { | |||
| use ModelWithSettings; | |||
|
|
|||
| public const CONTROLLED_VOCAB_ENTRY_TERM = 'term'; | |||
There was a problem hiding this comment.
what is difference between the term and name ? I see that in several place we are just replacing the name with term , however it seems they represent same information like
// calling as ControlledVocabEntry::find(130)
PKP\controlledVocab\ControlledVocabEntry {#1561
controlled_vocab_entry_id: 130,
controlled_vocab_id: 38,
seq: 2.0,
identifier: "http://www.yso.fi/onto/koko/p62290",
source: "finto",
name: [
"en" => "development aids",
],
}
calling as ControlledVocabEntry::find(130)->->getEntryData()
array:3 [
"identifier" => "http://www.yso.fi/onto/koko/p62290"
"source" => "finto"
"term" => array:1 [
"en" => "development aids"
]
]
if there is actually no difference, then can we use only name which is allow us to not to introduce another keyword to work with ? Or are there some other reason that I am missing ?
There was a problem hiding this comment.
Before the db was different but I kept 'term' now because outside of db it sounded more descriptive than 'name'. No other reason. I'll change it if term is not good
| ], | ||
| ...is_array($vocab) | ||
| ? Arr::whereNotNull(Arr::only($vocab, $controlledVocabEntry->getSettings())) |
There was a problem hiding this comment.
As we are already storing the settings field above in $controlledVocabEntrySettings, we can use that instead of calling the method each time .
There was a problem hiding this comment.
indeed, forgot to use it...
| "type": "object", | ||
| "properties": { | ||
| "term": { | ||
| "type": "string" |
There was a problem hiding this comment.
same as above as if term and name represent same thing and we are already using name, can you use name here and everywhere ?
| foreach ($entry->name as $locale => $value) { | ||
| $result[$locale][] = $value; | ||
| $result[$locale][] = $asEntryData ? $entry->getEntryData($locale) : $value; |
There was a problem hiding this comment.
Just wondering, what are the chances to standardise the output to one single format , so with $asEntryData set to true, we get
[
"en" => [
[
"identifier" => "http://www.yso.fi/onto/koko/p36127",
"source" => "finto",
"term" => "timber construction",
],
[
"identifier" => "http://www.yso.fi/onto/koko/p62290",
"source" => "finto",
"term" => "development aids",
],
[
"term" => "test",
],
],
]
and $asEntryData set to false, we get
[
"en" => [
"timber construction",
"development aids",
"test",
],
]
if we try to standardise it as
[
"en" => [
['name' => "timber construction"],
['name' => "development aids"],
['name' => "test"],
],
]
what about that ? or perhaps that will be too much changes as I guess need to port that changes to a lot of places .
There was a problem hiding this comment.
what about that ? or perhaps that will be too much changes as I guess need to port that changes to a lot of places .
I was thinking this too
| @@ -46,7 +46,8 @@ public function getBySymbolic( | |||
| string $symbolic, | |||
| int $assocType, | |||
| ?int $assocId, | |||
| ?array $locales = [] | |||
| ?array $locales = [], | |||
| bool $asEntryData = false | |||
There was a problem hiding this comment.
for boolean param , anything not passes should be by default to true and only when need to adjust, a visible change should be require . Also this make it hard to understand at implementation end what it means . How about a enum or simple const with default not to set as entry data ?
| return null; | ||
| } | ||
|
|
||
| $attributes[self::CONTROLLED_VOCAB_ENTRY_TERM] = $attributes['name']; |
There was a problem hiding this comment.
As pointed above, can we use name instead of term if they represent same thing ?
| public const CONTROLLED_VOCAB_ENTRY_IDENTIFIER = 'identifier'; | ||
| public const CONTROLLED_VOCAB_ENTRY_SOURCE = 'source'; |
There was a problem hiding this comment.
Any particular reason we are defining these core settings columns as const here ?
There was a problem hiding this comment.
OK, where would be a better place?
There was a problem hiding this comment.
I am thinking that just use these as settings columns in the getSettings method and not define these as const . Just wondering is there any chance to specifically use the fields by any plugin or have any special use later ? Better to use const when each of that information need to be specifically use for special purpose multiple time .
There was a problem hiding this comment.
Plugins should use these when creating suggestions for the fields. Downstream services may use 'identifier' (e.g. when it's url) but probably don't have use for 'service'. Also at some point in the future there could be more data added than just 'name', 'identifier', and 'source'. Currently core doesn't use identifier and source separately but that may change in the future
There was a problem hiding this comment.
In that case we can leave it as it is for now .
jyhein
force-pushed
the
f1550
branch
3 times, most recently
from
8d64a18 to
1c2fd45
Compare
