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

PHP 8 이상에서 Hook에서 발생할 수 있는 Fatal error #251

Open
kkigomi opened this issue Jul 8, 2023 · 2 comments
Open

PHP 8 이상에서 Hook에서 발생할 수 있는 Fatal error #251

kkigomi opened this issue Jul 8, 2023 · 2 comments

Comments

@kkigomi
Copy link
Contributor

kkigomi commented Jul 8, 2023

Hook에는 글로벌 함수와 getInstance() 메소드를 포함하는 싱글턴 클래스 그리고 인스턴스를 자동으로 생성하는 방법으로 크게 세 가지를 지원합니다.
#244 이슈가 해결되면 클로저도 지원하게 되겠죠.

PHP 8 미만에서는 getInstance()가 없는 클래스는 강제로 인스턴스를 생성하여 실행했지만, PHP 8에서는 Fatal error가 발생합니다.

아래 클래스는 PHP 8 미만에서는 someMethodC 메소드를 호출한 방법을 제외하고는 someMethodA, someMethodB는 warning 메시지를 출력하지만 동작은 합니다. 비록 인스턴스가 몇개가 생성되더라도요.

class SomeClass
{
    public function __construct()
    {
    }

    public function someMethodA()
    {
        error_log('someMethodA');
    }
    
    public static function someMethodB()
    {
        error_log('someMethodB');
    }
    
    public function someMethodC()
    {
        error_log('someMethodC');
    }
}
add_event('CUSTOM_EVENT', array('SomeClass', 'someMethodA')); // Fatal error
add_event('CUSTOM_EVENT', array('SomeClass', 'someMethodB')); // Fatal error
// add_event('CUSTOM_EVENT', array(new SomeClass(), 'someMethodC')); // Fatal error. 이건 지금도 안 됨

run_event('CUSTOM_EVENT');

하지만 PHP 8 이상에서는 getInstance() 메소드를 호출하는 call_user_func(array($class, $this->singleton)) 코드로 인해 Fatal error가 발생합니다.

PHP Fatal error: Uncaught TypeError: call_user_func(): Argument #1 ($function) must be a valid callback, class ListenerClass does not have a method "getInstance" in ...

즉, Hook을 PHP 8 이상에서도 문제 없이 사용하려면 클래스에는 getInstance() 메소드를 포함하는 싱글턴 클래스를 사용해야 합니다.
하지만 , PHP 8 미만에서는 getInstance() 메소드가 없다면 항상 새로운 인스턴스를 생성하여 동작하도록 되어 있는데 이것이 PHP 8 이상에서는 Fatal error로 인해 사용할 수 없게 됩니다.

if (! ($class && $method) && function_exists($function)) {
return call_user_func_array($function, $args);
} elseif ($obj = call_user_func(array($class, $this->singleton))) {
if ($obj !== false) {
return call_user_func_array(array($obj, $method), $args);
}
} elseif (class_exists($class)) {
$instance = new $class;
return call_user_func_array(array($instance, $method), $args);
}

/lib/Hook/hook.extend.php 파일에서 PHP warning 메시지를 무시하고 동작하는 상태이므로 호환성을 유지한채로 Fatal 오류를 방지해야 합니다.
무작정 getInstance() 메소드를 호출하기 전에 확인이 필요합니다.

또한, 아래와 같은 개선도 적용되면 좋겠네요.

  • Hook에 Closure를 리스너로 등록할 수 있도록 개선 #244 클로저 사용 문제 해결
    • add_event('CUSTOM_EVENT', function() {})
  • Static 메소드를 지정 시 현재는 항상 새로운 인스턴스를 생성하여 호출하지만, Static 메소드는 인스턴스 강제 생성을 하지 않도록 개선
    • add_event('CUSTOM_EVENT', array('SomeClass', 'someStaticMethod'))
    • add_event('CUSTOM_EVENT', 'SomeClass::someStaticMethod')
  • 이미 생성된 객체를 사용할 수 있도록 개선
    • add_event('CUSTOM_EVENT', [$this, 'someMethod'])

아래와 같이 고쳐봤는데 호환성 확인은 더 필요합니다. 참고하여 위와 같은 개선이 가능하면 좋겠네요.
(위에 링크한 hook.extends.class.php 코드 부분을 대체합니다. error_log()는 디버깅 참고를 위해 추가한 코드입니다.)

if ($class && $method) {
    $refMethod = new ReflectionMethod($class, $method);
    error_log(
        '@runAction - Method: ' . print_r(
            array(
                $class,
                $method,
                is_callable($class, $method),
                $refMethod->isPublic(),
                $refMethod->isStatic()
            ),
            true
        )
    );

    if (
        $refMethod->isStatic()
        || (
            is_object($class)
            && is_callable(array($class, $method))
        )
    ) {
        // Static 메소드이거나 객체로 전달 받았을 때
        return call_user_func_array(array($class, $method), $args);

    } else if (is_string($class)) {
        if (is_callable(array($class, $this->singleton))) {
            // $this->singleton 메소드를 포함하고 있을 떄
            $obj = call_user_func(array($class, $this->singleton));
        } else if (class_exists($class)) {
            // $this->singleton 메소드가 없으면 인스턴스 강제 생성
            $obj = new $class;
        }

        // 생성한 객체가 반환되었을 때만 콜백 실행
        if (isset($obj) && is_object($obj) && is_callable(array($obj, $method))) {
            return call_user_func_array(array($obj, $method), $args);
        }
    }
} else if (is_callable($function)) {
    // 글로벌 함수, 클로저, 'className::methodName' 형태의 Static 메소드
    return call_user_func_array($function, $args);
}

이와 같이 변경 후 아래의 코드로 PHP 5.2.17, 7.0.x, 7.4.x, 8.0.0, 8.2.0 버전에서 테스트 했습니다.

class SomeClass
{
    protected static $instance = null;

    public function __construct()
    {
        error_log('__construct');
    }

    // public static function getInstance()
    // {
    //     if (self::$instance === null) {
    //         self::$instance = new self();
    //     }

    //     return self::$instance;
    // }

    public function someMethodA()
    {
        error_log('someMethod ---------- A');
    }
    
    public static function someMethodB()
    {
        error_log('someMethod ---------- B');
    }
    
    public function someMethodC()
    {
        error_log('someMethod ---------- C');
    }

    public static function someMethodD()
    {
        error_log('someMethod ---------- D');
    }
}

// 클로저 (PHP 5.3 이상 지원)
add_event('CUSTOM_EVENT', function () {
    error_log('function ---------- Closure');
});

// 글로벌 함수
add_event('CUSTOM_EVENT', 'listenerCustomEvent');
function listenerCustomEvent() {
    error_log('function ---------- listenerCustomEvent');
}

$obj = new SomeClass();
add_event('CUSTOM_EVENT', array('SomeClass', 'someMethodA')); // 인스턴스 강제 생성
add_event('CUSTOM_EVENT', array('SomeClass', 'someMethodB')); // static 메소드
add_event('CUSTOM_EVENT', array($obj, 'someMethodC')); // 객체
add_event('CUSTOM_EVENT', 'SomeClass::someMethodD'); // static 메소드

run_event('CUSTOM_EVENT');
@thisgun
Copy link
Contributor

thisgun commented Jul 17, 2023

안녕하세요. SIR 입니다.

코드를 제공해 주셔서 대단히 감사합니다. ^_^

해당 내용은 차후에 참고하여 적용하겠습니다.

@kkigomi
Copy link
Contributor Author

kkigomi commented Jul 22, 2023

@thisgun 네. 충분한 테스트가 필요할 것 같습니다.
객체를 강제 생성하는건 사실상 실용성은 없고 사이드 이펙트만 발생시키니 빼는 게 낫다고 생각하지만 일단은 호환성 유지를 위해 남겨두긴 했습니다.

(근데 일은 혼자 다 하시나요... 항상 감사합니다.)

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

No branches or pull requests

2 participants