pkp/pkp-lib/issues/6201 Adapt to support OMP #84
Conversation
|
I also add DOI support for this plugin in OMP. pkp/omp#1035 |
NateWr
left a comment
NateWr
left a comment
There was a problem hiding this comment.
Looks good! I've left some comments, mostly syntax or style stuff, but a few more details to work out. I haven't yet done a functionality test, but I'll do that for the next round of review.
CitationStyleLanguagePlugin.inc.php
Outdated
| 'priority' => STYLE_SEQUENCE_LAST, | ||
| 'contexts' => array('frontend'), | ||
| 'inline' => false, | ||
| ) |
There was a problem hiding this comment.
Use bracket array syntax:
[
...
]
CitationStyleLanguagePlugin.inc.php
Outdated
| * @return string | ||
| */ | ||
| public function getCitation($request, $article, $citationStyle = 'apa', $issue = null, $publication = null) | ||
| public function getCitation($request, Submission $submission, string $citationStyle = 'apa', ?Issue $issue = null, ?Publication $publication = null, ?Chapter $chapter = null) |
There was a problem hiding this comment.
I think that the type hints here will fail because these classes have not been imported with a use statement.
CitationStyleLanguagePlugin.inc.php
Outdated
| $authors = $publication->getData('authors'); | ||
| $authorsGroup = $this->getAuthorGroup($context->getId()); | ||
| $editorsGroup = $this->getEditorGroup($context->getId()); | ||
| $translatorsGroup = $this->getTranslatorGroup($context->getId()); |
There was a problem hiding this comment.
Each of these variable names should be plural, xxxxxxGroups. The methods too.
CitationStyleLanguagePlugin.inc.php
Outdated
| $currentAuthor->given = htmlspecialchars($author->getLocalizedGivenName()); | ||
| } | ||
| $citationData->author[] = $currentAuthor; | ||
| $userGroup = $author->getUserGroup(); |
There was a problem hiding this comment.
This could be slow, especially on publications with lots of authors, because it makes a separate DB request for each author. Instead, you can use $currentAuthor->getUserGroupId() to check which group to put them in.
readme.md
Outdated
|
|
||
| ## Compatibility | ||
| This plugin is compatible with OJS 3.1.x. | ||
| This plugin is compatible with OJS 3.4.x. and OMP 3.4.x |
There was a problem hiding this comment.
@asmecher do you remember what the release strategy was for this plugin? I notice there is some code (see CitationStyleLanguageHandler::117) that handles 3.1.x compatibility. Is the idea to keep the main branch compatible with as many versions as possible?
There was a problem hiding this comment.
It's released along with OJS/OMP/OPS, so I'd rather we just targeted the main branch here. Our current "best practice" is to have branches for each plugin aligned with the equivalent branch in OJS/OMP/OPS; for example, [staticPages has branches(https://github.com/pkp/staticPages/branches) called main, stable-3_3_0, stable-3_2_1`, etc. This saves us having to remember what changed between versions, and avoids code bloat related to backwards compatibility.
templates/citation-styles/ris.tpl
Outdated
| TY - JOUR | ||
| {assign var="collectionTitle" value="collection-title"} | ||
| {assign var="collectionEditor" value="collection-editor"} | ||
| {assign var="publisherPlace" value="publisher-place"} |
There was a problem hiding this comment.
It looks like these three {assign ...} variables are not used?
There was a problem hiding this comment.
collectionTitle is used in line 57 & 58,
collectionEditor is used in line 27, 28, 37 & 38,
publisherPlace is used in line 63.
templates/citation-styles/ris.tpl
Outdated
| {/if} | ||
| {if $citationData->abstract} | ||
| AB - {$citationData->abstract|replace:"\r\n":""|replace:"\n":""} | ||
| {/if} |
There was a problem hiding this comment.
Have you tested the RIS output? I think some of the new conditional tags may result in empty lines. I'm not sure if that is valid in RIS or not.
There was a problem hiding this comment.
There shouldn't be empty lines in the RIS output. Can you give me an example, please?
templates/citationblock.tpl
Outdated
| </div> | ||
| </section> | ||
| </div> | ||
| {/if} |
There was a problem hiding this comment.
We'll want to use the same template in OJS and OMP by default. And most custom themes implement their own HTML code. So instead of hooking this to the ::details hooks on the book/article pages, let's load it directly from the default theme templates.
So where the how-to-cite block is loaded in the article-details.tpl file in OJS, you'll want to do something like:
{if $citation}
{include file="path/to/this/file.tpl"}
{/if}
You'll want to do the same thing in the default theme for OMP and OJS.
There was a problem hiding this comment.
(More discussion about this here: pkp/omp#1121 (comment))
CitationStyleLanguagePlugin.inc.php
Outdated
| 'publicationId' => $publication->getId(), | ||
| ]; | ||
| if ($chapter) { | ||
| $citationArgs['chapterId'] = $chapter->getSourceChapterId(); |
There was a problem hiding this comment.
What was your thinking for passing the source chapter ID instead of the chapter ID? It looks to me like when receiving the request you have to get all chapters related to a source and find the correct chapter anyway, so it seems like it would be easier to just pass the chapter ID.
I'm talking about CitationStyleLanguageHandler::136.
There was a problem hiding this comment.
I can't remember, what was my thinking, but it seems you're right. I change this.
asmecher
changed the title
CitationStyleLanguagePlugin.inc.php
Outdated
| public $_citationDownloads = []; | ||
|
|
||
| /** @var bool $applicationOmp */ | ||
| private bool $applicationOmp; |
There was a problem hiding this comment.
I would recommend using a private string $application, which is initialized from Application::get()->getName(). Then, when you need to implement different behaviour by application:
switch ($this->application) {
case 'ojs2':
// Do something OJSy
break;
case 'omp':
// Do something OMPy
break;
default: throw new Exception('Unknown application!');
}This will be future-proof if we later add OPS support to the plugin.
CitationStyleLanguagePlugin.inc.php
Outdated
| $publication = $args[3]; | ||
| if ($this->isApplicationOmp()) { | ||
| $submission =& $args[1]; | ||
| $publication = $request->_router->_handler->publication; |
There was a problem hiding this comment.
This is working around a situation where the hook should clearly be providing more data to its registrants (as the comparable OJS hook already does). If you can open a PR to OMP main to add this to the hook call, I can get it merged. This will be a better long-term outcome than having this plugin find the handler through a work-around; we'll be rolling out explicit public / protected / private declarations where they are currently missing, and anything prefixed with _ is going to get a public or private.
(Also for chapter below.)
There was a problem hiding this comment.
PR to add publication and chapter to hook call CatalogBookHandler::book is open now: pkp/omp#1245
| /** @var Submission article being requested */ | ||
| public $article = null; | ||
| /** @var null|Submission $submission being requested */ | ||
| public ?Submission $submission = null; |
There was a problem hiding this comment.
Just to clarify why it's working without use statements -- we've moved everything into namespaces starting with 3.4, but for backwards compatibility, we have class_alias statements to "export" these into the root namespace. So currently it's possible to refer to either \Submission or \APP\submission\Submission and they are equivalent.
Going forward, I'd encourage coders to move everything into namespaces -- including plugins. (See e.g. https://github.com/pkp/staticPages, https://github.com/pkp/customBlockManager, https://github.com/pkp/tinymce, etc.) Once a plugin gets moved into a namespace, its code will no longer live in the root namespace, so referring to e.g. Submission without a use statement will cause an error; that's a good time to add an explicit use APP\submission\Submission statement.
In a few major releases, once this transition is complete, we'll remove the class_alias statements.
(I'd be happy to convert this plugin over to including namespace support after this gets merged!)
| */ | ||
| public function __construct() { | ||
| parent::__construct(); | ||
| $this->plugin = PluginRegistry::getPlugin('generic', 'citationstylelanguageplugin'); |
There was a problem hiding this comment.
No need to change this right now, but just FYI, there are some new changes available in 3.4 that allow for the handler to be given the plugin as part of the constructor. Plugin code / handler code
readme.md
Outdated
|
|
||
| ## Compatibility | ||
| This plugin is compatible with OJS 3.1.x. | ||
| This plugin is compatible with OJS 3.4.x. and OMP 3.4.x |
There was a problem hiding this comment.
It's released along with OJS/OMP/OPS, so I'd rather we just targeted the main branch here. Our current "best practice" is to have branches for each plugin aligned with the equivalent branch in OJS/OMP/OPS; for example, [staticPages has branches(https://github.com/pkp/staticPages/branches) called main, stable-3_3_0, stable-3_2_1`, etc. This saves us having to remember what changed between versions, and avoids code bloat related to backwards compatibility.
templates/citationblock.tpl
Outdated
| </div> | ||
| </section> | ||
| </div> | ||
| {/if} |
There was a problem hiding this comment.
(More discussion about this here: pkp/omp#1121 (comment))
CitationStyleLanguagePlugin.php
Outdated
| * @brief Citation Style Language plugin class. | ||
| */ | ||
|
|
||
| namespace APP\plugins\generic\citationStyleLanguage; |
There was a problem hiding this comment.
👍 Happy to see all this code modernization going in at the same time!
CitationStyleLanguagePlugin.php
Outdated
| /** @var string Name of the application */ | ||
| public string $application; | ||
|
|
||
| private bool $book = false; |
There was a problem hiding this comment.
These names could be confused for the actual objects rather than booleans; I'd suggest renaming these to e.g. isBook, and getting rid of the getter functions; we're using fewer getter/setter methods lately, as they don't add much and take up a lot of space. And these are private anyway.
CitationStyleLanguagePlugin.php
Outdated
| /** | ||
| * Get the primary style name or default to the first available style | ||
| * | ||
| * @param $contextId integer Journal ID |
There was a problem hiding this comment.
This applies to this line and a few others below:
We're relying less on phpdoc-style self-documentation and using more typehints recently. Since this is an application-independent repository, we shouldn't be referring to OJS-specific terminology, so this comment should read "Context ID" -- but that adds nothing to the variable name :) so I'd suggest getting rid of the @param line entirely and adding a typehint to the function parameter.
Self-documentation for parameters and returns is fine when it adds value, but often it doesn't.
I don't think it's possible for this code to get called when $contextId is 0, so I'd suggest removing the default value entirely and also up the check at https://github.com/pkp/citationStyleLanguage/pull/84/files#diff-13b0240e88db06d12f97fde1af2328104d06b03bdab4c6b2b5e7bfa949e9be1aR350, assuming instead that $context will always exist as it should. I'd rather have a fatal error in a supposedly-unrunnable case than have it magically transform an ID into a 0; we're getting stricter about this in the codebase now that we're e.g. using referential integrity in the database.
(It is possible for article summaries to be displayed where $contextId is 0 -- e.g. in the site-wide search results page. But that's just the article summary, not the article view.)
(Sorry -- this is getting long! -- but if there was a valid case with $contextId = 0, then I would suggest instead using the constant CONTEXT_SITE from PKPApplication.php.)
CitationStyleLanguagePlugin.php
Outdated
| if ($chapter->getDoi()) { | ||
| $citationData->DOI = $chapter->getDoi(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The else case here should never be executed, right? If not, I'd suggest adding a
} else {
throw new Exception('Unknown submission content type!');
}
...or equivalent.
CitationStyleLanguagePlugin.php
Outdated
| return false; | ||
| } | ||
|
|
||
| if (!$publication) { |
There was a problem hiding this comment.
I think this if is new; shouldn't the $publication always be provided? (This is another example of something we're working to tidy up -- if we know something is always provided, a Publication $publication typehint will prove it; if it's nullable, a Publication ?$publication will document it.)
CitationStyleLanguagePlugin.php
Outdated
| public function setPageHandler($hookName, $params) | ||
| { | ||
| $page =& $params[0]; | ||
| $handler =& $params[3]; |
There was a problem hiding this comment.
Again, bonus points for modernizing this!
CitationStyleLanguagePlugin.php
Outdated
| if ($seriesId) { | ||
| /** @var SeriesDAO $seriesDao */ | ||
| $seriesDao = DAORegistry::getDAO('SeriesDAO'); | ||
| if (null !== $seriesDao) { |
There was a problem hiding this comment.
The $seriesDao will always be set, so no need for the null check. (In OJS, trying to get the SeriesDAO will cause a fatal error.)
CitationStyleLanguagePlugin.php
Outdated
| } elseif (isset($args[1], $args[3], $args[4]) && $args[1] === 'version' && $args[3] === 'chapter') { | ||
| $key = 4; | ||
| } else { | ||
| $key = 0; |
There was a problem hiding this comment.
I'd suggest a return null right away rather than continuing below. Saves some indentation, and keeps "invalid" code paths as short as possible.
CitationStyleLanguagePlugin.php
Outdated
| $chapterId = (int) $args[$key]; | ||
| if ($chapterId > 0) { | ||
| $chapterDao = DAORegistry::getDAO('ChapterDAO'); | ||
| $chapters = $chapterDao->getBySourceChapterId($chapterId); |
There was a problem hiding this comment.
I think this might be wrong -- the source chapter ID will be (if I remember right) the ID of the chapter from a previous version (publication), no? I think this should be $chapterDao->getChapter($chapterId, $publication->getId() (which does the publication ID check at the same time for free).
There was a problem hiding this comment.
The source chapter ID is the ID from the first version of a chapter. In the url for a chapter landing page we use always the source chapter ID. This is important for DOI registration of a chapter. So frontend useres should never see an other chapter ID as the source chapter ID. So we get from $request->getRequestedArgs() always the source chapter ID.
Maybe it would be better to change the function getBySourceChapterId($chapterId) to take a optional second argument publication ID. Then we could put this code in ChapterDAO.
There was a problem hiding this comment.
Ah! I'd forgotten about this element of OMP. You're right, the source chapter ID is the correct thing to use. Yes, if you don't mind, please do propose a change to add the publication ID to getBySourceChapterId as an optional second parameter, thanks!
templates/settings.tpl
Outdated
| {fbvFormSection list=true label="plugins.generic.citationStyleLanguage.settings.citationChooseTranslator"} | ||
| <p>{translate key='plugins.generic.citationStyleLanguage.settings.citationOptionChooseTranslator'}</p> | ||
| {foreach from=$allUserGroups item="group" key="id"} | ||
| {fbvElement type="checkbox" id="groupTranslator[]" value=$id checked=in_array($id, $groupTranslator) label=$group translate=false} |
There was a problem hiding this comment.
This might eventually be something we need elsewhere in the application! But for now let's keep it here.
|
I think all is done now. |
|
@nongenti, marvelous! But can you rebase from |
3 participants
Hi,
like we discuss a long time ago (#79) I add OMP support to csl main branch now. It supports book and chapter pages.