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

SessionHelper 클래스에서 dynamic 프로퍼티 문제 수정 #2279

Closed
wants to merge 1 commit into from

Conversation

kkigomi
Copy link
Contributor

@kkigomi kkigomi commented Jan 20, 2024

#[\AllowDynamicProperties] attribute 를 제거하여 미래의 PHP 변경에 대비합니다.

대신 __get(), __set() 매직 메소드를 이용해 프로퍼티 문제를 해결합니다.

주석에 이 클래스에서 사용할 수 있는 프로퍼티 목록과 타입을 정의합니다.

프로필 이미지를 확인하고 가져오는 메소드를 추가했습니다.

`#[\AllowDynamicProperties]`를 제거하고 미래의 PHP 변경사항에 대비합니다.
대신 `__get()`, `__set()` 매직 메소드를 이용해 프로퍼티 문제를 해결합니다.
주석에 이 클래스에서 사용할 수 있는 프로퍼티 목록과 타입을 정의합니다.
@kkigomi kkigomi marked this pull request as ready for review January 20, 2024 14:16
@kijin
Copy link
Member

kijin commented Jan 20, 2024

미래의 PHP 변경에 대비하기 위해 #[\AllowDynamicProperties] attribute를 붙여놓은 것입니다.

SessionHelper는 XE 초창기부터 사용해 온 Context::get('logged_info')에서 stdClass를 대체하는 역할을 하는 클래스로서, 모든 서드파티 자료와 호환성을 유지해야 합니다. 생성자에 int가 아닌 null, false, 배열 등 괴랄한 것이 넘어올 수도 있고, 어떤 속성에 접근하거나 속성을 조작할 때도 기존과 동일한 동작을 보장해야 한다는 뜻입니다.

안타깝게도 PHP의 magic method는 dynamic property를 완전히 대체할 수 없습니다.

예를 들어

$obj->foo[] = 'bar';

이런 코드는 객체의 속성에 직접 접근할 때와 __get() __set()을 통해 접근할 때 동작이 전혀 다릅니다. 왜 이 따위로 만들다 말았는지 모르겠지만 아무튼 언어 자체가 그렇습니다.

dynamic property를 가장 많이 사용하는 Context 클래스에 magic method를 적용하지 않고 있는 이유이기도 합니다.

@kijin
Copy link
Member

kijin commented Jan 20, 2024

참고로 Rhymix Framework에서 타입을 엄격하게 제한할 수 있는 곳은 2.1.x 초반에 거의 모두 변경해 놓았습니다. 타입 선언이 누락되었거나 지나치게 느슨해 보이는 곳이 있다면, 십중팔구 일부러 그렇게 해둔 것이니 static analyzer를 지나치게 신뢰하지 마시기 바랍니다.

@kkigomi
Copy link
Contributor Author

kkigomi commented Jan 20, 2024

아, 제가 AllowDynamicProperties 미봉책으로 오해했네요. ㅎㅎ

@kkigomi kkigomi closed this Jan 20, 2024
@kijin
Copy link
Member

kijin commented Jan 20, 2024

네, 미봉책은 아니구요... PHP 8.2부터 새로 생긴 따끈따끈한 땜빵책입니다. ㅎㅎ

프로필 사진 주소를 한 번에 불러오는 메소드는 괜찮은 것 같네요. 앞으로는 회원 정보도 DocumentItem, CommentItem처럼 별도의 클래스에 담으려고 하는데, 이 때 SessionHelper 기반으로 확장할 계획이니 끼곰이님 아이디어를 반영하도록 하겠습니다.

@kkigomi
Copy link
Contributor Author

kkigomi commented Jan 20, 2024

SessionHelper 가 정리된 클래스인 줄 알았는데 그렇지는 않은가 보네요.

자동 완성이 되질 않으니 디버그를 계속 찍어봐야해서 프로포티 목록을 나열했는데, 메소드를 추가해나가는게 낫겠네요.

@kijin
Copy link
Member

kijin commented Jan 21, 2024

user_name, nick_name, email_address 등 회원정보에 당연히 있을 만한 것들은 아예 정식으로 property를 추가하는 편이 낫겠습니다.

최소 지원 버전이 PHP 7.4로 변경되면 아래와 같은 문법을 사용할 수 있게 되니까요.

class SessionHelper
{
    public int $member_srl;
    public string $user_name;
    ...
}

주석은 오랜 기간 유지보수하다 보면 실제 코드와 달라져서 오히려 혼란을 일으키는 경우가 종종 생깁니다. 그래서 IDE나 특정한 툴만을 위한 주석을 추가하기보다, 가능하면 코드 자체에 표현하는 것을 선호합니다.

@kkigomi
Copy link
Contributor Author

kkigomi commented Jan 21, 2024

속성이 추가되면 자동완성이 되어 편리해지겠네요.

타입은 저도 주석보다는 php가 지원하는 방법으로 표현하는게 좋습니다. 지금은 7.2라서 불가능한 부분은 주석으로 표현할 수 밖에 없죠.

언어에서 지원하는 것을 우선하고, 주석으로 개발 환경을 점진적으로 보조하거나 개선할 수단으로는 충분히 보편적인 수단이라고 생각합니다.

아마 제가 PHPStan 글을 쓴게 영향을 준 것 같은데, 특정 툴만 지원하는 포맷이 아니라 생각보다 보편적으로 통용되는 수준이니까요.

개발 환경의 점진적 향상을 위한 수단으로 생각해주시면 좋을 것 같습니다. 분명 도움이 되는 작은 장치이니까요.

@kijin
Copy link
Member

kijin commented Jan 22, 2024

PHPStan에 대해서는 알고 있습니다. "특정 툴" 수준이 아니라 사실상 표준에 가까운 툴이라는 것도요.

다만, 이 PR에 당초 추가하셨던 것처럼 IDE나 static analyzer가 타입을 유추할 수 있도록 돕기 위해 수십 줄에 달하는 주석을 추가하여 사람이 보는 소스를 오히려 지저분하게 만드는 것은 다소 과하다는 의견입니다. 그런 주석이 필요하다는 것 자체가 PHP 7.2 지원으로 인한 부작용이거나, 배열처럼 분석하기 어려운 자료 구조를 사용했거나, 그 밖에 다른 문제가 있다는 뜻인데, 근본적인 해결 없이 땜빵하는 것이니까요.

비슷한 예로, 예전에 $oMemberModel = getModel('member') 이런 식으로 사용할 때는 getModel() 함수의 반환 타입을 정확하게 선언할 수 없었기 때문에 대부분의 개발툴이 $oMemberModel의 메소드명이나 파라미터를 자동완성해 주지 못했습니다. 그것 때문에 PHPStorm에서 불필요한 경고가 많이 뜨니까 주석을 붙여서 때우는 분도 계셨고요. 미봉책이었죠. MemberModel::getInstance()로 바뀐 지금은 그렇게 할 필요가 없어졌습니다. 다소 시간이 걸렸지만 더 깔끔한 해결책을 찾은 것입니다. (물론 getModel()도 여전히 지원합니다. 아마 머지않아 @deprecated 주석이 붙겠지요.)

덕분에 기존 설계가 미흡했던 부분, PHP 버전이나 하위호환성의 제약으로 제대로 날개를 펴지 못하고 있는 부분, 버전 변경시 제일 먼저 개선할 수 있을 만한 부분을 더 자세히 생각해 볼 기회를 얻은 것 같습니다. 현재 구조에서 무리하게 끼워맞추기보다는, 이렇게 장기적인 개선 방향을 함께 만들어나가는 데 더 많은 도움을 주시면 좋겠습니다.^^

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.

None yet

2 participants