Skip to content
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

Обработка ошибок от curl #55

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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