Добавлена возможность кэширования токена авторизации #23

rmrevin · 2016-03-29T15:15:23Z

Обновлён стиль кода, добавлены phpdoc комментарии, добавлен composer.lock и .gitignore
Добавлена возможность кэширования токена авторизации

…lock и .gitignore

zelenin · 2016-03-29T15:37:55Z

Благодарю. Но в таком виде не приму.
а) composer.lock не нужен в пакете
б) с кэшированием согласен - с реализацией нет.
в) убрать phpdoc из шапок классов - не несет смысловой нагрузки

Если готовы исправить, прокомментирую пункт б).

rmrevin · 2016-03-29T15:42:51Z

composer.lock вполне пригождается, т.к. я хочу ещё юнит тесты добавить, а с какими версиями библиотек используется пакет, неясно (только самые свежие, не все и не всегда обновляют пакеты в своих проектах постоянно).
Какую реализацию кэширования Вы можете предложить? Мне не хотелось сильно усложнять ещё одной зависимостью от пакета кэширования или вносить механизм кэширвоания внутрь пакета.
phpdoc у классов хороший тон, т.к. позже можно будет добавить описания или проперти (у меня, например, шторм настроен подсвечивать методы и классы без доков.).

zelenin · 2016-03-29T16:04:28Z

а) ок
б) без объединения ответственностей и статики. Token - value-object, хранящий только токен (приватный проперти, и геттер) (можно и строкой - не умрем).
Вводим интерфейс Zelenin\SmsRu\Auth\TokenCache\Cache (get/set), реализуем FileCache и NullCache (get всегда возращает null). FileCache в конструктор принимает путь до файла, по умолчанию null. Если null, то назначаем что-то в /tmp/... - в ОСях нет никаких ./../runtime.
https://github.com/zelenin/sms_ru/blob/master/Auth/LoginPasswordSecureAuth, передаем в конструктор Cache (по умолчанию null). Если null, то записываем в $this->cache = new NullCache;
в authGetToken() соответственно

$token = $this->cache->get();
if($token === null) {
        $token = $this->getContext()->getClient()->request('auth/get_token');
        $this->cache->set($token);
}
return $token;

в) про классы не согласен - для иде это не нужно, документацию я не генерю. phpdoc оставляем только для свойств и методов.

rmrevin · 2016-03-29T16:13:47Z

Вводим интерфейс

Ну вот собственно то, чего я не хотел делать - реализовывать кэширование внутри пакета. Лучше тогда добавить зависимость от какого-нибудь пакета кэширования, т.к. файловый кэш на чтение всё таки хромает под нагрузками.

про классы не согласен

С чем Вы не согласны? Что писать phpdoc - хороший тон?

zelenin · 2016-03-29T16:17:03Z

Ну вот собственно то, чего я не хотел делать - реализовывать кэширование внутри пакета. Лучше тогда добавить зависимость от какого-нибудь пакета кэширования, т.к. файловый кэш на чтение всё таки хромает под нагрузками.

ну собственно вы уже реализовали кэширование. я лишь предлагаю добавить интерфейс и расхардкодить.

С чем Вы не согласны? Что писать phpdoc - хороший тон?
c phpdoc в шапке класса - https://github.com/rmrevin/sms_ru/blob/master/Auth/ApiIdAuth.php#L6 вот это.

rmrevin · 2016-03-29T16:19:38Z

c phpdoc в шапке класса

Я понимаю о каких доках идёт речь. Непонимаю, чем они мешают. Почему они недопустимы.

Кэширование переделаю сейчас.

zelenin · 2016-03-29T16:26:21Z

Я понимаю о каких доках идёт речь. Непонимаю, чем они мешают. Почему они недопустимы.

они не несут смысловой нагрузки - не помогают иде хинтить, не нужны при генерации документации. Я хочу сохранить лаконичность классов, не загромождая шапку.
Хорошая практика - это, когда мы что-то делаем для профита.

Кэширование переделаю сейчас.

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

rmrevin · 2016-03-29T16:30:36Z

Ок. Уберу доки.

Я планирую просто добавить библиотеку для кеширования. Разраб сам уже решит, какой ему адаптер кэша лучше подойдет.

zelenin · 2016-03-29T16:33:06Z

Я планирую просто добавить библиотеку для кеширования. Разраб сам уже решит, какой ему адаптер кэша лучше подойдет.

не надо. это будет очень "тяжело" и излишне. Одного вашего файлового адаптера вполне хватит. а интерфейс позволит разработчику самому прикрутить любую другую библиотеку.

rmrevin · 2016-03-29T18:53:14Z

Готово

zelenin · 2016-03-29T19:43:36Z

Auth/LoginPasswordSecureAuth.php

@@ -20,15 +23,27 @@ class LoginPasswordSecureAuth extends AbstractAuth
 private $apiId;

 /**
+ * @var CacheInterface|null
+ */
+ public $Cache;


пожалуйста, приватный и с маленькой буквы

zelenin · 2016-03-29T19:45:24Z

Auth/LoginPasswordSecureAuth.php

+ /**
+ * @return string
+ */
+ protected function requestAuthToken()


protected => private

rmrevin · 2016-03-29T19:47:35Z

Скажите, каким style гайдом Вы пользуетесь?
Почему Вы везде используете private, а не protected?
Вы не предполагаете, что кто-то захочет унаследоваться и расширять функциональность внутри своего приложения?
В случае private этого сделать не выйдет.

zelenin · 2016-03-29T19:48:03Z

Client/Client.php

@@ -35,7 +38,7 @@ public function request($method, $params = [])
 *
 * @return string
 */
- private function getUrl($method)
+ protected function getUrl($method)


rmrevin · 2016-03-29T20:05:40Z

Готово.
Ответьте на коммент выше.

zelenin · 2016-03-29T20:06:24Z

Почему Вы везде используете private, а не protected?

best practices. https://www.reddit.com/r/PHP/comments/3jdbkd/protected_or_private_as_a_default_visibility_and/
по умолчанию private, при необходимости повышаем.
Для хорошего кода нам надо юзать интерфейсы, а не наследование.

zelenin · 2016-03-29T20:09:00Z

если хотите, можем в личке пообщаться - skype:zelenin_av например

rmrevin · 2016-03-29T20:11:20Z

Вот тут сразу же возникает проблема. Чтобы сделать кеширование токена в своём проекте, мне потребуется реализовывать полностю класс LoginPasswordSecureAuth с кэшированием, чтобы передать его в api. Вместо перезагрузки одного метода.

По поводу общения мне тут удобно. Возможно эта информация пригодится для будущих поколений =)

zelenin · 2016-03-29T20:20:29Z

Вот тут сразу же возникает проблема. Чтобы сделать кеширование токена в своём проекте, мне потребуется реализовывать полностю класс LoginPasswordSecureAuth с кэшированием, чтобы передать его в api. Вместо перезагрузки одного метода.

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

Возможно эта информация пригодится для будущих поколений =)

это верно

zelenin · 2016-03-29T20:22:57Z

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

rmrevin · 2016-03-30T16:05:31Z

Очередная проблема. Я не могу изменить url в клиенте.
Я хочу использовать https, а механизма для изменения private $baseUrl нет.

Что с этим пулреквестом?

zelenin · 2016-03-30T20:52:56Z

Очередная проблема. Я не могу изменить url в клиенте.

Роман, я предлагал пообщаться в личке, потому что не хотел публично рассказывать банальные вещи.
Данная версия библиотеки написана с использованием SOLID - разграничение ответственности (у нас есть отдельный Client например), есть разделение интерфейсов (подо все заведены интерфейсы), есть подстановка Лисков (мы можем дополнить поведение, но не менять его - к вопросу о приватности), ну и инверсия зависимостей (не библиотека навязывает зависимость, а разработчик ее сеттит). SOLID - базовая парадигма хорошего кода.
Практически все не-стилистические правки, описанные выше, касались именно этого. Например Token, который хранил токен, а еще почему-то умел доставать его из кэша.
Чем больше обязанностей у класса, тем больше вероятность превратить код в кашу, наплодить ошибок и сделать код неподдерживаемым.
Я знаю вас как yii-разработчика, но yii к сожалению практически не использует лучшие парадигмы, лучшие практики, накопленные программистами за десятки лет.

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

pr сейчас проверю.

zelenin · 2016-03-30T20:57:03Z

еще раз прошу прощения за мозгопарство. сейчас проставлю тег релизу.

rmrevin · 2016-03-30T21:04:38Z

Я понимаю что такое солид и для чего он нужен. Тут вопрос в том, что для того, чтобы сконфигурировать стороннее расширение вы предлагаете реализовать свой клиент. По-моему это уже перебор. Не забывайте о том, что чрезмерное кол-во слоев и абстракций это не best practices.

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

zelenin · 2016-03-30T21:19:01Z

Тут вопрос в том, что для того, чтобы сконфигурировать стороннее расширение вы предлагаете реализовать свой клиент.

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

Не забывайте о том, что чрезмерное кол-во слоев и абстракций это не best practices.

здесь самый минимум

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

я вам говорил, что реализовывать ничего не надо. надо было лишь вынести кэш из Token.
Я удалю все кроме FileCache, если вы не возражаете. Тестировать я не буду, а интерфейса хватит, если разработчик захочет хранить токен где-то кроме файла.

rmrevin added 2 commits March 29, 2016 17:50

Обновлён стиль кода, добавлены phpdoc комментарии, добавлен composer.…

2101784

…lock и .gitignore

Добавлена возможность кэширования токена

f8a9961

Убраны phpdoc комментарии классов

278b097

Доработан интерфейс кэширования

0450027

Закрыт метод получения токена

ec652a6

zelenin reviewed Mar 29, 2016
View reviewed changes

Фикс

c9dd908

zelenin reviewed Mar 29, 2016
View reviewed changes

Auth/LoginPasswordSecureAuth.php

/**

* @return string

*/

protected function requestAuthToken()

Copy link

Owner

zelenin Mar 29, 2016

Choose a reason for hiding this comment

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

protected => private

zelenin reviewed Mar 29, 2016
View reviewed changes

Фикс

8d22a0b

Фикс

1adf35a

zelenin merged commit 654f28c into zelenin:master Mar 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Добавлена возможность кэширования токена авторизации #23

Добавлена возможность кэширования токена авторизации #23

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin Mar 29, 2016

zelenin Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 30, 2016

zelenin commented Mar 30, 2016

zelenin commented Mar 30, 2016

rmrevin commented Mar 30, 2016

zelenin commented Mar 30, 2016

Добавлена возможность кэширования токена авторизации #23

Добавлена возможность кэширования токена авторизации #23

Conversation

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin Mar 29, 2016

Choose a reason for hiding this comment

zelenin Mar 29, 2016

Choose a reason for hiding this comment

rmrevin commented Mar 29, 2016

zelenin Mar 29, 2016

Choose a reason for hiding this comment

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 29, 2016

zelenin commented Mar 29, 2016

zelenin commented Mar 29, 2016

rmrevin commented Mar 30, 2016

zelenin commented Mar 30, 2016

zelenin commented Mar 30, 2016

rmrevin commented Mar 30, 2016

zelenin commented Mar 30, 2016