Fixed Bug #61025 __invoke() visibility not honored #298

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
@reeze
Contributor

reeze commented Mar 6, 2013

See bug https://bugs.php.net/bug.php?id=61025

This patch make __invoke() visibility consist with other magic methods.

reeze added some commits Mar 6, 2013

Fixed bug #61025 (__invoke() visibility not honored)
- check visibility of __invoke when calling
- make is_callable consist
- Add the ZEND_ACC_CLOSURE flag to the closure created with: zend_create_closure()
__invoke() magic method should not be declared as static
Related to bug #61025
Consist with similar magic methods which will raise warning
when modified with improper modifiers
+ if (UNEXPECTED(!zend_check_protected(zend_get_function_root_class(fcc->function_handler), EG(scope)))) {
+ return 0;
+ }
+ }

This comment has been minimized.

@lstrojny

lstrojny Mar 6, 2013

Contributor

Don’t we miss a return 0 here?

@lstrojny

lstrojny Mar 6, 2013

Contributor

Don’t we miss a return 0 here?

This comment has been minimized.

@reeze

reeze Mar 6, 2013

Contributor

Hmm, I only see four conditions, they are closure function and other three possible ZEND_ACC_PPP* flags.
or else it's common function, but that wouldn't happen when get the function with get_closure

@reeze

reeze Mar 6, 2013

Contributor

Hmm, I only see four conditions, they are closure function and other three possible ZEND_ACC_PPP* flags.
or else it's common function, but that wouldn't happen when get the function with get_closure

@laruence

This comment has been minimized.

Show comment
Hide comment
@laruence

laruence Mar 7, 2013

Member

I don't think this need to be fixed in this way, like __call:

<?php

class Bar {
    private function __call($name, $value) {
        return __CLASS__;
    }
}

$b = new Bar;
$b->__call("name", NULL);

works well, but with an warning:

Warning: The magic method __call() must have public visibility and cannot be static 

I am not sure whether this is a bug, or just need document.

Member

laruence commented Mar 7, 2013

I don't think this need to be fixed in this way, like __call:

<?php

class Bar {
    private function __call($name, $value) {
        return __CLASS__;
    }
}

$b = new Bar;
$b->__call("name", NULL);

works well, but with an warning:

Warning: The magic method __call() must have public visibility and cannot be static 

I am not sure whether this is a bug, or just need document.

@jpauli

This comment has been minimized.

Show comment
Hide comment
@jpauli

jpauli Mar 7, 2013

Member

This patch is inconsistent with what actually exist.
Actually, __magics() signature are checked at compile time, not at run-time. This patch adds runtime checks for __invoke() , which IMO is wrong.

I think it's better to stay consistent and add the checks in the zend_compile.c cases.
Visibility is never checked at runtime for __magics() (but used to be).

Member

jpauli commented Mar 7, 2013

This patch is inconsistent with what actually exist.
Actually, __magics() signature are checked at compile time, not at run-time. This patch adds runtime checks for __invoke() , which IMO is wrong.

I think it's better to stay consistent and add the checks in the zend_compile.c cases.
Visibility is never checked at runtime for __magics() (but used to be).

@php-pulls

This comment has been minimized.

Show comment
Hide comment
@php-pulls

php-pulls Mar 7, 2013

Comment on behalf of laruence at php.net:

wrong fix, closed

Comment on behalf of laruence at php.net:

wrong fix, closed

@php-pulls php-pulls closed this Mar 7, 2013

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