Skip to content

Conversation

AlexeyDsov
Copy link
Member

На работе поковыряли немножко CurlHttpClient и захотелось еще дополнительно дома его доковырять, что бы он умел делать все что нужно:

  1. отправлять массивы любой вложенности:
<?php
$request = HttpRequest::create()->setPost(array('a' => array('b' => array('c' => array(1, 2, 3))));
CurlHttpClient::create()->send($request);

В текущей версии вложенность для get и post ограничена двумерным массивом, однако более более глубокие массивы, возможно, кто-то получал заранее создавая массив вроде array('a[b][c][0]' => '123'). Теперь такие будет делать нельзя, т.к. теперь ключи массива прогоняются через urlencode так же как и значения.

  1. В случае HttpMethod::post предлагаю дополнять url параметрами из HttpRequest::getGet, так же как в случае HttpMethod::get

  2. Сделана загрузку файлов, их нужно задавать так:

<?php
HttpRequest::create()->setFiles(array(
    'file1' => '/tmp/file1.txt',
    'pngFile' => '/tmp/someimage.pmg'
));
  1. Добавлена возможность отправлять кастомное post тело (например, в некоторых случаях нужно отправлять post'ом xml файл):
<?php
HttpRequest::create()->setBody(
'<?xml version="1.0"?>
<!DOCTYPE metaconfiguration SYSTEM "meta.dtd">
<metaconfiguration><classes></classes></metaconfiguration>');
  1. Т.к. изменена логика формирования post и get (добавлен urlencode для названия параметров), то реализовано возможность использовать старый алгоритм. Если нужно во всем проекте использовать старый алогоритм, то в конфиге объявить временную константу ONPHP_CURL_CLIENT_OLD_TO_STRING в true:
<?php
define('ONPHP_CURL_CLIENT_OLD_TO_STRING', true);

В случае если нужно только в одном конкретном случае оставить старую логику формирования post/get параметров добавлена настройка в CurlHttpClient:

<?php
CurlHttpClient::create()->setOldUrlConstructor(true);

@dovg
Copy link
Member

dovg commented Jun 18, 2012

А тесты? )

@AlexeyDsov
Copy link
Member Author

Формирование url'а вынесено в отдельный класс и тесты на эту часть написаны.
Как тестить сам curl?

@dovg
Copy link
Member

dovg commented Jun 18, 2012

Сейчас для тестов https в Curl мы опрашиваем github. См HttpUtilsTest.

ИМХО как минимум нужно убедиться, что POST запрос с файлами и прочим не приводит к 400 коду ответа.

@AlexeyDsov
Copy link
Member Author

Не нравится мне идея что в unit тестах стучаться куда-то, особенно по многу раз... Как-то оно противоречит unit тестам.
Да и если файл/post отправлять, то нужно что-то осмысленное получать, что б понимать что загрузка верно прошла, и post параметры переданы так как надо.

@crazedr0m
Copy link
Contributor

А если унаследоваться? Или выделить интерфейс, а реализации сделать разные?
Мне почему-то не нравится "обрастание" константами для обратной совместимости.
Для пункта 2) - не уверен что это правильно.

@AlexeyDsov
Copy link
Member Author

Пообсуждав тестирование пришли к следующем варианту, которое сделаю надосуге:
В конфиге тестов указываем url где находится тестовый скрипт (это может быть localhost или у кого что)
Сам тестовый скрипт будет такой вот:

<?php
print_r(array($_GET, $_POST, $_FILE, file_get_content('php://input')), true);

В тесты будут выглядить так (кое-что сейчас в примере упускаю, типа HttpUrl, HttpMethod и т.д.):

<?php
$post = array('a' => array('b' => array('c' => 'd')));
$request = HttpRequest::create()->setPost($post);
$response = CurlHttpClient::create()->send($request);
$this->assertEquals(print_r($post, true), $response->getBody());

@crazedr0m
Copy link
Contributor

Выглядит не плохо

@AlexeyDsov
Copy link
Member Author

@crazedr0m наследование и вынесение не хочется делать, т.к. в случае CurlHttpClient все таки хочется протестировать именно правильную отправку curl запросов. Кирпичик которуй если замокать то теряется смысл его тестирования, зато при использовании в других классах надо его заменять на mock.

А чем плох пункт 2? По мне так если в реквесте get параметры заданы, то должны участвовать в формировании url'а будь то post запрос или get.

@AlexeyDsov
Copy link
Member Author

Добавил тесты, вскрылось два бага:

  1. Касается безопасности. Curl отправляет файл если засетить в CURLOPT_POSTFIELDS массив параметров и там где параметр начинается с символа собаки '@' - он считает что это путь к файлу который необходимо отправить. Таким образом нельзя отправлять вместе с файлами post данные, которые начинаются с этого же символа иначе curl будет думать что это файл, в общем ошибка безопасности. Может быть можно как-то заэкранировать собаку, но нигде не попалось подобного.
    Так или иначе сделал что б бросалась ошибка, дабы не получилось уюзвимости из-за такой "фичи"
  2. Даже если сделать urlencode ключей параметров, то закодированный символ квадратных скобок воспринимается так же как если б не был закодирован. В общем в ключах лучше их не использовать. Остальные необычные значения вроде бы ведут себя как надо.

@crazedr0m
Copy link
Contributor

Мне нравится отправка файлов и кастомного тела пост запроса и не нравится преобразование многомерных массивов, я считаю что это не входит в обязанности клиента.

@AlexeyDsov
Copy link
Member Author

Это уже входит в обязанности клиента, но ограничено лишь двумерными массивами. Он уже находится в состоянии "ни то ни сё".

@AlexeyDsov
Copy link
Member Author

А что смущает то в преобразовании массивов? Вот есть у нас HttpRequest который мы заполнаем из $_GET, $_POST и т.д. Сам php входные параметры преобразует во многомерные массивы $_GET и $_POST.
При отправки через CurlHttpClient мы используем HttpRequest у которого все так же (ну вроде как логично) могут быть многомерные массивы.
В итоге во всём коде мы используем нормальные многомерные массивы кроме как внутри CurlHttpClient, который конвертирует эти массивы все так же в массивы но только в url представлении - var1[sub][0]=55&var2[sup]=66).
Где еще быть в коде подобному преобразованию если не в клиенте?

@AlexeyDsov
Copy link
Member Author

Контр аргументов более не получил, на днях замержу

AlexeyDsov added a commit that referenced this pull request Sep 12, 2012
CurlHttpClient: allowing upload files and send post/get arrays with any deep lvl
@AlexeyDsov AlexeyDsov merged commit 081d8de into onPHP:master Sep 12, 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

Successfully merging this pull request may close these issues.

3 participants