UnifiedContainer clone fix + test

[avid]

Итак, еще раз предлагаю diff по issue #7.
Переопределен метод __clone и добавлен тест на этот случай для версии 1.1

Примеры кода с результатами работы

Meta

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE metaconfiguration SYSTEM "meta.dtd">
<metaconfiguration>
    <classes>
        <class name="Company">
            <properties>
                <identifier />
                <property name="name" type="String" size="255" required="true" />
                <property name="workers" type="Worker" relation="OneToMany" />
            </properties>
            <pattern name="StraightMapping" />
        </class>

        <class name="Worker">
            <properties>
                <identifier />
                <property name="name" type="String" size="255" required="true" />
                <property name="active" type="Boolean" default="true" required="true" />
                <property name="company" type="Company" relation="OneToOne" required="true" />
            </properties>
            <pattern name="StraightMapping" />
        </class>
    </classes>
</metaconfiguration>

Данные в БД

test=# SELECT * FROM "company";
 id |    name     
----+-------------
  1 | TestCompany
(1 row)


test=# SELECT * FROM "worker";
 id |   name    | active | company_id 
----+-----------+--------+------------
  1 | Worker 1  | t      |          1
  2 | Worker 2  | t      |          1
  3 | Worker 3  | f      |          1
  4 | Worker 4  | t      |          1
  5 | Worker 5  | t      |          1
  6 | Worker 6  | f      |          1
  7 | Worker 7  | t      |          1
  8 | Worker 8  | t      |          1
  9 | Worker 9  | f      |          1
 10 | Worker 10 | t      |          1
(10 rows)

Обращаю ваше внимание на то, что сущности Worker с id 3,6 и 9 имеет значение FALSE в поле active.

Добавляем два своих метода

Методы будут получать только активных и только неактивных работников у заданной компании соответственно.

<?php
/*****************************************************************************
 *   Copyright (C) 2006-2009, onPHP's MetaConfiguration Builder.             *
 *   Generated by onPHP-1.1.master at 2013-01-15 00:06:35                    *
 *   This file will never be generated again - feel free to edit.            *
 *****************************************************************************/

    class Company extends AutoCompany implements Prototyped, DAOConnected
    {
        /**
         * @return Company
        **/
        public static function create()
        {
            return new self;
        }

        /**
         * @return CompanyDAO
        **/
        public static function dao()
        {
            return Singleton::getInstance('CompanyDAO');
        }

        /**
         * @return ProtoCompany
        **/
        public static function proto()
        {
            return Singleton::getInstance('ProtoCompany');
        }

        // your brilliant stuff goes here

        /**
         * @param bool $lazy
         * @return CompanyWorkersDAO
         */
        public function getActiveWorkers($lazy = false) {
            $container = clone $this->getWorkers($lazy = false);
            $container->setCriteria(
                Criteria::create()
                    ->add(Expression::isTrue('active'))
            );
            return $container;
        }

        /**
         * @param bool $lazy
         * @return CompanyWorkersDAO
         */
        public function getInactiveWorkers($lazy = false) {
            $container = clone $this->getWorkers($lazy = false);
            $container->setCriteria(
                Criteria::create()
                    ->add(Expression::isFalse('active'))
            );
            return $container;
        }

    }
?>

Код теста

Во всех трёх тестах мы получаем количество сущностей Worker, привязанных к заданной Company, используя три разные функции в разных порядках.

<?php
require_once '../config/config.inc.php';

/** @var $company Company */
$company = Company::dao()->getById(1);

echo "Test 1 (All/Active/Inactive):";
print_r(
    array(
        'all' => $company->getWorkers()->getCount(),
        'active' => $company->getActiveWorkers()->getCount(),
        'inactive' => $company->getInactiveWorkers()->getCount(),
    )
);
echo "\n";

echo "Test 2 (Active/All/Inactive):";
print_r(
    array(
        'active' => $company->getActiveWorkers()->getCount(),
        'all' => $company->getWorkers()->getCount(),
        'inactive' => $company->getInactiveWorkers()->getCount(),
    )
);
echo "\n";

echo "Test 3 (Active/Inactive/All):";
print_r(
    array(
        'active' => $company->getActiveWorkers()->fetch()->getCount(),
        'inactive' => $company->getInactiveWorkers()->fetch()->getCount(),
        'all' => $company->getWorkers()->fetch()->getCount(),
    )
);
echo "\n";

В последнем тесте специально вставил перед getCount() вызов fetch(), ибо наличие/отсутсвие fetch() ни на что не внияет.

Результаты

Перейдем к самому вкусному :)

Без патча

php test.php 
Test 1 (All/Active/Inactive):Array
(
    [all] => 10
    [active] => 7
    [inactive] => 3
)

Test 2 (Active/All/Inactive):Array
(
    [active] => 7
    [all] => 7
    [inactive] => 3
)

Test 3 (Active/Inactive/All):Array
(
    [active] => 7
    [inactive] => 3
    [all] => 3
)

С патчем

php test.php 
Test 1 (All/Active/Inactive):Array
(
    [all] => 10
    [active] => 7
    [inactive] => 3
)

Test 2 (Active/All/Inactive):Array
(
    [active] => 7
    [all] => 10
    [inactive] => 3
)

Test 3 (Active/Inactive/All):Array
(
    [active] => 7
    [inactive] => 3
    [all] => 10
)

Итоги

Разница видна невооружённым глазом.
В результатх работы кода без пачта видно, что во втором и третьем тестах общее количество сотрудников неверно.
Используя патч, подобных проблем мы не наблюдаем, и во всех случаях цифры верные.

P.S. Выжимки из предыдущего обсуждения:

soloweb commented:

Ну собственно вот кейс:

$container1 = $products->getVariants();
$container2 = $products->getVariants()->setCriteria(
Criteria::create()->add(
bla-bla-bla-expression(s)
)
);

$list1 = $container1->getList();
$list2 = $container2->getList();

Без этого фикса у нас получится что $list1 == $list2

Neerrar replied:

Иначе есть только 1 вариант.
Такой метод генериться автоматом и лежит в Auto класс:
public function getEncapsulants($lazy = false)
{
if (!$this->encapsulants || ($this->encapsulants->isLazy() != $lazy)) {
$this->encapsulants = new TestUserEncapsulantsDAO($this, $lazy);
}
return $this->encapsulants;
}

А мы делаем свой метод в унаследованном как-то примерно так:
public function getNewEncapsulants($lazy = false)
{
$conatiner = new TestUserEncapsulantsDAO($this, $lazy);
$conatiner->setCriteria(...);
return $conatiner;
}

Но мне такой способ кажется очень неудобным.

AlexeyDsov commented:

Справедливости ради надо отметить что то dao которое кланируется совсем не синглтон.

crazedr0m commented:

я к тому, что пока не понял зачем вообще клонировать контейнер

AlexeyDsov commented:

По большому счету контейнер - это просто простой способ получить список объектов которые ссылаются на объект - владелец контейнера а так же сохранить-модифицировать этот список. При этом сейчас в нем использовать setCriteria опасно, т.к. в одном месте сделаешь что б список доставался с дополнительными условиями, а удалить потом не удалишь и в другом месте там где не ожидаешь и где не надо будешь получать список с этими дополнительными условиями...

Кстати, тут походу нужно в таком случае дописать метод __clone также и у UnifiedContainerWorker'а

crazedr0m commented:

@Neerrar @soloweb Если еще актуально, сделайте отдельную ветку и с неё пулл реквест.

[dovg]

Я вот тоже не понимаю зачем клонировать контейнер.

public function getInactiveWorkers

Мы так не пишем :) Если нужны неактивные рабочие, то можно вернуть сразу их, а не контейнер. Причем для этого критерию можно построить по Worker::dao()

[avid]

Мы так не пишем :) Если нужны неактивные рабочие, то можно вернуть сразу их, а не контейнер. Причем для этого критерию можно построить по Worker::dao()

Это же не значит, что так делать нельзя?! К тому же на мой взгляд, достаточно удобно. Ведь в моем примере можно использовать такие преимущества UnifiedContainer как lazy и getCount. А в предложенном @dovg варианте придется написать аж три метода: для получения списка объектов, для получения списка идентификаторов и для получения количества.

Если же вы считаете, что такое применение некоррректно, то метод __clone у UnifiedContainer надо сделать protected.

[AlexeyDsov]

Моё мнение всё такое же - почему бы и не клонировать. Никто не требует использовать клонирование, но руки оно немного развязывает.
Так же мне кажется раз уж происходит клонирование, то нужно клонировать и свойство UnifiedContainerWorker->$criteria

[avid]

Так же мне кажется раз уж происходит клонирование, то нужно клонировать и свойство UnifiedContainerWorker->$criteria

Не проблема. Добавить?

[AlexeyDsov]

Если не только мне кажется правильным, то добавить. Иначе разубедить :)