| Review of MvcTools-1.0-alpha at 26.11.08 |
| ======================================== |
| |
| :Author: kn |
| |
| API |
| --- |
| |
| [-] The abstract class ezcMvcRouter contains a methods, which I do not really |
| understand: |
| |
| - The static public method prefix() is not used or referenced anywhere |
| else in that class |
| |
| dr: It is supposed to be used in the user inherited router, so that you |
| can include the result of other routers immediately by applying a prefix |
| to the returned routes. This means you can reuse for example a "blog" |
| router class in your application, by prefixing all routes with "/blog". |
| |
| [-] Make ezcMvcResult an interface or do not already implement any final |
| methods. If some properties are required, there should be an extensible |
| method for it. Otherwise this strongly limits the extensibility. |
| |
| dr: Can you explain to me how and why you would like to do this? |
| |
| kn: I sometimes have validators (__set) in my result structs, which may |
| not be the ezc-way, but works fine for me. Additionally I sometimes |
| convert business models to their view model equivalents on __set. With a |
| final __set method I would be required to move this logic into my |
| controller, where it imho does not belong. |
| |
| The $status property still seems not really important / used to me. |
| |
| [X] The DispatcherConfiguration is documented to return a view handler. The |
| configurableDispatcher calls the method createResponse() on the returned |
| view handler. But this method is not defined in the interface. Instead |
| there are other methods defined, I did not see called anywhere. |
| |
| dr: The documentation was wrong, it should return an ezcMvcView object. |
| |
| [X] The ezcMvcResponse::$status variable needs better documentation, its purpose |
| is at least unclear to me. The check in |
| MvcTools/src/response_writers/http.php +61 probably should be changed to |
| is_object(), otherwise values like "null" cause PHP errors there. |
| |
| [X] Initialize status with 0, even when the constructor is not called - if this |
| property really is required. |
| |
| ts: Properties which may contain objects should have null as the default / |
| correct value, not an integer. |
| |
| Documentation |
| ------------- |
| |
| [X] Didn't we agree to document parameters in teh method description and not |
| behind the parameter, like in MvcTools/src/interfaces/route.php +25? |
| |
| [X] The purpose behind ezcMvcRoute::prefix() is not obvious to me from the |
| documentation. |
| |
| dr: it depends on the implementation what it does, in the implementations |
| documentation should be extensive. |
| |
| And I don't think the prefixing could fix all the issues, which occur |
| regarding prefixes of controller paths (application in subdirectory, running |
| inside a PHAR, using mod_rewrite, ..) |
| |
| I actually remove the prefix in the HttpParser (script name, subdir, ..) and |
| pass the basePath and the sanitized URI around. Using this inside the |
| templates should solve most issues. |
| |
| [X] The documentation in ezcMvcHttpRequestParser is incomplete. |
| |
| [X] The return value documentation of |
| arbitDispatcherConfiguration::createFatalRedirectRequest() is wrong and |
| should be: @return ezcMvcRequest. Using the currently documented return |
| value breaks the main loop in ezcMvcConfigurableDispatcher. |
| |
| dr: There is no return value for |
| ezcMvcDispatcherConfiguration::runPreRoutingFilters documented -- nor |
| is it necessary. |
| |
| kn: Fixed method name, it should have said: |
| arbitDispatcherConfiguration::createFatalRedirectRequest |
| |
| Trivial, maybe irrelevant |
| ------------------------- |
| |
| ezcMvcConfigurableDispatcher |
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
| |
| [X] Increase $redirects at begin of loop, reduces redundancy and makes it less |
| error prone if somebody extends / changes the code. |
| |
| dr: already done |
| |
| [X] Even 25 should be save, make the maximum internal redirects configurable? |
| |
| dr: I don't think this is important enough to warrant a whole option |
| class. We can do this when we get a feature request :) |