Bug 4205 - Overhaul of Handler Validation and Announcement Abstraction
Overhaul of Handler Validation and Announcement Abstraction
Status: RESOLVED FIXED
Product: PKP-LIB
Classification: Unclassified
Component: General
2.0
PC Mac OS X 10.3
: P5 enhancement
Assigned To: Juan Pablo Alperin
: 3885 3901 4232 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-17 08:37 PDT by Juan Pablo Alperin
Modified: 2011-09-12 14:25 PDT (History)
5 users (show)

See Also:
Version Reported In:
Also Affects:


Attachments
patch against PKP Library CVS (109.68 KB, patch)
2009-04-17 09:26 PDT, Juan Pablo Alperin
Details | Diff
Patch against OCS-dev CVS (pre 2.3) (316.94 KB, patch)
2009-04-17 09:26 PDT, Juan Pablo Alperin
Details | Diff
Patch against OJS-dev CVS (pre 2.3) (447.86 KB, patch)
2009-04-20 08:46 PDT, Juan Pablo Alperin
Details | Diff
Patch against Harvester-dev CVS (pre 2.3) (64.75 KB, patch)
2009-04-20 11:25 PDT, Juan Pablo Alperin
Details | Diff
Additional patch against OCS pre-2.3 CVS (21.21 KB, patch)
2009-04-30 11:39 PDT, Matthew Crider
Details | Diff
patch against OMP pre 1.0 CVS (28.59 KB, patch)
2009-05-06 15:29 PDT, Juan Pablo Alperin
Details | Diff
additional patch against OMP pre 1.0 CVS (258.18 KB, patch)
2009-05-06 15:30 PDT, Juan Pablo Alperin
Details | Diff
additional patch against PKP-lib CVS (8.63 KB, patch)
2009-05-16 18:08 PDT, Juan Pablo Alperin
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Juan Pablo Alperin 2009-04-17 08:37:11 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).
Comment 1 Juan Pablo Alperin 2009-04-17 09:26:06 PDT
Created attachment 1686 [details]
patch against PKP Library CVS
Comment 2 Juan Pablo Alperin 2009-04-17 09:26:43 PDT
Created attachment 1687 [details]
Patch against OCS-dev CVS (pre 2.3)
Comment 3 Juan Pablo Alperin 2009-04-20 08:28:21 PDT
*** Bug 3885 has been marked as a duplicate of this bug. ***
Comment 4 Juan Pablo Alperin 2009-04-20 08:29:03 PDT
*** Bug 3901 has been marked as a duplicate of this bug. ***
Comment 5 Alec Smecher 2009-04-20 08:38:56 PDT
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
Comment 6 Juan Pablo Alperin 2009-04-20 08:46:55 PDT
Created attachment 1692 [details]
Patch against OJS-dev CVS (pre 2.3)
Comment 7 Juan Pablo Alperin 2009-04-20 08:49:28 PDT
(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.
Comment 8 Juan Pablo Alperin 2009-04-20 08:55:33 PDT
(In reply to comment #7)

Yes.  Those files should definitely not have been there.  I removed them from CVS.
Comment 9 Alec Smecher 2009-04-20 08:56:27 PDT
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).
Comment 10 Juan Pablo Alperin 2009-04-20 08:56:52 PDT
above patches committed.
Comment 11 Juan Pablo Alperin 2009-04-20 08:57:47 PDT
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.
Comment 12 Juan Pablo Alperin 2009-04-20 09:44:29 PDT
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.
Comment 13 Alec Smecher 2009-04-20 09:47:11 PDT
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).
Comment 14 Juan Pablo Alperin 2009-04-20 09:54:07 PDT
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.
Comment 15 Juan Pablo Alperin 2009-04-20 11:25:27 PDT
Created attachment 1693 [details]
Patch against Harvester-dev CVS (pre 2.3)

patch committed.
Comment 16 Alec Smecher 2009-04-20 11:28:53 PDT
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
Comment 17 Juan Pablo Alperin 2009-04-20 11:48:59 PDT
(In reply to comment #16)
fixed.
Comment 18 Matthew Crider 2009-04-28 17:17:03 PDT
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?
Comment 19 Juan Pablo Alperin 2009-04-28 17:18:35 PDT
(In reply to comment #18)

Could be related to one I found in OJS recently.  I'll have a look.
Comment 20 Juan Pablo Alperin 2009-04-28 18:02:41 PDT
try now.
Comment 21 Matthew Crider 2009-04-29 11:37:17 PDT
Works like a charm, after a quick fix I posted in bug #4098.
Comment 22 Matthew Crider 2009-04-30 11:39:04 PDT
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.
Comment 23 Juan Pablo Alperin 2009-05-06 09:13:01 PDT
*** Bug 4232 has been marked as a duplicate of this bug. ***
Comment 24 Juan Pablo Alperin 2009-05-06 15:29:46 PDT
Created attachment 1737 [details]
patch against OMP pre 1.0 CVS
Comment 25 Juan Pablo Alperin 2009-05-06 15:30:13 PDT
Created attachment 1738 [details]
additional patch against OMP pre 1.0 CVS
Comment 26 Juan Pablo Alperin 2009-05-16 18:08:23 PDT
Created attachment 1786 [details]
additional patch against PKP-lib CVS
Comment 27 Alec Smecher 2009-06-09 07:46:54 PDT
Juan, is this entry complete?
Comment 28 Juan Pablo Alperin 2009-06-26 09:24:37 PDT
closing
Comment 29 Alec Smecher 2011-09-12 14:15:02 PDT
Request to non-static calls
https://github.com/pkp/pkp-lib/commit/a517bb06be2eddbe7240c97f3fb3ac3ab164786c
Comment 30 Alec Smecher 2011-09-12 14:20:02 PDT
Request to non-static calls
https://github.com/pkp/ojs/commit/0d37bb66f40e825a900382083cbf8a7ea9222555
Comment 31 Alec Smecher 2011-09-12 14:20:02 PDT
Request to non-static calls
https://github.com/pkp/omp/commit/eb70e8b8915302897e4814cc67b53a4059215ba8
Comment 32 Alec Smecher 2011-09-12 14:25:02 PDT
Request to non-static calls
https://github.com/pkp/ocs/commit/64da1e75ede379e7740c51733498c6c92c46f820