-
Notifications
You must be signed in to change notification settings - Fork 8
/
review.txt
128 lines (94 loc) · 3.87 KB
/
review.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
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
> Comments by: Jan Borsodi (11-16-2005)
> -------------------------------------
> ezcCacheManager::createCache()
> Documents that it throws an exception but there is no code for it.
> Also it does not really explain why an exception would be thrown.
> The test code lacks a check for a thrown exception.
Thanks for information, there were some sanity checks missing. I fixed
that part. Regarding the documentation I consider it sufficient
to set a link to the specific error constant (as I did in all of my
packages), which then explais what a specific error code should mean.
If this solution is insufficient for you, please inform me!
> ezcCacheManager::getCache()
> Does not check if the requested $id exists and no exception is thrown.
> Also it does not really explain why an exception would be thrown.
> The test code lacks a check for a thrown exception.
Fixed.
> ezcCacheStorage::store()
> No explanation of when exceptions are thrown.
Fixed.
> ezcCacheManagerException
> ezcCacheStorageException
> This exception does not look complete, only contains some constants.
Which is pretty much correct, since it is only invented to devide management
exceptions from storage exceptions. Both classes are only containers for
their error codes.
> ezcCacheStorageFile
> ezcCacheStorageFileArray
> ezcCacheStorageFileEvalArray
> ezcCacheStorageFilePlain
> The naming is from the old standard, shouldn't this be named
> ezcCacheFileStorage etc.?
I don't know of such a change in the naming scheme. By now, the
docs/guidelines/class_filenames.txt document states:
Good example: ::
ezcDbHandlerMysql
ezcDbHandlerPostgresql
Bad example: ::
ezcDbPostgresqlHandler
ezcDbMysqlHandler
This means, if I change to your suggested names and someone create a storage
backend later, to store arrays into a database, I'd have
ezcCacheDatabaseStorageArray
which sounds a bit rediculous for me. Specifying the file names after their
inheritance from unspecific to specific sounds most logical to me. Therefore I
chose
ezcCacheStorageFileArray extends ezcCacheStorageFile extends ezcCacheStorage.
Did I miss something in this direction?
> ezcCacheStorageFile::hasData()
> Doc for @return is not correct.
Fixed.
> ezcCacheStorageFileEvalArray
> Doc for class does not really explain what it is meant for. Even the code is
> pretty similar to the ezcCacheStorageFileArray class. I'm not sure I
> understand why this class is present.
Updated docs and explained difference between *Array and *Evalarray.
> ezcCacheStorageFileArray::prepareData
> ezcCacheStorageFileEvalArray::prepareData
> ezcCacheStorageFilePlain::prepareData
> No direct explanation of why the exception is thrown.
Fixed.
> Cache/trunk/src/
> The directory structure does not follow our new standard. The abstract
> classes should be placed in an 'interfaces' directory and the storage
> handlers directly under 'storage' dir.
I'm not sure I really understand. I did not see any "new dir strcture"
document in SVN and the example listed in class_filenaming.txt looks quite
similar to mine... I either do not consider this "new structure" a good thing,
since it does not reflect the class structure correctly anymore. An abstract
class is definitly not an interface (although it declares partly an interface,
through it's abstract methods). It therefore should not be located under the
interfaces/ directory. Another point is, that my handler structure is getting
much more unclear, when moving those classes to a different directory. It
would result in:
src/
interfaces/
storage.php
storage_file.php
storage_db.php
storage_db_sql.php
storage_db_xml.php
...
storage/
file_array.php
file_evalarray.php
file_plain.php
db_sql_mysql.php
db_sql_sqlite.php
db_xml_tamino.php
...
Please give me a hint how we will handle this and update the docs regarding
the new standard. Thanks!
Bugs:
http://ez.no/bugs/view/7469
> Fixed.