Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Миграция ошибки из Формы в Примитив #101

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

suquant
Copy link
Member

@suquant suquant commented May 16, 2012

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

Раньше кем-то было свойство в BasePrimitive::$customError собсна это оно и есть, но не доработанно наверно оказалось.

@crazedr0m
Copy link
Contributor

Кто сказал что не правильно?
Простой контейнер для примитивов - PlainForm
Ошибки могут быть не только по конкретному примитиву но и по их комбинации - RegulatedForm::checkRules()

offtopic:
В наш век проверки орфографии практически во всех приложениях ..... "неправельно"

$this->primitive->import(
array($this->primitive->getName() => $scope[$this->name])
$this->primitive->importValue(
$scope[$this->name]
);
Copy link
Contributor

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.

Все старые тесты работают :-)

@suquant
Copy link
Member Author

suquant commented May 16, 2012

В наш век проверки орфографии практически во всех приложениях ..... "неправельно"

упс :-)

Ошибки могут быть не только по конкретному примитиву но и по их комбинации - RegulatedForm::checkRules()

По идее Rul-езы должны так-же быть примитивами, раньше где-то, в какой-то ветке был чудо примитив PrimitiveRule

Простой контейнер для примитивов - PlainForm

Точно? Ты глянь его ))) Он совсем не прост ;) Тут я про Form!!!

if (null === $result) {
if ($prm->isRequired())
$this->errors[$name] = self::MISSING;
$this->markCustom($name, BasePrimitive::MISSING);
Copy link
Member

Choose a reason for hiding this comment

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

по описанной идее за MISSING должен отвечать примитив а не Form

Copy link
Member Author

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 17, 2012

Логичное предложение +1

@AlexeyDsov
Copy link
Member

Кажется тут пока коммитить рано, должно же ведь быть продолжение.

@suquant
Copy link
Member Author

suquant commented May 21, 2012

Добавил в BasePrimitive:

  • $label название filed-а
  • $description описание $field-а
  • Покрыл тестами

Это вроде как минимальный набор для отображения и описания что за примитив на человеко-понятном языке

Вынес все типы ошибок и описания самих ошибок в BasePrimitive.
тесты все работаю BC не нарушаем пока :-)

Primitive::rule($name)
->setExpression($rule)
->setForm($this)
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Coding style convention.

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.

как минимум стрелочеки не с той стороны

Copy link
Contributor

Choose a reason for hiding this comment

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

Primitive::rule($name)->
setExpression($rule)->
setForm($this)

@AlexeyDsov, именно )

@pupkinV
Copy link
Contributor

pupkinV commented Dec 17, 2012

Георгий, предлагаю свою помощь, т.к. ветка pupkinV@f650741 явно конфликтует с твоим здоровенным реквестом и я жду

@suquant
Copy link
Member Author

suquant commented Dec 17, 2012

Да тут довльно большой реквест, тут я концептуально переделал работу с примитивами и контейнером Form
У вас там небольшие изминения свзяанные с генератором, как я понимаю и наверно лучше все-же отдельным реквестом пустить, не ? :)

Conflicts:
	core/Form/Form.class.php
	test/core/AggregateCacheTest.class.php
	test/core/SequentialCacheTest.class.php
@suquant
Copy link
Member Author

suquant commented Dec 20, 2012

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

В общем нужно переколбасить практически все примитивы, т.е. теперь у нас примитив отвечает за импорт и проставление у себя ошибки.

В общем нужны желающие которые готовы взять на себя хоть часть примитивов и заимплементить туда всю эту логику :) просто один я долго это делать буду
Если таковые найдутся прошу отписать тут за какие примитивы вы возметесь в рамках этого пул реквеста

P.S. Из приятных новостей начал играться с Трависом (рядом с пул реквестом Merge with caution) выше ссылка на билд в трависе этого бранча ;)

@pupkinV
Copy link
Contributor

pupkinV commented Dec 20, 2012

как пионер -- всегда готов. можешь посоветовать или самому выбирать?

@suquant
Copy link
Member Author

suquant commented Dec 20, 2012

@pupkinV собсно какие захочешь ;)

@stev
Copy link
Contributor

stev commented Dec 20, 2012

Если что-то останется, приму участие днем. Помнится, мы это уже проделывали на работе 5

@pupkinV
Copy link
Contributor

pupkinV commented Dec 21, 2012

#101 (comment) а я чёт не понял. Как твою ветку склонировать? Понаделали кнопок!
Ага спасиб. Все ок. @soloweb можно тебя попросить котакты, ибо тебе придется меня малость скоординировать, а сюда писать -- много текста.

@suquant
Copy link
Member Author

suquant commented Dec 22, 2012

@pupkinV Мне можно в jabber писать

а я чёт не понял. Как твою ветку склонировать? Понаделали кнопок!

Правильнее это форкнутся от моей ветки после делать мне пул реквесты с указанием того бранча в которую ты хочешь мне запулть свой реквест :)


public function __clone()
{
$this->form = clone $this->form;
Copy link
Contributor

Choose a reason for hiding this comment

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

я надеюсь, что более опытные товарищи меня поправят, но если это планируется использовать формой, то после клонирования у каждого примитива окажется свой собсвенный клон формы.
Вызов до определения setForm или setExpression взможен?

Copy link
Member

Choose a reason for hiding this comment

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

Тоже кажется что клонировать форму тут не надо.
Так же PrimitiveRule и все связанное с ним отсюда вообще желательно в отдельный request вынести что б не мешался и ему не мешали.

Copy link
Contributor

Choose a reason for hiding this comment

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

в отдельный request вынести

при детальном рассмотрении видно, что лучше одним т.к. порядочно изменится код, как например здесь. Ни перед этим реквестом ни после не удобно.
А вот тесты предлагаю вперед этого пульнуть.

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
Contributor

Choose a reason for hiding this comment

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

А вот тесты предлагаю вперед этого пульнуть.

да ну что ты такое говоришь!? я в первую очередь задумался о том как будут смотреть, проверять. без тестов можно чокнуться даже пытаясь помочь добить этот пулл =)
Про ссылка "здесь" -- там весь diff состоит из выпиливания использования отдельных $this->errors и $this->violated, а также $this->primitives[$name] и $this->rules[$name]. Все методы будут перепилены дважды. Если один пулл создаст много проблем, можно распилить его.

@pupkinV
Copy link
Contributor

pupkinV commented Dec 22, 2012

беру три примитива на букву A: alias any arr

@suquant
Copy link
Member Author

suquant commented Dec 22, 2012

У меня в профиле почту gmail можно подсмотреть :)

22.12.2012, в 20:19, Vasiliy [email protected] написал(а):

@soloweb > Мне можно в jabber писать

не понял. там не требуется указывать кому писать?


Reply to this email directly or view it on GitHub.

@pupkinV
Copy link
Contributor

pupkinV commented Dec 24, 2012

все что касается импорта и экспорта предлагаю перенести в PlainForm или наоборот из PlainForm вытащить в Form все getValue. Получается что мы уже тут знаем о value но нет ни малейшего представления как это происходит.
Правильнее было бы оставить:

add(BasePrimitive $prm)
set(BasePrimitive $prm)
get($name)
drop($name)

exists($name) // жаль не участвовал в создании. было бы has($name)
primitiveExists($name)

getPrimitiveNames()
getPrimitiveList()

и именно в таком порядке.
Тот-же clean уже не комильфо и затрагивает реализацию примитивов.
Это оч. важно т.к. я помогаю пилить @soloweb эту ветку. Какие будут мнения?

}

return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

правильно. ООП-шно. но hasError не имеет, более, смысла в контексте этих изменений, ибо если err !== null вернуть err иначе вернуть null =)

@pupkinV
Copy link
Contributor

pupkinV commented Dec 24, 2012

PlainForm не перестает радовать!
getChoiceValue и getActualChoiceValue
могут бросить WrongArgumentException, тогда как это UnsupportedMethodException
Стоит ли эти методы использовать? Может $form->get('anyListedPrimitive')->getNotBasedMethods(), а?

@pupkinV
Copy link
Contributor

pupkinV commented Dec 24, 2012

Схожесть методов PlainForm, BasePrimitive и PrimitiveAlias наталкивает на мысль о интерфейсе.
PrimitiveAlias extends BasePrimitive -- ой ли? едва ли расширяет, не более чем проксирует все методы.

private $strategy = null;

private $label = null;
private $description = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

@soloweb вот тут прям не понял, простите, совсем. Label нужон форме чтобы вывести возле инпута в тэг label (в больш. случ.).
И вот его в LightMetaProperty? А get_text? У нас на сайте планируется _('все тексты в последствии "мультиязычить" ');
Хотя может кому и будет полезным. Ну и виджеты видимо без этого не взлетят?

Copy link
Member Author

Choose a reason for hiding this comment

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

А почему-бы опционально в мете не укзать label и description ммм ?
Удобно

Copy link
Contributor

Choose a reason for hiding this comment

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

хех! удобно не то слово. я бы даже в Женин( @stev ) make schema sql впилил

 COMMENT ON COLUMN "person"."name" IS 'КО заявляет, что сие -- есть имя';

исходя из твоих изменений! это здорово, но вот слишком часто от программиста требуют в одной вьюшычке вот так, а в другой вот эдак. И мультиязычность затрудняет с этим ихним poedit.
Я вот еще не уверен что понял почему их два, т.е. в чем принципиальное отличие? Не забывай что default посеяли и в сущностях он есть, а вот форма рождается без него ;) Мне не жалко конечно но на prodaction версии у меня уже 13 параметров (там еще formAlias), еще 2 твоих возможно скоро прикачуют ;)
Да и самое главное -- сделать это отдельным пулом! Отдельный пулл с тестами Form у меня уже зреет.

Copy link
Member Author

Choose a reason for hiding this comment

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

По поводу мултиязычности, тут наверно он снаруже должен приходить уже в мулти _() но вот с xml проблема, но для метагенератора тогда запускать _() те внтури метагенератора будет идти присвоение не просто $label а _($label) тогда смело можно переводить :)

Copy link
Contributor

Choose a reason for hiding this comment

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

'firstName' => LightMetaProperty::fill(new LightMetaProperty(), 'firstName', 'first_name', 'mountain of parameters', _('Имя'), _('Имя, которым Вы будете представлены на форуме')),

ты про такое? Ой боюсь. Я вот честно не уверен, что это одобрят (это не сгущая красок =)

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