On Sun, 29 Jun 2008, Tobias Schlitt wrote: > 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.
That's the name we had previously, but as some people pointed out: This class does not *parse* a request - it builds a request object from the raw request data. RequestParser would sortof indicate it accepted a request object already, instead of creating one. This is why we changed it to ezcMvcRequestBuilder. > 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. The reason for not calling the interface methods just run() is so that you can implement all the three filter interfaces in one class: ezcMvcAuthenticationFilter implements ezcRequestFilter, ezcResultFilter, ezcResponseFilter. The filters should indeed just be run by the controller from the run() method in some way. I think you're right that we can just remove it. (your point a). As for b, that would be a bad idea, as the request builder doesn't actually run the controller, but just returns the request object. The dispatcher calls the controller with this request object. How the controller runs the filter, should not really matter - however, I think it would be good to standardize it, and thus leave it in the interface like we've currently designed. > 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. It's a struct, so I don't see what you're trying to say here. > 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. That's why this is just part of the stuct as a property of ezcMvcRequest. It can be absent of course. > ezcMvcRequestFile > ----------------- > > This class defines an attribute $status. What is this meant for? Upload status field, see: http://php.net/manual/en/features.file-upload.errors.php > 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? It's good to normalize this values sometimes - feature enhancements could extend this to extract browsecap information into this struct. > 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... > } I agree. The request hierachy should be: ezcMvcRequest's properties: $variables $content $files (array(ezcMvcRequestFile)) $userAgent (ezcMvcRequestUserAgent) $raw (ezcMvcRawRequest) ezcMvcRawHttpRequest extends ezcMvcRawRequest ezcMvcRawSoapRequest extends ezcMvcRawRequest > 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? It isn't - I fixed that in the document just now. > 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? Yes, easier testability. > 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 > } > } The preparring is done by the controller, I don't see why you should prepare the result from the controller in the view again... > ezcMvcHtmlViewSomething extends ezcMvcViewSomething > { > public function handleResponse( ezcMvcHtmlResponse $response ) > { > // Process a template and print the result... > } > } The handleResponse method should just send the output and set the correct HTTP headers. > 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. Yeah, I agree. > 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. It was a joke ;-) > The implementation also makes use of a $routerManager, which is not > defined anyhwhere else. You have to implement it, just like the viewManager. Both are at the top of the example implementation of the dispatcher. regards, Derick -- Components mailing list Components@lists.ez.no http://lists.ez.no/mailman/listinfo/components
