Hi!
I just reviewed the design document. Thanks for adding the class /
method stubs to it. The design looks great, overall. Here are some
comments from my side:
ezcMvcRequestBuilder
--------------------
I think the name of this class is a bit strange. I'd favor to call it
ezcMvcRequestParser. I think this sounds more common.
ezcMvcRequestFilter
-------------------
The filters are described to be used in a method named
ezcMvcController->runRequestFilters(), while this method should be
called by ezcMvcController->run(). It does make little sense to me to
mention that a method ezcMvcController->runRequestFilters() should be
implemented at all, if the controller calls the filter itself.
Instead I would suggest to design it in either of the following ways:
a) Remove the ezcMvcController->runRequestFilters() part and just
mention that filters should be run by the controller.
b) Require the method ezcMvcController->runRequestFilters() in the
interface and make it be run by the request builder.
ezcMvcRequest
-------------
The class defines a $content and a $variables attribute. Where is the
difference here?
In case of GET/POST requests, there are only variables, files, etc. So
not content. In case of PUT requests, there might be content, but no
variables. Shouldn't this be unified? In case of a SOAP request, there
are only variables, too, but no content.
ezcMvcRequestAuthentication
---------------------------
This class defines an $identifier and $password field. These fields only
make sense for authentications as Basic-Auth. However, in case of e.g.
OpenID, they are not available. Such differences should be abstracted in
the class.
ezcMvcRequestFile
-----------------
This class defines an attribute $status. What is this meant for?
ezcMvcRequestUserAgent
----------------------
This struct only defines 1 attribute. Therefore it looks like overhead
to me. We can simply store the user agent as a string in the
ezcMvcRequest object. Or am I wrong?
ezcMvcRawRequest
-------------
I think that this class should not contain anything, but just work as a
base class for all raw request classes, to allows proper instanceof
checks. E.g.:
ezcMvcRawHttpRequest extends ezcMvcRawRequest
{
// Whatever the request contains...
// ... PHPs super global arrays make sense here...
}
ezcMvcRawSoapRequest extends ezcMvcRawRequest
{
// Whatever the request contains...
// ... objects from ext/soap make sense here...
}
ezcMvcRouter
------------
The description says "... select one or more controllers to run and
collect their output...". However, its method createController() only
allows to return 1 controller object. How is it supposed to select
multiple controllers?
ezcMvcViewHandler
-----------------
The design states, that this class should implement createResponse() and
handleResponse(). Both methods are just called in a chain. Is there much
sense in defining both, then? It would only make sense, if
createResponse() is defined in a different class than handleResponse().
Shouldn't this be different classes anyway? I imagine:
ezcMvcProductHtmlViewHandler extends ezcMvcViewHandler
{
public function createResponse( ezcMvcResult $result )
{
// Prepare the product result for HTML output...
// ... return a HTML response object
}
}
ezcMvcHtmlViewSomething extends ezcMvcViewSomething
{
public function handleResponse( ezcMvcHtmlResponse $response )
{
// Process a template and print the result...
}
}
ezcMvcController
----------------
So far the design defines that the action is selected internally by the
controller. While I can live with this solution, I think the decision
belongs more to the dispatcher. This avoids quite some duplication in
controller code, I'd say.
Example Dispatcher
------------------
The example implementation uses goto. While I like that this is added to
PHP in 5.3 (e.g. for parser implementations), I don't see a sense in
using it here. We should better extract the re-run part into a method
and call it recursively, if needed.
The implementation also makes use of a $routerManager, which is not
defined anyhwhere else.
Regards,
Toby
--
Mit freundlichen Grüßen / Med vennlig hilsen / With kind regards
Tobias Schlitt (GPG: 0xC462BC14) eZ Components Developer
[EMAIL PROTECTED] | eZ Systems AS | ez.no
--
Components mailing list
Components@lists.ez.no
http://lists.ez.no/mailman/listinfo/components
