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

Fix issue with SimplePhpView::create() #183

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix issue with SimplePhpView::create() #183

wants to merge 1 commit into from

Conversation

DanielPlainview
Copy link
Contributor

В данный момент SimplePhpView::create() возвращает инстанс EmptyView. Данный PR позволяет исправить ситуацию таким образом, чтобы сохранить BC.

@stev
Copy link
Contributor

stev commented Mar 26, 2013

если проблема только в возврате EmptyView, через ReflectionClass это уж слишком (:
так не достаточно будет ?

public static function create()
{
    return new static();
}

@DanielPlainview
Copy link
Contributor Author

У них разные конструкторы.

@stev
Copy link
Contributor

stev commented Mar 26, 2013

а да. все равно похоже на хак
интерфейсы у них разные, и это попытка прилепить костыль сохраняя ВС.
по хорошему здесь не должно быть наследование SimplePhpView от EmptyView.

@DanielPlainview
Copy link
Contributor Author

Это не «похоже не хак», а хак и есть. Мне кажется, это очевидно. Я не очень улавливаю мысль, которую вы пытаетесь донести.

@stev
Copy link
Contributor

stev commented Mar 26, 2013

@@ -12,11 +12,16 @@
        /**
         * @ingroup Flow
        **/
-       class SimplePhpView extends EmptyView
+       class SimplePhpView implements View, Stringable
        {
                protected $templatePath         = null;
                protected $partViewResolver     = null;
-               
+
+               public static function create($templatePath, ViewResolver $partViewResolver)
+               {
+                       return new static($templatePath, $partViewResolver);
+               }
+
                public function __construct($templatePath, ViewResolver $partViewResolver)
                {
                        $this->templatePath = $templatePath;

@DanielPlainview
Copy link
Contributor Author

Мы с вами говорим на разных языках. Приведённый код всем очевиден. Его проблема в том, что он ломает BC, поэтому нужен хак.

@crazedr0m
Copy link
Contributor

Простите, ни как не могу увидеть, а чем ломатся BC?

@DanielPlainview
Copy link
Contributor Author

public function render(EmptyView $view)
{
    // ...
}

или какой-нибудь instanceof EmptyView.

@crazedr0m
Copy link
Contributor

ну я не знаю, на мой взгляд привязываться нужно или к SimplePhpView или к View
потому как SimplePhpView чаще всего далеко не empty

@DanielPlainview
Copy link
Contributor Author

@crazedr0m тут рассуждать особо нечего: да, наследоваться так было нельзя. Теперь желательно исправить ситуацию. Либо хаком и с BC, либо нормально.

@stev
Copy link
Contributor

stev commented Mar 26, 2013

public function render(EmptyView $view)

не нашел в коде фрейм-ворка похожего фрагмента.

@crazedr0m +1
не нашел поломки BC

@DanielPlainview
Copy link
Contributor Author

не нашел в коде фрейм-ворка похожего фрагмента

Вы правда надеялись найти это в коде фреймворка? Интересный вы человек. Я говорил про обычный код проекта, в котором используется onPHP.

@suquant
Copy link
Member

suquant commented Mar 26, 2013

:D
Предлагаю сделать другой класс нормальный а этот сделать deprecated

@stev
Copy link
Contributor

stev commented Mar 26, 2013

Уважаемый @DanielPlainview - понаписать у себя в проекте можно все что угодно.
Это не ваш личный ВС.
Поддерживать то что каждый "кулибин" у себя написал, в рамках фреймворка - это утопия.
Ситуация может быть другой если львиная доля участников использует данный кусок.
Посмотрим что скажут остальные.

@AlexeyDsov
Copy link
Member

При изменении наследования BC ломается, если кто-то вдруг проверяет instanceof EmptyView

@dovg
Copy link
Member

dovg commented Apr 1, 2013

@stev мушку спили.

По существу

Предлагаю сделать другой класс нормальный а этот сделать deprecated

+1

Во времена Voxus bc ломали только в путь (со сменой версии), ничего страшного в этом нет. Если тянуть совместимость вечно, то получится винда какая-то.

@AlexeyDsov
Copy link
Member

Впринципе почемы бы и не сломать здесь BC, убрав из цепочки наследования EmptyView. Делать deprecated и потом переименовывать в проекте класс на другой будет сложнее чем пробежаться по использованию EmptyView которого должно быть не много или вообще не быть.

@pupkinV
Copy link
Contributor

pupkinV commented Apr 2, 2013

@dovg святые слова. очень часто здесь употребляется BC и даже тогда, когда оно является Bug Compatibility. Хотя в данном случае действительно изменяется сигнатура метода.

@stev
Copy link
Contributor

stev commented Apr 13, 2013

для 1.0 - deprecated,
для 1.1 - я за поломку ВС (Bug Compatibility) :)

есть еще кроме нас кто работает на прод. с 1.1 ?

PS. и да, если вдруг слова про "Кулибина" могли быть восприняты как-то негативно, то это не в таком ключе говорилось. Кулибин - выдающийся человек, исследователь и изобретатель, и каждого из нас можно в какой-то степени соотнести к нему. (ну мало ли как могут слова быть интерпретированы)

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.

7 participants