PKP Bugzilla – Bug 4205
Overhaul of Handler Validation and Announcement Abstraction
Last modified: 2011-09-12 14:25:02 PDT
In the process of abstracting Announcements, a complete overhaul was needed. Handler classes are now instantiated and validation checks can be added, much like form checks. The return of the validation functions was standardized to just return true or redirect away from page. Note that Handlers that are moved to the core must extend Handler and not PKPHandler. A typically abstracted Handler should now have a class hieararchy of PKPHandler :: Handler :: PKPSomeHandler :: SomeHandler. To facilitate the instantiation of the appropriate handler class, the index.php for each Handler now acts as a wrapper. It is no longer necessary to replicate the function names in the main handler and the subhandlers. An entry can simply be added to the switch on index.php (see pages/manager/index.php).
Created attachment 1686 [details] patch against PKP Library CVS
Created attachment 1687 [details] Patch against OCS-dev CVS (pre 2.3)
*** Bug 3885 has been marked as a duplicate of this bug. ***
*** Bug 3901 has been marked as a duplicate of this bug. ***
Juan, some comments on the PKP patch: - classes/security/Validation.inc.php includes app-specific funcs (conferenceId & schedConfId variables, isConferenceManager func, etc.) - classes/security/PKPRoleDAO.inc.php contains OCS roles - The locale keys used in PKP code should be included in PKP locale files
Created attachment 1692 [details] Patch against OJS-dev CVS (pre 2.3)
(In reply to comment #5) > Juan, some comments on the PKP patch: > > - classes/security/Validation.inc.php includes app-specific funcs (conferenceId > & schedConfId variables, isConferenceManager func, etc.) > - classes/security/PKPRoleDAO.inc.php contains OCS roles > - The locale keys used in PKP code should be included in PKP locale files I think I left those files in there by accident. I was not done abstracting Roles/Security stuff. So what might have been there was incomplete stuff I was workign on. I'll see and remove.
(In reply to comment #7) Yes. Those files should definitely not have been there. I removed them from CVS.
Thanks, Juan. I suspect this is something you're currently working on, but the Harvester isn't running (I get a missing handler/Handler.inc.php when I request the installer).
above patches committed.
Alec. Yes. You will also run into a few problems on some pages of OJS/OCS that I just discovered (some missing imports). I haven't made the changes to the Harvester yet. All should be done in the next few hours.
if we want to putt the addChecks in the constructors, we have to make sure that every handler class explicitly calls its parent constructor. This could create some security holes, for example, if community plugins are not careful to follow this new protocol. Need to think about this a little and choose an approach.
Juan, I'm OK with requiring subclasses to call the parent constructor (generally good practice anyway). We can check over our own plugins, but I don't think we have any cases of plugins extending handler classes (it's probably not the best practice anyway, because no two plugins can do this to the same handler, meaning plugins will be incompatible in some combinations).
Right. But all will have to extend the Handler class and make sure they call its constructor. I think I put some of the first validation checks in there. I'll go through and add constructor calls to the rest of the classes, after I get the Harvester working again.
Created attachment 1693 [details] Patch against Harvester-dev CVS (pre 2.3) patch committed.
Juan, small tune-up needed to address the following: Warning: Call-time pass-by-reference has been deprecated - argument passed by value; If you would like to pass it by reference, modify the declaration of [runtime function name](). If you would like to enable call-time pass-by-reference, you can set allow_call_time_pass_reference to true in your INI file. However, future versions may not support this any longer. in (...)/classes/handler/PKPHandler.inc.php on line 27
(In reply to comment #16) fixed.
Hey Juan, I'm having trouble trying to submit a paper in OCS--I've fixed a few bugs, but they're getting a bit more complicated and in the domain of what you've been doing--Would you be able to take a look when you get a minute?
(In reply to comment #18) Could be related to one I found in OJS recently. I'll have a look.
try now.
Works like a charm, after a quick fix I posted in bug #4098.
Created attachment 1716 [details] Additional patch against OCS pre-2.3 CVS Cleans up some bugs here and there, some of which are related to the validation overhaul. This has been committed.
*** Bug 4232 has been marked as a duplicate of this bug. ***
Created attachment 1737 [details] patch against OMP pre 1.0 CVS
Created attachment 1738 [details] additional patch against OMP pre 1.0 CVS
Created attachment 1786 [details] additional patch against PKP-lib CVS
Juan, is this entry complete?
closing
Request to non-static calls https://github.com/pkp/pkp-lib/commit/a517bb06be2eddbe7240c97f3fb3ac3ab164786c
Request to non-static calls https://github.com/pkp/ojs/commit/0d37bb66f40e825a900382083cbf8a7ea9222555
Request to non-static calls https://github.com/pkp/omp/commit/eb70e8b8915302897e4814cc67b53a4059215ba8
Request to non-static calls https://github.com/pkp/ocs/commit/64da1e75ede379e7740c51733498c6c92c46f820