From 03ea25b1c8615d4d09f2619da8ebb4c366ce3c60 Mon Sep 17 00:00:00 2001 From: Alexander Makarov Date: Sun, 13 Nov 2011 02:14:59 +0400 Subject: [PATCH] code review --- docs/code_style.md | 11 +- docs/internals/base.md | 44 ++++++++ docs/internals/git.md | 13 +++ docs/review_2011_11_12_alex.md | 171 +++++++++++++++++++++++++++++ tests/unit/framework/base/BehaviorTest.php | 32 ++++++ tests/unit/framework/base/ObjectTest.php | 22 ++++ 6 files changed, 290 insertions(+), 3 deletions(-) create mode 100644 docs/internals/base.md create mode 100644 docs/internals/git.md create mode 100644 docs/review_2011_11_12_alex.md create mode 100644 tests/unit/framework/base/BehaviorTest.php create mode 100644 tests/unit/framework/base/ObjectTest.php diff --git a/docs/code_style.md b/docs/code_style.md index 01ab79e..414f866 100644 --- a/docs/code_style.md +++ b/docs/code_style.md @@ -1,8 +1,15 @@ Yii2 core code style ==================== + +Proposals +--------- + ### Brackets +It's better to be consistent with brackets not to remember where should we use +newline and where not: + ~~~ class MyClass { @@ -20,9 +27,7 @@ class MyClass } ~~~ - -Proposals ---------- +Use brackets even for one line `if`s. ### Use type hinting like diff --git a/docs/internals/base.md b/docs/internals/base.md new file mode 100644 index 0000000..ede6ab2 --- /dev/null +++ b/docs/internals/base.md @@ -0,0 +1,44 @@ +Base classes and interfaces +=========================== + +Object +------ + +Object is the base class for many other Yii2 classes. + +### property feature + +#### Why + +To be able to make property `public` initially and then seamlessly make it +`private` or `protected` by adding getter and setter method. That will *not* +change API. Results in less repetitive code. Performance drop isn't significant. + +### callbacks and expressions + +### [[Object::create()]|create] method + +This method is a powerful way to instantiate a class. Differences from `new`: + +- Calls class constructor (same the `new` operator); +- Initializes the object properties using the name-value pairs given as the + last parameter to this method; +- Calls [[Initable::init|init]] if the class implements [[Initable]]. + +#### Why + +To support class dependencies and their lazy loading. + +### [[Initable]] interface + +Developer will implement initable interface if running `init()` needed and will +skip it if not. + +#### Why + +Indicates where `init()` will be called and where not. More explicit than it was +in Yii 1. + +Component +--------- + diff --git a/docs/internals/git.md b/docs/internals/git.md new file mode 100644 index 0000000..0d660aa --- /dev/null +++ b/docs/internals/git.md @@ -0,0 +1,13 @@ +Git branches and tags +===================== + +Tags +---- + +Each release should be tagged with v2.X.X and message "Yii 2.X.X release". + +Branches +-------- + +What should be in master branch? +Do we need another branches? \ No newline at end of file diff --git a/docs/review_2011_11_12_alex.md b/docs/review_2011_11_12_alex.md new file mode 100644 index 0000000..dd612d2 --- /dev/null +++ b/docs/review_2011_11_12_alex.md @@ -0,0 +1,171 @@ +Alex's Code Review, 2011.11.12 +============================== + +Overall hierarchy +------------------ + +Generally is OK. Like that `Object` and `Component` are now separated. +I've generated 2 diagrams under `docs/` to see it better as a whole. + +Object +------ + +### property feature + +Why returning anything when setting a value? + +~~~ +if (method_exists($this, $setter)) { + // ??? + return $this->$setter($value); +} +~~~ + +Is it OK that `canGetProperty` and `canSetProperty` will return `false` for real +class members? + +### callbacks and expressions + +We're using 5.3. What's the reason to support `eval()` in `evaluateExpression` if +we have anonymous functions? Is that for storing code as string inside of DB (RBAC)? + +If we're going to get rid of `eval()`, cosider remaning method to something about callback. +If not then we definitely need to use anonymous functions in API docs and the guide +where possible. + +### Object::create() + +#### `__construct` issue + +Often a class doesn't have `__construct` implementation and `stdClass` doesn't have +default one either but Object::create() always expects constructor to be +defined. See `ObjectTest`. Either `method_exists` call or `Object::__construct` needed. + +#### How to support object factory like we do with CWidgetFactory? + +~~~ +class ObjectConfig +{ + public function configure($class) + { + $config = $this->load($class); + // apply config to $class + } + + private function load($class) + { + // get class properties from a config file + // in this method we need to walk all the + // inheritance hierarchy down to Object itself + return array( + 'property' => 'value', + // … + ); + } +} +~~~ + +Then we need to add `__construct` to `Object` (or implement `Initalbe`): + +~~~ +class Object +{ + public function __construct() + { + $conf = new ObjectConfig(); + $conf->configure($this); + } +} +~~~ + +This way we'll be able to set defaults for any object. + +#### Do we need to support lazy class injection? + +Currently there's no way to lazy-inject class into another class property via +config. Do we need it? If yes then we can probably extend component config to support +the following: + +~~~ +class Foo extends Object +{ + public $prop; +} + +class Bar extends Object +{ + public $prop; +} + +$config = array( + 'prop' => array( + 'class' => 'Bar', + 'prop' => 'Hello!', + ), +); + +$foo = Foo::create($config); +echo $foo->bar->prop; +// will output Hello! +~~~ + +Should it support infinite nesting level? + +### Why `Event` is `Object`? + +There's no need to extend from `Object`. Is there a plan to use `Object` features +later? + +Initable +-------- + +Interface itself looks OK. Its usage is OK too. + +`Initable::preinit` mentioned in `Yii::create()` docs but neither defined in +the interface nor called in the code. + +Behaviors +--------- + +Overall I wasn't able to use behaviors. See `BehaviorTest`. + +### Wrong API docs at Behavior + +Docs mention properties and events but not methods. + +### Should behaviors be able to define events for owner components? + +Why not? Should be a very good feature in order to make behaviors customizable. + +### Multiple behaviors can be attached to the same component + +What if we'll have multiple methods / properties / events with the same name? + +### How to use Behavior::attach? + +Looks like it is used by `Component::attachBehavior` but can't be used without it. +Why it's public then? Can we move it to `Component?` + +Events +------ + +Class itself looks OK. Component part is OK as well but I've not tested +it carefully. Overall it seems concept is the same as in Yii1. + +### Event declaration: the on-method is mostly repetitive for every event. Should we choose a different way of declaring events? + +Maybe. People complained previously about too many code for event declaration. + +### Should we implement some additional event mechanism, such as global events? + +Why use two different implementations in a single application? + +Exceptions +---------- + +- Should we convert all errors, warnings and notices to exceptions? + +Coding style +------------ + +See `docs/code_style.md`. \ No newline at end of file diff --git a/tests/unit/framework/base/BehaviorTest.php b/tests/unit/framework/base/BehaviorTest.php new file mode 100644 index 0000000..519653e --- /dev/null +++ b/tests/unit/framework/base/BehaviorTest.php @@ -0,0 +1,32 @@ +attachBehavior('bar', $bar); + $this->assertEquals('behavior property', $bar->behaviorProperty); + $this->assertEquals('behavior method', $bar->behaviorMethod); + $this->assertEquals('behavior property', $bar->bar->behaviorProperty); + $this->assertEquals('behavior method', $bar->bar->behaviorMethod); + } +} diff --git a/tests/unit/framework/base/ObjectTest.php b/tests/unit/framework/base/ObjectTest.php new file mode 100644 index 0000000..d1b71af --- /dev/null +++ b/tests/unit/framework/base/ObjectTest.php @@ -0,0 +1,22 @@ + array( + 'test' => 'test', + ), + )); + + $this->assertEquals('test', $foo->prop['test']); + } +}