Correction comments and fix same bugs #106
Conversation
| if($criteria->limit!=0) | ||
| $this->cursor->limit($criteria->limit); | ||
| }else{ | ||
| $this->cursor = $this->model->getCollection()->find($criteria->getCondition(), $criteria->getProject())->sort($criteria->getSort()); |
There was a problem hiding this comment.
Even though this line uses the explicit functions ->project does actually route to getProject()
There was a problem hiding this comment.
Yes, I know, but if you have given getter, a good programming practice will cause exactly that!
- If you are using PhpStorm, it is very well send you what method return to you.
There was a problem hiding this comment.
However, this code was coded by others than me and the consensus is that the preferred method is to use magics to ascertain properties in Yii.
I personally would prefer the use of magics here currently, it is the preferred Yii way and this is a Yii extension.
There was a problem hiding this comment.
Well, then why are described getters when using magic methods?
There was a problem hiding this comment.
If you use magic methods, why should define get/set methods for property?
There was a problem hiding this comment.
Because those magics route to those getters and setters, if you check the __get/__set of CComponent you will see they do this. Here is a good example of __get:
public function __get($name)
{
$getter='get'.$name;
if(method_exists($this,$getter))
return $this->$getter();
elseif(strncasecmp($name,'on',2)===0 && method_exists($this,$name))
{
// duplicating getEventHandlers() here for performance
$name=strtolower($name);
if(!isset($this->_e[$name]))
$this->_e[$name]=new CList;
return $this->_e[$name];
}
elseif(isset($this->_m[$name]))
return $this->_m[$name];
elseif(is_array($this->_m))
{
foreach($this->_m as $object)
{
if($object->getEnabled() && (property_exists($object,$name) || $object->canGetProperty($name)))
return $object->$name;
}
}
throw new CException(Yii::t('yii','Property "{class}.{property}" is not defined.',
array('{class}'=>get_class($this), '{property}'=>$name)));
}
You can see in the first 3 lines where it routes to the getter
|
Added some audit notes to the code |
|
Hows this coming along? |
|
Tonight fix and push, yesterday celebrated the anniversary with my wife. |
|
Hope you had a good one :) I'll wait for it |
|
Yes, it was not bad. Do you have Skype? |
|
Nah, never needed it, I am a very textual kinda communicator :) |
|
Ok, I have a friend just claimed that you know Russian language, if so - chat on Skype would be faster. |
|
Ah hell no, any suggestion I have been able to make of it is pure luck; hell I know more Arabic than Russian (I have actually been learning some Arabic). I play eve with quite a few Russians and was even in a Russian alliance on it but my understanding of it is just to understand the sort of notion of what's going on, at the end of the day they all spoke okish English when needed so we were able to play together. |
|
I understand :) |
|
Back |
| } | ||
|
|
||
| return $this; // Maintain chainability | ||
| // TODO I think this is should to investigate and remove |
There was a problem hiding this comment.
I will run the unit tests with this removed when I merge and see if it can be removed
There was a problem hiding this comment.
It would be good, since this line is really misleading
Correction comments and fix same bugs
|
Ok lets take a look at this |
|
All unit tests passing after merge, continuing with changes |
Correction comments
Fix bug:
EMongoCriteria.php:231 -> EMongoCriteria.php:245 - is_numeric -> is_array