PHP-библиотека для работы с API Яндекс.Денег

[dovg]

Не ради холивара:
Зачем нужен интерфейс, если у него ровно одна авторизация.

[solvadore]

Упс. Промахнулся мимо кнопки. Мой ответ комментарием ниже.

[solvadore]

Ровно одна «реализация» может быть?
Это было сделано в большей степи для того, чтобы описание фукнциональности было в одном месте, чтобы легче было понимать его возможности.
Опять же, если кого-то эта реализация не устраивает, они могут написать свою или изменить существующую. Ну и мне кажется, это хороший тон отделять описание поведения от реализации. Здесь это может и не нужно, но в будущем такое восприятие может уберечь кого-нибудь от потенциальных граблей.

[dovg]

ИМХО у яндекс-денег в рамках одного фреймворка должна быть одна реализация. Если кому-то потребуется другая реализация, то он возьмет другой интерфеейс.

Другое дело, если бы у вас был интерфейс для работы с платёжными системами и его реализации для ЯД, вебмани и прочего.

[aleks_raiden]

а зачем вам было что-то править в денвере? он хорош как раз тем что работает из коробки

[solvadore]

Мне хотелось достучаться до него по локалке. А там из коробки это как-то проблематично. А вообще у всех WAMP'ов (LAMP'ов) все работает из коробки. Тут дело вкуса.

[Slash]

там это делается одним движением ноги, и даже описано в факе: www.denwer.ru/faq/shared.html

директорию переименовал, перезапустил и погнал…

[solvadore]

Да, но перед тем как лезть в фак, я попробовал это сделать руками — не получилось. Сборки без автоматизации (читай: наворотов) кажутся мне более понятными и прозрачными, но это субъективно.

[youROCK]

Автор, простите, но ваша библиотека (и статья) — это ужас.

Статья

В статье вы говорите, что ни разу до этого не работали с PHP — и это заметно. Хорошо хоть, вы этого не скрываете. Я не понимаю, как это — вы не смогли разобраться с тем, как в Денвере прописать возможность доступа извне, и, видимо, вообще не читали описание Денвера, и как с ним работать :). Как работнику Яндекса, вам должно быть стыдно. Потому что в Денвере можно реально поменять всё, достаточно просто прочитать описание, как это устроено и как работает (и почему именно так). Свои вхосты в denwer тоже добавить очень легко — достаточно в httpd.conf их прописать

О проекте на github

В описании сказано следующее:

Для успешного запуска примеров из комплекта следует проделать следующее:

* зарегистрировать приложение, т.е. получить идентификатор клиента (https://sp-money.yandex.ru/myservices/new.xml) и прописать его в константы примеров (consts.php);
* установить какой-нибудь WAMP (Windows-Apache-MySQL-PHP) или LAMP сервер (http://en.wikipedia.org/wiki/Comparison_of_WAMPs);
* скопировать файлы примеров и библиотеку в каталог www.

Не сказано ни-че-го про Mac OS X, а также описываются действия, которые человеку, который хотя бы один раз работал с PHP, объяснять не надо :). То есть, по сути, шаги 2 и 3 — неправильны. Второй шаг неверен потому, что, во-первых, у человека уже может стоять PHP, а во-вторых потому, что можно, скажем, на удаленный хостинг скопировать файлы и не устанавливать себе ничего. Третий шаг неправилен потому, что «каталог www» может называться как угодно, и, как правило, действительно называется не www.

Следующий код просто ужасен:

print_r('Номер счета: ' . $accountInfoResponse->getAccount() . '<br>');
print_r('Баланс: ' . $accountInfoResponse->getBalance() . '<br>');
print_r('Код валюты: ' . $accountInfoResponse->getCurrency() . '<br>');  

Чем вас не устроил echo :)? Про <br> я уж промолчу.

Реализация класса
Про интерфейс вам уже сказали, это не самое страшное :).
Для начала, отметим, что вы не используете неймспейсы (в принципе, это логично, если вам нужна совместимость с версиями PHP ниже 5.3)

1)

<?php
/*
* Модуль для простой и удобной работы с API Яндекс.Деньги.
* Использует библиотеки: cUrl, Mcrypt. 
*/

include('AES.php'); // модуль шифрования

Начнем с того, что в include_path может быть первой директорией отнюдь не ".", а что-нибудь другое, и файл может не заинклюдиться.
Потом, класс Crypt_AES может уже присутствовать в системе, и файл не нужно инклюдить, если этот класс уже есть (иначе будет Fatal error).
Ну, и include() в данном случае лишь сгенерирует warning, если файл не получилось подключить, а вашему классу этот файл _необходим_ для работы, поэтому здесь нужно писать require().

Итого, правильный код для включения класса AES будет следующим:

if(!class_exists('Crypt_AES')) {
	requre(dirname(__FILE__).'/AES.php');
}

2)

/**
 * Класс-перечисление прав приложения на использование эккаунта Яндекс.Денег
 * пользователя.
 */
class Scope {

...

/**
 * Класс для получения информации о конкретной операции из списка операций
 * метода operationHistory. Используется в объекте OperationHistoryResponse
 * @author dvmelnikov
 */
class Operation {

...

Вы никогда не задумывались над тем, что у кого-нибудь в проекте могут быть уже объявлены эти классы :)? Поскольку неймспейсы вы не используете, то это, опять же, будет Fatal error в PHP.

3)

/**
     * @return string возвращает содержимое объекта в качестве строки
     */
    public function __toString() {
        $parent = parent::__toString();
        return "OperationDetailResponse{error: $this->error, details: $this->details,
                sender: $this->sender, recipient: $this->recipient, message: 
                $this->message, codepro: $this->codepro}" . $parent;
    }

Я честно скажу, я сильно не уверен, что этот код действительно нужен, потому что print_r($this) выдал бы почти тот же самый результат. Вы, наверное, перепутали PHP с C#/Java

4)

class YandexMoney implements IYandexMoney {

...

public static function authorize($clientId, $scope = NULL, $redirectUri = NULL) {
        if (!isset($clientId) || $clientId == '') {
            throw new YandexMoneyException(YandexMoneyException::ERR_MESS_CLIENT_ID, 1001);
        }

        if (!isset($scope) || $scope == '') {
            $scope = Scope::ACCOUNT_INFO . Scope::OPERATION_HISTORY;
        }
        $scope = trim($scope);

        header('Location: ' . self::URI_YM_AUTH . "?client_id=$clientId" .
               "&response_type=code&scope=$scope&redirect_uri=$redirectUri");
    }

Библиотека общего назначения не должна отправлять header'ы ни в коем случае, ибо это является грубейшим нарушением MVC и это поведение будет несовместимо с распространнеными PHP-фреймворками, да и вообще любым более-менее сложным кодом на PHP.
Даже если вы очень хотите оставить header(), нужно поставить после него die(), ибо просто посылка header('Location: '); не прекращает исполнения скрипта в PHP после перенаправления. В любом случае, как максимум, библиотека должна возвращать URL, куда перекинуть пользователя, не больше.

В общем, возможно, вас в Яндексе уже отругали за ваш код, но я просто хотел бы вам указать на явные недостатки, которые, ИМХО, совершенно недостойны такой компании, как Яндекс

[ajaxtelamonid]

упс, случайно заминусовал вас.
совершенно согласен с вашим комментом.

[solvadore]

Еще раз спасибо за дельные замечания. Самые важные уже подправил (include и die после header).
Неймспейсы включать не буду для php < 5.3, но классы переименую. toString'и тоже наверно уберу.

[solvadore]

Спасибо вам за развернутый ответ. Рекомендации учту и поправлю код.

[cheef]

YandexMoney::authorize(Consts::CLIENT_ID, 'account-info operation-history', Consts::REDIRECT_URL);
По-моему ужасно нечитабельный интерфейс — трудно предположить как вызвать функцию не прочитав инструкцию, если она конечно имеется. Имхо гораздно удобнее использовать ключ-значение:
YandexMoney::authorize(array('client_id' => Consts::CLIENT_ID, ...));

[solvadore]

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