PKP Bugzilla – Bug 4934
Convert legacy Request calls to new router architecture
Last modified: 2010-01-04 13:16:16 PST
Todo: add an example patch for a typical handler refactoring.
Created attachment 2726 [details] sample refactoring for a page
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.
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);
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
Florian, sounds good to me.
Created attachment 2732 [details] simplify request path access: patch against omp pre-release cvs
Created attachment 2733 [details] simplify request path access: patch against ojs pre-2.3.1 cvs
Created attachment 2734 [details] simplify request path access: patch against pkp-lib pre-release cvs (pre OJS 2.3.1)
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?
Looks good!
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.
Created attachment 2787 [details] sample refactoring for an infrastructure class in pkp lib (some callers are not yet refactored)
Created attachment 2788 [details] a more complex refactoring example