-
Notifications
You must be signed in to change notification settings - Fork 154
pkp/pkp-lib/issues/6201 Add citationStyleLanguagePlugin support #1121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
||
| {* How to cite *} | ||
| {if $citation} | ||
| {include file="../plugins/generic/citationStyleLanguage/templates/citationblock.tpl"} |
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.
It should be possible to code the plugin to augment OMP without needing to be called from within the OMP code. I'd suggest following the pattern used by the OJS citationStyleLanguage plugin in this regard.
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.
Well, @NateWr told me to do it at this way, see pkp/citationStyleLanguage#84 (comment). Idea was to change it also in OJS. Then there is only one template, used at three places and it's in the plugin it belongs to. But if you want, then I change it.
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.
Right now, the default theme in OJS includes the full template in article_details.tpl. However, the JavaScript that makes it work (articleCitation.js) and the data assigned to $templateMgr all live in the citation style language plugin.
I'm not too bothered either way, but it seemed sensible to bring it all together in one place and reduce the code duplication.
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.
Sorry for sending you in two directions, @nongenti 🥴
The idea is that plugins should be self-contained, i.e. they shouldn't require any changes to the core codebase. This change refers directly to a plugin template from the core codebase, which I'd like to avoid. We're privileged in that we can ship plugins with the software and entangle the core codebase and plugin code as much as we like, but 1) there's not much point in coding it as a plugin if we're integrating part of it anyway, and 2) other (third party) plugins don't have the same privilege and it would be better to set down patterns that the community can use.
We don't have a 100% clear policy on "plugins that alter plugins" -- e.g. a theme plugin that wants to alter the markup generated in this plugin. In my opinion, we should try to avoid the situation because it'll lead to complicated dependencies -- the author of a theme plugin will have to know the development details of the other plugins it interacts with, in addition to OJS.
The "entanglement" between citationStyleLanguage and OJS already existed before this PR, but I like @NateWr's idea of moving the template into this plugin so that less code is duplicated. The thing I'd like to fix is the way the OJS/OMP code calls on it. Rather than using {include} with a path into the plugin's codebase, use the existing Templates::Article::Details hook:
diff --git a/CitationStyleLanguagePlugin.inc.php b/CitationStyleLanguagePlugin.inc.php
index e72ff9b..f71bf89 100644
--- a/CitationStyleLanguagePlugin.inc.php
+++ b/CitationStyleLanguagePlugin.inc.php
@@ -72,10 +72,20 @@ class CitationStyleLanguagePlugin extends GenericPlugin
HookRegistry::register('ArticleHandler::view', [$this, 'getArticleTemplateData']);
HookRegistry::register('PreprintHandler::view', array($this, 'getPreprintTemplateData'));
HookRegistry::register('LoadHandler', [$this, 'setPageHandler']);
+ HookRegistry::register('Templates::Article::Details', [$this, 'addCitationMarkup']);
}
return $success;
}
+ public function addCitationMarkup($hookName, $args) {
+ $params =& $args[0];
+ $smarty =& $args[1];
+ $output =& $args[2];
+
+ $output .= /* ...code to fetch the contents of the citation template goes here... */
+ return false;
+ }
+
/**
* Get list of citation styles available
*...calling on the citation block template that you've now moved into this plugin. (The equivalent OMP hook is Templates::Catalog::Book::Details.)
As for how themes etc. can alter the presentation of citations, as I say, I'm hoping we can avoid substantive changes -- but good markup classnaming will e.g. allow plugins to style citations, which I'm hoping is enough.
(One feature we're missing in either implementation is the ability to reorganize the sidebar content in the article view. At some point we might want to reconcile our article sidebar implementation with our website sidebar blocks implementation, but let's not get into that now.)
@NateWr, I trust your instincts on theme development/maintenance. Does that work OK for you?
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 agree using the hook is best in general. But it will change the order of content in the side area, and I do think people will be unhappy about that. (Maybe it's ok for a major release.) In the past, we've included things we consider "core" plugins right into the template (ORCID, DOI, How to Cite).
The long-term solution is to break the article sidebar components into "blocks" that can be re-ordered. But we've had that on the todo list for years and never got round to it.
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.
Okay, I'll change it to using the hook.
See also: pkp/citationStyleLanguage#84