We are moving to Git Issues for bug tracking in future releases. During transition, content will be in both tools. If you'd like to file a new bug, please create an issue.

Bug 4934 - Convert legacy Request calls to new router architecture
Convert legacy Request calls to new router architecture
Status: ASSIGNED
Product: PKP-LIB
Classification: Unclassified
Component: Application Refactoring
Undetermined
All All
: P4 minor
Assigned To: PKP Support
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-08 21:10 PST by jerico
Modified: 2010-01-04 13:16 PST (History)
3 users (show)

See Also:
Version Reported In:
Also Affects:


Attachments
sample refactoring for a page (2.37 KB, patch)
2009-12-10 14:24 PST, jerico
Details | Diff
simplify request path access: patch against omp pre-release cvs (1.23 KB, patch)
2009-12-11 14:20 PST, jerico
Details | Diff
simplify request path access: patch against ojs pre-2.3.1 cvs (1.24 KB, patch)
2009-12-11 14:22 PST, jerico
Details | Diff
simplify request path access: patch against pkp-lib pre-release cvs (pre OJS 2.3.1) (9.21 KB, patch)
2009-12-11 14:28 PST, jerico
Details | Diff
sample refactoring for a page class (against OJS pre 2.3.1 cvs) (1.08 KB, patch)
2009-12-11 14:31 PST, jerico
Details | Diff
sample refactoring for an infrastructure class in pkp lib (some callers are not yet refactored) (1.31 KB, patch)
2010-01-04 11:13 PST, jerico
Details | Diff
a more complex refactoring example (12.14 KB, patch)
2010-01-04 11:14 PST, jerico
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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