Bug 4934

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