Possible bug in Zend_Db_Select::assemble function BA7-899

[Mathieu Suen-3]

Hi,

I think that the Zend_Db_Select::assemble method is bugy.
I would expected something like:

     /**
      * Converts this object to an SQL SELECT string.
      *
      * @return string This object as a SELECT string.
      */
     public function assemble()
     {
         $sql = self::SQL_SELECT;
         foreach (array_keys($this->_parts) as $part) {
             $method = '_render' . ucfirst($part);
             if (method_exists($this, $method)) {
                 $sql = $this->$method($sql);
             }
         }
         return $sql;
     }


If so I can create un bug report. With the atteched patch

Thanks

--
-- Mathieu Suen
--

Index: global/library/Zend/Db/Select.php
===================================================================
--- global/library/Zend/Db/Select.php (revision 4086)
+++ global/library/Zend/Db/Select.php (working copy)
@@ -682,7 +682,7 @@
     public function assemble()
     {
         $sql = self::SQL_SELECT;
-        foreach (array_keys(self::$_partsInit) as $part) {
+        foreach (array_keys($this->_parts) as $part) {
             $method = '_render' . ucfirst($part);
             if (method_exists($this, $method)) {
                 $sql = $this->$method($sql);

[Ralph Schindler-2]

This not a bug, IMO.  Zend_Db_Select uses $_partsInit as a skeleton for
what _parts will look like AND for which _render{Part} methods exist.

To extend Zend_Db_Select for a new SQL part, you'd have to include its
name (as a new constant) into the _partsInit array as well as the
corresponding _render{Part}() method.  Basically, its a way of keeping
developers honest when it comes to extending Zend_Db_Select for new
functionality.  (It also is required for resetting values.)

Out of curiosity, why do you feel this is a bug?  Is it hindering a user
case?

-ralph

Mathieu Suen wrote:

> Hi,
>
> I think that the Zend_Db_Select::assemble method is bugy.
> I would expected something like:
>
>     /**
>      * Converts this object to an SQL SELECT string.
>      *
>      * @return string This object as a SELECT string.
>      */
>     public function assemble()
>     {
>         $sql = self::SQL_SELECT;
>         foreach (array_keys($this->_parts) as $part) {
>             $method = '_render' . ucfirst($part);
>             if (method_exists($this, $method)) {
>                 $sql = $this->$method($sql);
>             }
>         }
>         return $sql;
>     }
>
>
> If so I can create un bug report. With the atteched patch
>
> Thanks
>

[Mathieu Suen-3]

Ralph Schindler a écrit :

> This not a bug, IMO.  Zend_Db_Select uses $_partsInit as a skeleton for
> what _parts will look like AND for which _render{Part} methods exist.
>
> To extend Zend_Db_Select for a new SQL part, you'd have to include its
> name (as a new constant) into the _partsInit array as well as the
> corresponding _render{Part}() method.  Basically, its a way of keeping
> developers honest when it comes to extending Zend_Db_Select for new
> functionality.  (It also is required for resetting values.)
>
> Out of curiosity, why do you feel this is a bug?  Is it hindering a user
> case?
>

Yeap, In fact you are breaking reusability of the framwork with the static.
For example I would like to add the MySQL function MASTER_POS_WAIT(...).

> -ralph
>
> Mathieu Suen wrote:
>> Hi,
>>
>> I think that the Zend_Db_Select::assemble method is bugy.
>> I would expected something like:
>>
>>     /**
>>      * Converts this object to an SQL SELECT string.
>>      *
>>      * @return string This object as a SELECT string.
>>      */
>>     public function assemble()
>>     {
>>         $sql = self::SQL_SELECT;
>>         foreach (array_keys($this->_parts) as $part) {
>>             $method = '_render' . ucfirst($part);
>>             if (method_exists($this, $method)) {
>>                 $sql = $this->$method($sql);
>>             }
>>         }
>>         return $sql;
>>     }
>>
>>
>> If so I can create un bug report. With the atteched patch
>>
>> Thanks
>>

--
-- Mathieu Suen
--

[Ralph Schindler-2]

Ah, you are correct, I must have glazed over the static keyword there. I
am not sure why they were marked as static when the class was created.

Also, I guess I see value in what you are proposing, and see very little
harm from doing it.  Can you file an issue, supply your patch and mark
as improvement?

-ralph

Mathieu Suen wrote:

> Ralph Schindler a écrit :
>> This not a bug, IMO.  Zend_Db_Select uses $_partsInit as a skeleton
>> for what _parts will look like AND for which _render{Part} methods exist.
>>
>> To extend Zend_Db_Select for a new SQL part, you'd have to include its
>> name (as a new constant) into the _partsInit array as well as the
>> corresponding _render{Part}() method.  Basically, its a way of keeping
>> developers honest when it comes to extending Zend_Db_Select for new
>> functionality.  (It also is required for resetting values.)
>>
>> Out of curiosity, why do you feel this is a bug?  Is it hindering a
>> user case?
>>
>
> Yeap, In fact you are breaking reusability of the framwork with the static.
> For example I would like to add the MySQL function MASTER_POS_WAIT(...).
>
>
>
>> -ralph
>>
>> Mathieu Suen wrote:
>>> Hi,
>>>
>>> I think that the Zend_Db_Select::assemble method is bugy.
>>> I would expected something like:
>>>
>>>     /**
>>>      * Converts this object to an SQL SELECT string.
>>>      *
>>>      * @return string This object as a SELECT string.
>>>      */
>>>     public function assemble()
>>>     {
>>>         $sql = self::SQL_SELECT;
>>>         foreach (array_keys($this->_parts) as $part) {
>>>             $method = '_render' . ucfirst($part);
>>>             if (method_exists($this, $method)) {
>>>                 $sql = $this->$method($sql);
>>>             }
>>>         }
>>>         return $sql;
>>>     }
>>>
>>>
>>> If so I can create un bug report. With the atteched patch
>>>
>>> Thanks
>>>
>
>