PKP Bugzilla – Full Text Bug Listing
|Summary:||Remove internal author arrays and functions from Submission classes & fix value objects that instantiate DAOs|
|Product:||OJS||Reporter:||Matthew Crider <mattcrider>|
|Component:||General||Assignee:||PKP Support <pkp-support>|
|Severity:||normal||CC:||alec, jerico.dev, juan, pkp-support|
|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 1 Matthew Crider 2010-03-19 12:53:49 PDT
Commits that will help backport this to OJS and OCS: http://github.com/pkp/omp/commit/15e78686b4737dc4a1f6a50d927452c24f277fb9 http://github.com/pkp/omp/commit/7df49ecd3414157ff6e25f177a4169afa41d4899
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
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 12 Matthew Crider 2011-06-03 10:44:09 PDT
Posted to my github for review: https://github.com/mcrider/ocs/commit/0e04f03effde19ed65a6a6c5a6e2aa829fe3e0c0 https://github.com/mcrider/ojs/commit/7c9240104d100b7adb3adc3cbaf4b575dbc0cd82 https://github.com/mcrider/omp/commit/f489fd247f685e85d97fab19d85dbe011ccd8e59 https://github.com/mcrider/pkp-lib/commit/4b61a74861274103c2d18a20540b47362fa8843e
Comment 13 Matthew Crider 2011-06-03 14:49:23 PDT
Second pass, removed some convenience methods from Submission class: https://github.com/mcrider/pkp-lib/commit/731d395cb2f895254c692aec533429a0ccfeae08 https://github.com/mcrider/omp/commit/f489fd247f685e85d97fab19d85dbe011ccd8e59 https://github.com/mcrider/ojs/commit/909e6aa6b49247c31153eb96b7a57bdf0ebfe22d https://github.com/mcrider/ocs/commit/1a738c7461c1bf36918bcc2899eeb905d7e9f17c
Comment 14 Matthew Crider 2011-06-03 16:15:05 PDT
Committed to official. Closing. https://github.com/pkp/ocs/commit/9facaaf81c911231dff53f2d54223a25a64102ab https://github.com/pkp/ojs/commit/70a6e682d7d471162101e91b2bab47995f32148a https://github.com/pkp/omp/commit/d08b3386dc4411ae18d69b76b398eb04611ad581 https://github.com/pkp/pkp-lib/commit/9d72544cc5d89c592f6707101e1403defa4c69aa
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>.