Skip to content

PrimitiveString->import must be protected #155

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

Closed
DanielPlainview opened this issue Dec 11, 2012 · 13 comments
Closed

PrimitiveString->import must be protected #155

DanielPlainview opened this issue Dec 11, 2012 · 13 comments

Comments

@DanielPlainview
Copy link
Contributor

PrimitiveString:

    class PrimitiveString extends FiltrablePrimitive
    {
        // ...

        public function import($scope)
        {
            // ...
        }
    }

BasePrimitive:

    abstract class BasePrimitive
    {
        // ...

        protected function import($scope)
        {
            // ...
        }
    }
@AlexeyDsov
Copy link
Member

У PrimitiveIdentifier так же к слову. Ошибка ли? Неа. Form вызывает у примитивов import

@DanielPlainview
Copy link
Contributor Author

Значит ошибка в BasePrimitive и там должен быть public. К тому же в таком случае BC сохраняется.

@pupkinV
Copy link
Contributor

pupkinV commented Dec 16, 2012

В чем именно ошибка? Расширять видимость - допустимо. Исползовать import от BasePrimitive унаследовавшись и не переопределяя - недопустимо. А переопределяя вы получаете доступ к protected родителя. По моему тонко. А? DanielPlainview, как Вы считаете?

@DanielPlainview
Copy link
Contributor Author

Расширять видимость - допустимо.

Покажите это в документации.

IDE подчеркивает вызов метода import() как недопустимый в контексте других классов, если в phpDoc или TypeHinting'е указан BasePrimitive. Не существует ни одной причины использовать эту сомнительную недокументированную возможность.

@pupkinV
Copy link
Contributor

pupkinV commented Dec 16, 2012

Покажите это в документации.

может и недокументирован. по факту разрешено. если Вы попробуете public перековать в protected, то такой код не будет работать в принципе. расширять никто не запрещает <-- так справедливее (видать опечатался =), даже нотиса не видать при E_ALL.
а собсна, что за ide? может в ней проблема закралась коварная.
кстать о документации, там есть такие, разрешенные, вещи, что волосы дыбом!

@DanielPlainview
Copy link
Contributor Author

может и недокументирован. по факту разрешено

Вы обещаете нам, что это с какой-нибудь из последующих версий PHP не будет E_STRICT'ом?

@AlexeyDsov
Copy link
Member

Если хочется решить это, то я представил себе два варианта:

  1. Сделать BasePrimitive::import public и все будут счастливы
  2. Другой:
    1. Переименовать protected BasePrimitive::import в protected BasePrimive::innerImport
    2. Добавить abstract public BasePrimitive::import для наследования
    3. Пройтись по всем примитивам и поправить там где они использую parent::import из BasePrimitive или просто BasePrimitive::import - заменить на parent::innerImport и на BasePrimitive::innerImport соотвественно.

@DanielPlainview
Copy link
Contributor Author

Если хочется решить это, то я представил себе два варианта

Первый вариант, по-моему, лучше, меньше риск что-то сломать. В onPHP есть некий PrimitiveAnyType, не реализующий public-версию import() метода вообще, например.

@DanielPlainview
Copy link
Contributor Author

soloweb, к сожалению, всё что Вы пишите, видно лишь в оповещениях на почту. В web-интерфейсе сообщения пустые.

Там ничего вроде не должно сломаться, тесты все проходят на ура :)

PrimitiveAnyType не покрыт тестами.

@suquant
Copy link
Member

suquant commented Dec 17, 2012

Я в своем пул реквесте с примитивами решил эту проблему и много других кстати ;)

#101

Там ничего вроде не должно сломаться, тесты все проходят на ура :)

@pupkinV
Copy link
Contributor

pupkinV commented Dec 17, 2012

по смыслу, подходит второе: в базовом классе объявлено value, но метод import не знает о нем ничего, что не страшно, если-бы value было объявлено выше. => import - это недоимпорт который обязывает определить процесс впиха raw в value.
Но лично я за решение soloweb: менее трудоемко; уже есть; принципиально не правильно, но роли не играет;
если еще актуально: http://php.net/manual/en/language.oop5.paamayim-nekudotayim.php#example-189
и я так подозреваю никто не описывал как само собой разумеющееся.

@AlexeyDsov
Copy link
Member

решение soloweb накатывать мне страшно :) (хотя часть оттуда иногда хочется накатить, но все никак не добираюсь) + оно не мержится сейчас автоматом и надо значит дорабатывать + 19 changed files with 789 additions and 227 deletions - я бы не назвал это менее трудоёмко.

Про пример из php.net - все круто, но дело в интерфейсе BasePrimitive - раз уж форме говорят что это BasePrimitive то у него нет import и вызывать такой метод она не может (так считает во всяком случае IDE, которое подсвечивает такой вызов). Соотвественно либо форма должна работать с другим интерфейсом который все примитивы должны реализовывать либо у BasePrimitive должен быть этот метод публичным.

@suquant
Copy link
Member

suquant commented Dec 17, 2012

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

Если хочется быстрого фикса то тогда наверно отдельный мерж реквест создавать :)

@ssserj ssserj closed this as completed Dec 27, 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

No branches or pull requests

5 participants