Skip to content

Conversation

AlexeyDsov
Copy link
Member

Все три используемые хоть как-то в onPHP базы (Postgres, Mysql и Sqlite) поддерживают savepoint'ы, см. ссылки.

Т.к. всегда очень не нравилось при begin следить - внутри ли мы транзакции или еще нет, то написал класс, который это делает вместо вас. Работает с ним примерно так:

<?php
$transaction = InnerTransaction::begin($dao);
//do your actions
$transaction->commit();

Если транзакция не была начата, то он делает в базе begin/commit/rollback. Если транзакция была начата, то использует savepoint'ы внутри транзакции.

Дополнительно для еще большего уменьшения копи-паста работы, когда нужно выполнить набор действий в транзакции, и при успешном их выполнении сделать commit, а в случае ошибки rollback, написал класс InnerTransactionWrapper, с которым работать примерно так:

<?php
$function = function ($foo) use ($bar) {return $foo.$bar};

$fooBar = InnerTransactionWrapper::create()
    ->setDao($dao)
    ->setFunction($function)
    ->run($foo);

Предлагаю замержить в мастер и если последующим реквестом заменить несколько участков кода в onPHP на использование этого механизма.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему TypeHinting закомментирован? Если его раскоментировать, то он будет пропускать или объект существующего класса, или null.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, действительно, не помню уже почему. Вроде должно работать, попробую раскомментировать.

@dovg
Copy link
Member

dovg commented May 2, 2012

А в целом, +1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А давай вынесем beginTransaction() в метод begin(), что бы при создании объекта он ничего не писал в базу, а только создавался

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А мне текущая реализация нравится :)
$trans = InnerTransaction::begin(); //вполне себе самодокументированно и хорошо.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мне честно говоря самому текущий вариант понравился. Он короче и нагляден и вроде бы нет причин создавать объект раньше чем надо начать трнзакцию.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Логично )))

@stev
Copy link
Contributor

stev commented May 2, 2012

Да. хорошая тема. +1

@suquant
Copy link
Member

suquant commented May 2, 2012

Согласен, реально нужная весчь :-) +1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

на уровне идеи - а может сделать интерфейс, который бы реализовывали оба этих класса и ожидать именно его. DBFull, например.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему бы и нет, к примеру DBTransactional, также в диалекте реализовать эти методы и вроде как бы гуд должно получится :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это сделано было для упрощения работы с классом. Попробую пояснить ход мыслей, когда именно так сделал
Реально классу нужно иметь именно DB под рукой, т.к. именного у него вызывает begin, commit, rollback. Но обычно мы работаем с объектами и у них под боком есть DAO. Из DAO можно вытащить DB, вот так: $db = DBPool::GetByDao(TestObject::dao()); На N'ый раз уже жутко раздражает этот копипаст.
Можно было бы всегда принимать только DAO и из него доставать DB, но это не хорошо, т.к. нет нет, да захочется вдруг передать именно DB и тогда жуть как сложно придется выкручиваться. Сделать два метода с чуть разными названиями и разными аргументами тоже не хочется, т.к. это вносит лишний копипаст и путаницу.
Будь в php перегрузка методов с разными аргументами - не было бы проблем. Собственно по этом и делаем в этом методе вид, что какая-то перегрузка методов есть.
Обий интерфейс придумывать и вешать на классы тоже не хочется ради какой-то новой фичи. Так классы глядишь и обрастут дясятком других интерфейсов. Поэтому я сделал так как оно есть сейчас и усложнять как-то совсем не хочется.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не знаю что имел в виду Женя, но я понял это как некий интерейс для дтранзакционности в БД, сейчас у нас есть "BaseTransaction" и "DBTransaction" которые нигде не используются, ну собсно понятно почему )))
Вот за место них сделать интерфейс DBTransactional и дальше этот интефейс имплементировать в сам дравер и диалект )))

Лично я так подумал.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я имел ввиду то, что описал @AlexeyDsov ;)
Ну нет, так нет, это не так и принципиально.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну значит я не в тему описал.
Ну я за то что сейчас есть ))) вроде как оптимально пока ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Помоему имелось ввиду что-то другое. Про BaseTransaction и DBTransaction я думаю все так же - из-за них begin, commit и т.д. в базе объявлены deprecated но при этом их самих использовать неудобно + inTransaction свойство по ощущениям очень привязано к базе, а за базу отвечает именно DB класс. С другой стороны из DB сложную логику нужно выносить, т.к. вроде бы он достаточно перегружен и всякие наворотам c queue и queueFlush там не особо место, пожалуй.
P.S. пока писал ответы уже появились

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну собсно о чем и речь, и лучше всего думаю это сделать интерфейсами.

@ghost ghost assigned AlexeyDsov May 2, 2012
@AlexeyDsov
Copy link
Member Author

Обновил немножко

  1. Поправил coding-style.
  2. Раскомментировал /* IsolationLevel */ в аргументах.
  3. Добавил методы savepoint(Begin|Release|Rollback) в DB.
  4. Добавил проверку на is_callable в методе InnerTransactionWrapper::setFunction
  5. Пофиксил тесты.

Еще, думаю, теперь надо добавить тесты в DAOTest для проверки работы с реальными базами. Полюс в DB надо сделать валидацию названия savepoint'ов, которые, кстати, никак не эскейпяться.

@AlexeyDsov
Copy link
Member Author

Что-то майские праздники, начало велосезона и последующий единственный выходной неблагоприятно влияют на нерабочее написание кода :)
По задаче, что еще осталось прежде чем мержить.

  1. В DAOTest хочу забацать тестик или два + заменить в тестовом конфиге SQLite на SQLitePDO
  2. Много думал и все таки не нравится здесь искуственный InnerTransactionWrapperException, т.к. заставляет код внутри думать что есть снаружи враппер или нет. Лучше сделать метод InnerTransactionWrapper->setCatchExceptionFunction с помощью которой можно будет задавать свой обработчик ошибки на случай Exception'а. rollback соответственно будет выполнен до обработчика, т.к. если его не выполнить раньше, то в случае, например, ошибки базы она будет отказываться выполнять какие-либо запросы и от обработчика будет мало толку.

Думаю сделаю это все в начале следующей недели.

@AlexeyDsov
Copy link
Member Author

Такс, вроде бы сделал все что хотел. Если ни у кого нет никаких замечаний, то мержу в мастер ближе к выходным.

@AlexeyDsov
Copy link
Member Author

Мержу в мастер, в 1.0 оно не идет.

AlexeyDsov added a commit that referenced this pull request May 18, 2012
@AlexeyDsov AlexeyDsov merged commit 041f8ee into onPHP:master May 18, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants