-
Notifications
You must be signed in to change notification settings - Fork 9
/
review-1.0.txt
100 lines (70 loc) · 3.92 KB
/
review-1.0.txt
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
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 :)