Skip to content

Conversation

@ezoterik
Copy link
Contributor

Существует проблема. Если в процессе запроса через curl происходит ошибка соединения, например, timeout (проблема была до моего PR #18 :)), то метод NovaPoshtaApi2::request возвращает null в ответ. Как минимум, проблема в том, что ты не можешь понять, в чем же конкретно ошибка в этом случае.

В коммите я решил эту проблему бросанием исключения с текстом и кодом ошибки, но была одна дилемма с флагом NovaPoshtaApi2::$throwErrors. Если брать во внимание этот флаг, то есть какая-то логика в том, что если он равен true, то нужно выбрасывать исключение, а если false, то необходимо возвращать искусственно созданный ответ (будто он от НП) в виде:

array(
    'success' => false,
    'data' => array(),
    'errors' => array(curl_error($ch) . ' (' . curl_errno($ch) . ')'),
    'warnings' => array(),
    'info' => array(),
);

Сначала я так и сделал, но потом подумалось, что проблема с соединением - это все же исключительная ситуация, и нужно просто выбрасывать исключение, а флаг NovaPoshtaApi2::$throwErrors скорее про преобразование в исключения штатных ошибок от НП, получаемых в условии корректного ответа сервера.

С моим итоговым решением есть одна потенциальная проблемка с обратной совместимостью. Поведение многих функций частично меняется. Если раньше кто-то ориентировался на то, что в случае ошибки будет возвращен null, то теперь такого не должно будет происходить (по крайней мере с curl). Будет возвращен массив со структурированным и корректным ответом от НП или же выброшено исключение. Как вариант, можно было бы решить это через SemVer (за столько лет уже можно стать версией 1.0.0 :)), если в целом есть согласие, что вариант с исключением логически верен, но при этом хочется наиболее осторожно относиться к обратной совместимости.

@lis-dev
Copy link
Owner

lis-dev commented Mar 23, 2021

Вариант с исключением действительно логически верен и следовало бы так и реализовать. Однако менять мажорную версию я планирую только после значительного рефакторинга кода. PR пока что оставлю открытым, после рефакторинга - приму. Спасибо за помощь!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants