MvcTools/review-1.0.txt

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 :)