PKP Bugzilla – Bug 5231
Remove internal author arrays and functions from Submission classes & fix value objects that instantiate DAOs
Last modified: 2011-12-12 09:18:55 PST
We are moving to Git Issues for bug tracking in future releases. During transition, content will be in both tools. If you'd like to file a new bug, please create an issue.
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.
Commits that will help backport this to OJS and OCS:
moving to OJS as reminder to backport
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).
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.
*** Bug 5912 has been marked as a duplicate of this bug. ***
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.
Same for OMP's Author::getLocalizedUserGroupName()
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.
marking as "also affects" for backporting detection in OCS and OJS.
*** Bug 5913 has been marked as a duplicate of this bug. ***
Posted to my github for review:
Second pass, removed some convenience methods from Submission class:
Committed to official. Closing.
Matt, there's still a case where this isn't working. See https://github.com/pkp/pkp-lib/commit/9d72544cc5d89c592f6707101e1403defa4c69aa#commitcomment-729003 for details.
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.
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>.