Bug 4934

Summary: Convert legacy Request calls to new router architecture
Product: PKP-LIB Reporter: jerico <jerico.dev>
Component: Application RefactoringAssignee: PKP Support <pkp-support>
Status: ASSIGNED ---    
Severity: minor CC: alec, jerico.dev, juan
Priority: P4    
Version: Undetermined   
Hardware: All   
OS: All   
Version Reported In: Also Affects:
Attachments: sample refactoring for a page
simplify request path access: patch against omp pre-release cvs
simplify request path access: patch against ojs pre-2.3.1 cvs
simplify request path access: patch against pkp-lib pre-release cvs (pre OJS 2.3.1)
sample refactoring for a page class (against OJS pre 2.3.1 cvs)
sample refactoring for an infrastructure class in pkp lib (some callers are not yet refactored)
a more complex refactoring example

Description jerico 2009-12-08 21:10:30 PST
Todo: add an example patch for a typical handler refactoring.
Comment 1 jerico 2009-12-10 14:24:20 PST
Created attachment 2726 [details]
sample refactoring for a page
Comment 2 jerico 2009-12-10 14:26:24 PST
Please have a look at the attached patch for an example of how the new router/request objects are passed along to pages and how they are used. If you have a more complex case or need to pass the request along to another class then you can always ask me of course.
Comment 3 Alec Smecher 2009-12-11 10:42:43 PST
Florian, looks good -- but it would be nice to have a couple of convenience methods, i.e. Router::getRequestedJournalPath, which would invoke Router::getRequestedContextPath($request, CONTEXT_JOURNAL, false);
Comment 4 jerico 2009-12-11 13:09:22 PST
Hi Alec,

you are right. It makes sense to simplify the code here for convenience.

I propose something slightly different, though. I've seen that much application specific code in the classes folder is just due to the old app-specific request convenience methods and can be re-factored to the library if we avoid putting in new app-specific methods. Even in app-specific pages/classes any kind of future re-factoring/modularization/clustering, etc. will be easier if we avoid app-specific method calls.

As we don't need to be backwards compatible in the new router I'd change the semantics and defaults of the method to something more useful, i.e. getRequestedContextPath(&$request, $requestedContextLevel = 1) {...} which always returns a scalar and getRequestedContextPaths(&$request) {...} which always returns an array. (And similarly for other context-methods.)

Backwards compatibility is not a problem as we can maintain the old method behavior in the Request's compatibility method.

For applications with context depth 0 or 1 (all except OCS) this allows us to simply use $router->getRequestedContextPath($request) in all cases which I believe should be equally convenient. Only OCS would require to set the second argument and only if a scheduled conference is called.

It's true that your proposal has better readability but I think that discouraging application specific code weighs more as it considerably reduces maintenance cost.

Kind regards,
Florian
Comment 5 Alec Smecher 2009-12-11 13:17:13 PST
Florian, sounds good to me.
Comment 6 jerico 2009-12-11 14:20:11 PST
Created attachment 2732 [details]
simplify request path access: patch against omp pre-release cvs
Comment 7 jerico 2009-12-11 14:22:11 PST
Created attachment 2733 [details]
simplify request path access: patch against ojs pre-2.3.1 cvs
Comment 8 jerico 2009-12-11 14:28:59 PST
Created attachment 2734 [details]
simplify request path access: patch against pkp-lib pre-release cvs (pre OJS 2.3.1)
Comment 9 jerico 2009-12-11 14:31:12 PST
Created attachment 2735 [details]
sample refactoring for a page class (against OJS pre 2.3.1 cvs)

@Alec: That's now the simplified code. Is that ok?
Comment 10 Alec Smecher 2009-12-12 21:09:02 PST
Looks good!
Comment 11 Juan Pablo Alperin 2009-12-13 18:39:46 PST
worth noting that the PKPHandler class should be given special attention.  There is a call there to Request::getContext() that may need to be fixed.
Comment 12 jerico 2010-01-04 11:13:43 PST
Created attachment 2787 [details]
sample refactoring for an infrastructure class in pkp lib (some callers are not yet refactored)
Comment 13 jerico 2010-01-04 11:14:54 PST
Created attachment 2788 [details]
a more complex refactoring example