gnuboard / gnuboard5

그누보드5 (영카트 포함) 공개형 Git
Other
299 stars 244 forks source link

alert(), alert_close(), confirm() 함수에서 JSON 응답을 지원하도록 개선 #288

Closed kkigomi closed 2 months ago

kkigomi commented 10 months ago

이 함수들에서 JSON 포맷의 응답을 지원하도록 개선했습니다.

응답 데이터는 이 함수가 받은 파라미터를 담습니다.

각 함수의 Hook은 여전히 이 변경사항에 우선하여 먼저 실행됩니다.

smaker commented 10 months ago

좋은 커밋인 것 같습니다. JSON 요청인 경우, 따로 분기처리해줬는데 그렇게 하지 않아도 되겠네요. 다만, 에러가 발생하면 400 에러를 리턴하는데 수많은 AJAX 요청 코드에서 success에 대해서만 처리하고, error에 대해서는 전혀 처리(응답)해주지 않는 소스 코드가 많을거라 생각합니다. 이 부분에 대한 호환성도 고려해야할 것 같아요.

kkigomi commented 10 months ago

@smaker 응답 바디는 결국 데이터일 뿐이라서 status code로 판단하는게 정석이긴 하다고 봅니다. jQuery 등에서도 status code로 success, fail을 구분하니 그누보드 내에서도 문제는 없을 것 같고요. https://github.com/jquery/jquery/blob/ace646f6e83e653f666ba715c200739f1cbdba52/src/ajax.js#L742C3-L742C3

그누보드 내에서도 ajax 응답 결과가 포맷이 통일되어 있지도 않고 오류에 대한 어떠한 규격도 없이 전부 제각각이기 때문에 어디에 맞추기도 난해하기도 하고요.

_g5_cause로 정상적인 응답인지, alert 등에서 오류 등으로 응답을 해버린건지 구분이 필요한 경우를 대비해서 추가해두긴 했지만, 이 역시 좀 애매하긴 합니다. 고민을 좀 하긴했는데 없는 것 보다는 나을 것 같아서 추가해두긴 했습니다.

그누보드가 이런 규격화에 너무 게을렀기 때문에 어떤 규격으로 코드를 제시해야할지도 갈등이 많습니다. 사실 이 PR도 지난 몇달간 고드를 고쳐놨다가 보낼까 말까 많이 갈등을 했었네요. 흠...

어쨌든 Hook을 사용해서 해결이 가능한 부분이지만 플러그인 형식으로 배포하려다보니 각 개별 플러그인의 Hook에서 응답하는 데이터가 다르면 이게 골치 아파지겠더라고요.


243 이 PR도 사실 그누보드에서는 $_REQUEST['w'] 값으로 모드를 구분해서 Hook을 호출하면 되는데 저는 아직 익숙치 않아서 admin_content_created, admin_content_updated, admin_content_deleted 이렇게 세개로 나눠서 추가했거든요. run_event('admin_content', $w); // $w: '', 'u', 'd' 이런식으로 코드를 구현했었어야 기존 구현들과 어우러졌을 텐데 저 PR에서 추가한 hook들만 너무 확튀게 되어버렸네요. 이런건 지적해주셔야 하는데 그냥 병합해버리셔서 PR 보내기 더 불편한 마음을 가지게 됐어요. 🥲

smaker commented 10 months ago

치명적인 에러가 아니면, 에러가 생겨도 상태 코드는 200여서 success callback에서 응답 결과에 대한 처리가 가능한데, 호환성을 깨뜨리는 부분이 상태 코드가 400이 되는 경우입니다. 이 경우는 success callback을 호출하지 않고 error callback을 호출합니다.

어떤 개발자는 done callback에서 처리하고, 어떤 개발자는 success callback에서만 처리하다보니 중구난방이더군요. 따라서, PR 반영 후 에러 핸들링이 전혀 되지 않을 가능성이 있습니다.

이 부분은 호환성이 분명히 깨지는 부분이고, 조심스럽게 판단해야 하지 않나 생각합니다.

kkigomi commented 9 months ago

@smaker 중구난방이 문제라면 명확한 스펙을 따르는 것이 맞지 않을까요?

HTTP 상태는 오류나 결과를 나타내기 아주 명확한 방법이라고 봅니다.

이 PR은 기존에 처리되지 않던 json 요청 시 alert 등의 함수로 오류 처리를 요청에 맞게 응답하게 위한 것이고, 해당 상황일 때에 오류로 지정된 메시지만 400으로 응답합니다. html 요청/응답일 때는 바꾸지 않고요.

말씀하신 그런 관행이 있는 것은 사실입니다. 어찌보면 이 PR 자체가 호환성을 깨는 행위입니다. json 응답으로 요청하더라도 HTML로 반환했었기 때문에 바디에서 "올바른 방법으로 이용해 주십시오."라는 문자열이 있는지를 확인하여 오류 처리를 하던 사람들도 있을테니 그런 사람들에겐 재앙이겠죠?

hook을 이용할 수 있으니 이 PR이 적용되지 않아도 상관은 없으나, PR 본문에 적은 것처럼 hook을 처리하는 순서에 따라 예상하지 못한 결과가 반영될 수 있는 말 그대로 중구난방의 문제로 통일시키기 위한 목적이므로 사실 모든 관행을 포용하기는 불가능합니다.

여러 관행이 있다는 것을 알지만 그것을 모두 포용해야한다면 위에서 말한 것처럼 이 PR 자체가 받아들여져서는 안 되겠죠.

smaker commented 9 months ago

REST API 처럼 동작하도록 코드가 개선이 되는 건 찬성입니다만, 호환성을 깨는데 그만큼의 큰 이점이 있는지도 의문입니다.

kkigomi commented 9 months ago

@smaker PR은 당장 반영해달라는 요청으로 생각하지 않습니다. 적절한 시점에 반영되어도 좋고, 문제가 된다면 반려될 수 있는 것이 당연하다고 생각하므로 반영여부의 결정이나 적용 시점은 운영 주체가 결정하시겠죠.