Bug 5231

Summary: Remove internal author arrays and functions from Submission classes & fix value objects that instantiate DAOs
Product: OJS Reporter: Matthew Crider <mattcrider>
Component: GeneralAssignee: PKP Support <pkp-support>
Status: REOPENED ---    
Severity: normal CC: alec, jerico.dev, juan, pkp-support
Priority: P5    
Version: 2.4.x   
Hardware: PC   
OS: All   
Version Reported In: Also Affects: OCS 2.3.x, OJS 2.3.x

Description Matthew Crider 2010-03-17 11:58:55 PDT
The Submission class contains an array of authors and various functions to maintain this array from within the submission class.  This code is redundant and authors should be maintained exclusively in the AuthorDAO.  For instance, $monograph->getAuthors(), if it exists at all, should be an interface to $authorDao->getAuthorsByMonographId().  Anything necessary to manipulate authors (e.g. getting/setting/deleting/reordering) is already available in the AuthorDAO.

This can be done in OMP without modifying pkp-lib classes, but should be ported to the other apps after the next release.
Comment 2 Juan Pablo Alperin 2010-04-09 19:56:17 PDT
moving to OJS as reminder to backport
Comment 3 Alec Smecher 2010-07-20 12:44:38 PDT
Deferring.
Comment 4 Matthew Crider 2010-10-08 11:01:09 PDT
Need to remove calls to the AuthorDAO from within Monograph.

Consider leaving calls to AuthorDAO in Paper and Article classes to maintain backwards-compatibility (but mark as deprecated).
Comment 5 jerico 2010-11-02 15:40:01 PDT
Also see the comments in bug #5912: value objects like MonographFile and its decendants (i.e. ArtworkFile) and potentially also corresponding classes in other apps (e.g. ArticleFile, etc.) cannot instantiate their own DAO. What's especially ugly is that MonographFile, etc. seem to "instantiate themselves", see getFile(), getFilePath(), etc. which is the wrong way round. The DAO should instantiate the value object and not the value object the DAO to then retrieve itself via it's own ID using a different object instance.

One example: getFilePath()
		$monographDao =& DAORegistry::getDAO('MonographDAO');
		$monograph =& $monographDao->getMonograph($this->getMonographId());
		$pressId = $monograph->getPressId();

We should simply be able to say $this->getPressId() to retrieve the press id rather than having to "instantiate ourselves". If this doesn't work then there's probably a conceptual error elsewhere.
Comment 6 jerico 2010-11-02 15:40:56 PDT
*** Bug 5912 has been marked as a duplicate of this bug. ***
Comment 7 jerico 2010-11-02 15:45:10 PDT
Same for ArtworkFile::getPermissionFile() - Should be populated when retrieving the file from the DAO. See the different descendants of Submission for good examples how this should be done.
Comment 8 jerico 2010-11-02 15:46:57 PDT
Same for OMP's Author::getLocalizedUserGroupName()
Comment 9 jerico 2010-11-02 15:51:00 PDT
Re: getLocalizedUserGroupName() - Should be either getUserGroup() or getUserGroupId() rather than a localized user group name. Maybe this shouldn't even be part of the Author object but be queried from the UserGroupDAO when needed or be some method like UserGroupDAO::getObjectByUserId(), etc. Otherwise we'd have to go on with similar accessors for all dependent entities that happen to be 1:n-related to authors.
Comment 10 Juan Pablo Alperin 2011-03-20 23:45:35 PDT
marking as "also affects" for backporting detection in OCS and OJS.
Comment 11 Juan Pablo Alperin 2011-03-20 23:46:15 PDT
*** Bug 5913 has been marked as a duplicate of this bug. ***
Comment 15 Alec Smecher 2011-11-18 08:02:44 PST
Matt, there's still a case where this isn't working. See https://github.com/pkp/pkp-lib/commit/9d72544cc5d89c592f6707101e1403defa4c69aa#commitcomment-729003 for details.
Comment 16 Matthew Crider 2011-11-23 13:53:59 PST
Alec, is there a document that shows how metadata adapters work?  This should be a one-line fix, but I have no idea how to reproduce the issue to confirm the fix.
Comment 17 Alec Smecher 2011-11-23 13:58:03 PST
It's not clear from the stack trace how it's being invoked; might as well follow up with Florian on the github comment. The filter stuff is part of the metadata framework, documented at <http://pkp.sfu.ca/wiki/index.php/Metadata_and_Filter_Framework>.