Open jnugent opened 3 years ago
Thanks @jnugent. The refactor is designed to take the fetching from the database out of the DataObject
, since that can lead to lots of duplicate lookups. But we do need to get this back in. I think the data should be there in the authors array, so we should be able to filter that alright.
+1 from PKP|PS on this one and I can confirm it's still an issue in 3.3.0.8
As @NateWr suggested, it's available in the author array, so this is actually a pretty quick fix.
PRs:
@NateWr, assuming these pass, could you have a quick look? It's a one-liner.
PRs are against stable-3_3_0
-- this will need cherry-picking forward.
Publication::getAuthorString()
is used in several places, not just in browse lists. I think some care would need to be taken to ensure that this didn't have a negative impact on a stable branch. Here are all the places I find it used in OJS stable-3_3_0
:
classes/search/ArticleSearch.inc.php
80,29: $orderKey = $submission->getAuthorString();
classes/submission/Submission.inc.php
57,64: $fieldValue = array($context->getPrimaryLocale() => $this->getAuthorString());
lib/pkp/classes/components/forms/publication/PKPPublicationLicenseForm.inc.php
44,29: $copyright = $publication->getAuthorString($userGroups);
lib/pkp/classes/mail/SubmissionMailTemplate.inc.php
57,45: 'authorString' => strip_tags($submission->getAuthorString()),
lib/pkp/classes/services/PKPPublicationService.inc.php
179,36: $values[$prop] = $publication->getAuthorString($args['userGroups']);
lib/pkp/classes/submission/PKPSubmission.inc.php
405,22: return $publication->getAuthorString($userGroups);
lib/pkp/controllers/modals/editorDecision/form/EditorDecisionWithEmailForm.inc.php
73,31: 'authorName' => $submission->getAuthorString(),
87,31: 'authorName' => $submission->getAuthorString(),
322,32: 'authorName' => $submission->getAuthorString(),
348,42: 'authorName' => strip_tags($submission->getAuthorString()),
lib/pkp/controllers/modals/editorDecision/form/SendReviewsForm.inc.php
81,32: 'authorName' => $submission->getAuthorString(),
lib/pkp/controllers/modals/submission/ViewSubmissionMetadataHandler.inc.php
50,48: $templateMgr->assign('authors', $publication->getAuthorString($userGroups));
plugins/generic/webFeed/templates/rss2.tpl
67,27: <dc:creator>{$article->getAuthorString(false)|escape:"html"}</dc:creator>
plugins/themes/pragma/templates/frontend/objects/article_summary.tpl
31,33: <p class="metadata">{$article->getAuthorString()|escape}</p>
templates/frontend/objects/article_summary.tpl
56,13: {$article->getAuthorString()|escape}
Of those, the use in the license (APP\submission\Submission::57
) is probably the biggest risk. But maybe also in the EditorDecisionWithEmailForm
and SendReviewsForm
.
In the main
branch, this has been addressed by adding a flag to getAuthorString()
. See here. But it doesn't look like it's been applied in all of the right places. I think the right places would be article_summary.tpl
and rss2.tpl
. But there may be some other uses that qualify (ArticleSearch
?).
Thanks, @NateWr, I moved too quickly on what I thought was a one-liner.
I've taken a look over it and reworked this for both stable-3_3_0
and main
branches:
PRs:
The changes...
Publication::getAuthorString
function used in main
and stable-3_3_0
stable-3_3_0
, fixes the deprecated Submission::getAuthorString
function; in main
, removes it (it's been deprecated since 3.2.0).Publication::getAuthorString
easier to use (e.g. when replacing calls to Submission::getAuthorString) by making the user group parameter optionalSubmission::getAuthorString
to use the non-deprecated Publication::getAuthorString
insteadgetAuthorString
-- for example rss2.tpl
noted above; I also did change ArticleSearch
, which is used for search sort ordering, even though that's unlikely to matter.The other cases are fine, I think.
This is a little more than I had hoped to commit to a stable branch, but worth the trouble IMO.
Could you have another look? If that's OK I'll port to the other apps.
The removal of $submission->getAuthorString
in favour of $submission->getCurrentPublication()->getAuthorString()
should probably be noted in the release documentation. In most cases it'll be a straight-across swap.
The code looks good. A couple of thoughts about the changes and why I chose before not to bring some features over to Publication::getAuthorString()
.
Re-adds the option that was previously available to choose whether or not to use the preferred public name
I don't think we ever want to not use the preferred public name with this method, do we? My understanding is that if someone has supplied us with a preferred public name, we should always use it when compiling the author string. The only time we want to not use the preferred public name is when preparing information for a deposit that requires a first/last distinction. I think this flag is only used in rss2.tpl
, but even there we should probably use the preferred name.
Makes
Publication::getAuthorString
easier to use (e.g. when replacing calls toSubmission::getAuthorString
) by making the user group parameter optional
I chose not to do this in Publication::getAuthorString()
for performance reasons. The method, Submission::getAuthorString()
is commonly used in templates, and often used again and again on the same page. Retrieving the user groups from the database every time is bad practice. (For example, an issue table of contents that lists 20 articles may fetch the context's user groups 20 times.)
So when I deprecated that method and wrote Publication::getAuthorString()
, the idea was to eventually force templates to migrate away from this bad practice. That's why I chose to require that the user groups are passed in. I still think, in the long-term, that's a good practice to force, even though it will cause some hurdles for theme developers. Maybe 3.4.0 is the right time to do away with Submission::getAuthorString()
, especially if 3.3 becomes the LTS version.
I don't think we ever want to not use the preferred public name with this method, do we? My understanding is that if someone has supplied us with a preferred public name, we should always use it when compiling the author string. The only time we want to not use the preferred public name is when preparing information for a deposit that requires a first/last distinction. I think this flag is only used in
rss2.tpl
, but even there we should probably use the preferred name.
The RSS feed (and other services) were changed to specifically avoid using the preferred public name with this commit: https://github.com/pkp/ojs/commit/7b1707e2326c5536270cd2f03d0459e2aae70b6f#diff-31f40756c38c3d73c69d835b525a3d7ac6882045dc09fbc75c6e9a7fa8a5ed5d Bozana, do you remember the context around this?
I chose not to do this in
Publication::getAuthorString()
for performance reasons.
Ah, I remember this conversation. I was going to argue against forcing optimization concerns up in the stack, particularly into the templates, but actually cleaning this up per your suggestion did improve a lot of wasteful re-fetching of the user group list. I've adjusted this per your original idea. I do intend to remove the deprecated Submission::getAuthorString()
function when I forward-port this to main
.
(Sorry, forgot to tag you by username on the above comment, @bozana!)
@NateWr, could you have another look at the stable-3_3_0
PRs? I've changed them to avoid fetching the user groups in the getAuthorString
function, per your recommendation.
@bozana, do you remember the context around https://github.com/pkp/ojs/commit/7b1707e2326c5536270cd2f03d0459e2aae70b6f#diff-31f40756c38c3d73c69d835b525a3d7ac6882045dc09fbc75c6e9a7fa8a5ed5d -- why it was important not to rely on the preferred name, when provided, in these cases?
Sorry to be so late -- somehow I first now see this :( I cannot remember it well :-( I've just seen these comments from the issue back then: s. No 13) here https://github.com/pkp/pkp-lib/issues/2958#issuecomment-387148988 and https://github.com/pkp/pkp-lib/issues/2958#issuecomment-387353098. So I think there was maybe not a very specific reason, we thought it would be better so I would say, maybe because usually the 3. party services would like to have the names in a specific way if possible... :thinking:
Describe the bug In OJS 3.1.2 the
getAuthorString()
method used to pass a booleantrue
togetAuthors
which respected the$includeInBrowse
flag. This was useful when displaying an author string on a front end template, like an issue's table of contents. That can be seen here:https://github.com/pkp/pkp-lib/blob/stable-3_1_2/classes/submission/Submission.inc.php#L257
With the refactor to
Publications
beginning in 3.2, thegetAuthorString()
method in thePKPSubmission
class just hands off that call to thePublication
object, here:https://github.com/pkp/pkp-lib/blob/stable-3_2_1/classes/submission/PKPSubmission.inc.php#L382
But the
Publication
object now just fetches the authors property:https://github.com/pkp/pkp-lib/blob/stable-3_2_1/classes/publication/PKPPublication.inc.php#L137
I'm not quite sure if it's still possible to filter by
$includeInBrowse
, at least on the front end. This seems like a potentially complicated refactor, but perhaps the templates can iterate over the authors instead of calling this method?We had a hosted client use the Quick Submit plugin to populate an issue in 3.2 and noticed this behaviour.
What application are you using? OJS 3.2.1-4 (probably also in 3.3)