pkp/pkp-lib#13221 Implement crossref's citedBy - #108
taslangraham wants to merge 2 commits into
Conversation
taslangraham
changed the title
taslangraham
changed the title
taslangraham
changed the title
a2e70c3 to
65782b5
Compare
6925927 to
d25433a
Compare
bozana
left a comment
bozana
left a comment
There was a problem hiding this comment.
Hi @taslangraham, here a few comments! Thanks a lot! Great work!
| */ | ||
| public function registerEnabledHooks(): void | ||
| { | ||
| Hook::add('Templates::Article::Details::Metrics::CitedBy', $this->displayCitedByComponent(...)); |
There was a problem hiding this comment.
To double check with Devika and Jarda, but here the hook Templates::Article::Footer::PageFooter could maybe be used for default theme.
There was a problem hiding this comment.
Were you able to confirm?
| public function registerEnabledHooks(): void | ||
| { | ||
| Hook::add('Templates::Article::Details::Metrics::CitedBy', $this->displayCitedByComponent(...)); | ||
| Hook::add('Templates::Article::Metrics::CitedByCount', $this->displayCitedByCountComponent(...)); |
There was a problem hiding this comment.
Hmmm... this I do not know where would best be displayed in the default theme... maybe no need at all...
There was a problem hiding this comment.
I went ahead and removed it.
|
|
||
| return $success; | ||
| } | ||
|
|
There was a problem hiding this comment.
maybe to leave this blank line here :-)
| ], | ||
| 'citedBy' => (object) [ | ||
| 'type' => 'boolean', | ||
| ], |
There was a problem hiding this comment.
would it maybe be an easy way to require username+password if this field is enabled?
Would something like this work or be enough:
'username' => (object) [
'type' => 'string',
'validation' => ['nullable', 'max:120', 'required_if:citedBy,true'],
],
'password' => (object) [
'type' => 'string',
'validation' => ['nullable', 'max:50', 'required_if:citedBy,true'],
],
?
5e82ed0 to
9c989d4
Compare
e5dee28 to
ed7814e
Compare
ed7814e to
a87940e
Compare
bozana
left a comment
bozana
left a comment
There was a problem hiding this comment.
Hi @taslangraham, a few comments. I wanted also to test it but have not managed yet.
Thanks a lot!
|
|
||
| $templateMgr->addJavaScript( | ||
| 'CrossrefCitedByBody', | ||
| "{$request->getBaseUrl()}/{$this-> plugin->getPluginPath()}/public/build/crossref.js", |
There was a problem hiding this comment.
Why do we add java script 4 times in this function?
hmmm... Crossmark adds it too...
Is requiresVueRuntime() as in Crossmark case needed?
Also, note that there is a space after $this-> in several places here
| --}} | ||
| <section class="item cited-by" data-vue-root> | ||
| <pkp-cited-by v-bind='{$citedByConfig|json_encode}'></pkp-cited-by> | ||
| </section> |
There was a problem hiding this comment.
This looks like a smarty template syntax? Shouldn't it be something similar to crossmarkButton.blade?
| --}} | ||
| <div class="item cited-by-count" data-vue-root> | ||
| <pkp-cited-by-count v-bind='{$citedByConfig|json_encode}'></pkp-cited-by-count> | ||
| </div> |
There was a problem hiding this comment.
This looks like a smarty template syntax? Shouldn't it be something similar to crossmarkButton.blade?
| } | ||
|
|
||
| /** @var Submission $article */ | ||
| $article = &$params[0]; |
There was a problem hiding this comment.
With the hook used I believe we need this:
$displayedArticle = $smarty->getTemplateVars('article');
| public function setupCitedByComponents(string $hookName, array $params): bool | ||
| { | ||
| $request = $params[0]; | ||
| $isCitedByEnabled = (bool)$this->plugin->getSetting($this->plugin->getCurrentContextId(), 'citedBy'); |
There was a problem hiding this comment.
I think here and in the Controller one and the same function should be used to determine isEnabled. Else, the journal could change the registration agency but leave the crossref plugin enabled (with enabled citedBy).
Maybe to have it as an extra function in this class, that checks the configured registration agency, the setting citedBy and the credentials ?
| citation.year, | ||
| citation.volume, | ||
| citation.authors, | ||
| citation.pages, |
| case 'conf_cite': | ||
| $result['title'] = $item->{$type}->volume_title ? (string)$item->{$type}->volume_title : null; | ||
| $result['journal'] = $item->{$type}->series_title ? (string)$item->{$type}->series_title : null; | ||
| $result['componentNumber'] = $item->{$type}->component_number ? (int)$item->{$type}->component_number : null; |
There was a problem hiding this comment.
also here I would use string and not int
| if ($citeType === 'msg') { | ||
| // The msg cite type contains a forward link to other cite types. | ||
| // So we recursively get those citations as well. | ||
| // See https://data.crossref.org/reports/help/schema_doc/crossref_query_output2.0/2_0.html#msg |
There was a problem hiding this comment.
Hmmm... I think msg is just a string and sub-element of forward_link or query element, so I do not think this is needed here?
| 'report_cite', | ||
| 'standard_cite' | ||
| ]; | ||
| } |
There was a problem hiding this comment.
This is very difficult to maintain -- if something changes and new types are added...
Could extractCitationsFromXMLList() instead take any child element of forward_link whose name ends in _cite, and read its no matter what the type is? 🤔
| * | ||
| * Copyright (c) 2026 Simon Fraser University | ||
| * Copyright (c) 2026 John Willinsky | ||
| * Distributed under the GNU GPL v3. For full terms see the file docs/COPYING. |
There was a problem hiding this comment.
somehow for crossref plugin we use MIT license, so maybe to apply everywhere:
Distributed under The MIT License. For full terms see the file LICENSE.
2 participants
For pkp/pkp-lib#13221 & pkp/dev-team/issues/316