Skip to content

Conversation

suquant
Copy link
Member

@suquant suquant commented Mar 27, 2012

Собственно сюда приаттачю решение шде Enumeration реализованно через Static свойства

Немного истории: Сейчас у нас класс Enumeration выглядит как обычный класс с неститчным свойством $names
и совсем непонятно зачем создавать обьект чтобы, к примеру получить ids или получить названия (алиас)

вообщем не логично, по скорости работы и потребелнию памяти мы с stev тестили одинаково ))), он не даст мне соврать )))

@dovg
Copy link
Member

dovg commented Mar 27, 2012

Друзья, а какая глобальная цель этого pullRequest?
Чем не угодил текущий Enumeration с наследниками?

@suquant
Copy link
Member Author

suquant commented Mar 27, 2012

Ну т.к.

Enum::getList();

куда логичнее чем

$enum = new Enumeration($id);
$enum->getList();

собственно текущий enumeration немного костыльный в плане использования!

@dovg
Copy link
Member

dovg commented Mar 27, 2012

т.е. "Showing 19 changed files with 1,493 additions and 13 deletions." ради добавления одного метода? Его можно реализовать по-другому, через lsb и создание временного объекта, например.

собственно текущий enumeration немного костыльный в плане использования!

А какие еще костыли есть, не считая отсутствия возможности получить лист без инстанцирования?

@suquant
Copy link
Member Author

suquant commented Mar 27, 2012

т.е. ты предлагаешь еще один костыль :-) я правельно тебя понимаю Женя?

@suquant
Copy link
Member Author

suquant commented Mar 27, 2012

1,493 additions - там в основном из-за класса MimeType :-)

@AlexeyDsov
Copy link
Member

Причина почему Enumeration такой какой он есть - до 5.3 в php не было позднего статического связывания. От Enumeration'а как такого никто не предлагает отказываться на данный момент и просто подменить его Enum'ом было бы не правильно, а вот параллельно они вполне себе могут существовать.
Большая часть из 1,493 additions приходится на класс MimeType. Вот, кстати, не уверен на счет правильности его названия.

@suquant
Copy link
Member Author

suquant commented Mar 27, 2012

Ну название можно поменять, ну вроде-как нискем не конфликутем!

Да и пора бы уже задумываться об namespace-ах ;)

@stev
Copy link
Contributor

stev commented Mar 28, 2012

@dovg
Его можно реализовать по-другому, через lsb и создание временного объекта, например. это конечно проще, но как не крути костыль.

А какие еще костыли есть, не считая отсутствия возможности получить лист без инстанцирования? есть еще один момент,

К примеру есть testEnumeration класс с 1000 элементами, создаем список объектов, к примеру 20.
var_dump показывает что у Enumeration каждый объект дублирует в себе всю структуру класса testEnumeration
что явно не правильно. Статические данные класса не должны дублироваться, в объектах.
Объекты Enum'а такого недостатка не имеют.

что именно происходит в ядре php пока не выяснял.

@AlexeyDsov AlexeyDsov mentioned this pull request Mar 29, 2012
@crazedr0m
Copy link
Contributor

А мне в свое время понравилось как Денис делал, что для $className+$id всегда был один инстанс.

@AlexeyDsov
Copy link
Member

@crazedr0m а почему не прижился такой вариант?

@suquant
Copy link
Member Author

suquant commented Mar 30, 2012

А мне в свое время понравилось как Денис делал, что для $className+$id всегда был один инстанс.

Это типа синглтона ? ))) ну так это костыль все-же получается

Все методобы добится того чтобы работал старый класс Enumeartion вместо Enum приведут к заксотылеванию )))

@crazedr0m
Copy link
Contributor

"типа синглтона", но тогда речь шла не о модификации текущего Enumeartion а о новой реализации с использованием get_called_class.

@AlexeyDsov это было из другого проекта )

@stev
Copy link
Contributor

stev commented Apr 3, 2012

$className+$id всегда был один инстанс.

это уже оптимизация. Реализация Enum'a - позволяет добавить эту возможность без поломки интерфейса.

@AlexeyDsov
Copy link
Member

@stev совместимость c Enumeration ломается поболее. То есть если вдруг кто-то менял состояние enumeration'а через setId - то это работать уже не будет корректно. Просто приведет к страшным вещам.

А вот если б от последствий защититься, то почему бы тогда не сделать.

@stev
Copy link
Contributor

stev commented Apr 3, 2012

@AlexeyDsov где мной про совместимость с Enumeration написано?

я уже как бы мыслями в 1.1.x и Enumeration - не рассматриваю :)

@AlexeyDsov
Copy link
Member

в 1.1.x Enumeration все так же будут, но deprecated.
Так же если их сделать уникальными для каждого идентификатора, то Enum нельзя будет создавать через new HttpStatus(HttpStatus:CODE_404);
Еще сложнее переходить от Enumeration к Enum.

@stev
Copy link
Contributor

stev commented Apr 3, 2012

..то Enum нельзя будет создавать через new HttpStatus(HttpStatus:CODE_404);

через static в __construct. (не сторонник, для совместимости допускаю) Боле того можно настроить mapping и стратегию.. для определенных Енумов, с прописыванием в конфиге, на подобии дао воркеров, только пока зачем?

про "не легкий переезд" - согласен, это сломает BC так как Enumeration прошит и отказ от него пока не произойдет, несколько этапов понадобится. В итоге апнуть первый индекс "новой вехи" и в отдельную ветку. Новые проекты уже на этом делать. onPHP не раз менялся ломая BC, а что делать, нужно развиваться.

@AlexeyDsov
Copy link
Member

@stev Новые проекты это круто, но не каждый день их начинаешь. И обновления хочется максимально использовать в текущих проектах. Патчи и фичи в onPHP идут из них, а не из будуйщих проектов.

Перейти с Enumeration'а на вариант который сейчас в Pull Request'е, вполне реально при этом не сильно перелопачивая весь код. Нужно буквально поправить сами Enumeration'ы и изменить их тип в мете. Если добавить уникальные Enum'ы - это будет требовать большего рефакторинга и внимания - надо просматривать/искать все строки кода которые их используют по всему проекту.

@suquant
Copy link
Member Author

suquant commented Apr 3, 2012

Кстати логично было бы сделать в случае вызова метода setId у Enum-а выкидывать UnsupportedMethodException ?
Как вы на это смотрите?

Copy link
Member

Choose a reason for hiding this comment

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

Еще раз рассматривал классы, вот тут вот можно сделать не через get_called_class(), а просто:
return new static($id);

@suquant
Copy link
Member Author

suquant commented Apr 25, 2012

Небольшие правки + имплементирование интерфейса ListedPrimitive

@AlexeyDsov
Copy link
Member

Вроде теперь все сделано?

@suquant
Copy link
Member Author

suquant commented Apr 25, 2012

Я думаю можно мержить, если что полезет то в порядке фиксов или фич :)

Отправлено с iPhone

26.04.2012, в 0:02, Alexey [email protected] написал(а):

Вроде теперь все сделано?


Reply to this email directly or view it on GitHub:
#82 (comment)

AlexeyDsov added a commit that referenced this pull request Apr 27, 2012
Static Enumeration aka Enum
@AlexeyDsov AlexeyDsov merged commit efba9a7 into onPHP:master Apr 27, 2012
@AlexeyDsov
Copy link
Member

Замержил. Соотвественно в мастере (1.1) оно будет, а в 1.0 не идет.

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