php / php-src

The PHP Interpreter
https://www.php.net
Other
38.33k stars 7.76k forks source link

Creation of dynamic property is deprecated ! #12694

Open Tak-Pesar opened 1 year ago

Tak-Pesar commented 1 year ago

Description

The following code ( This should never happen when I'm using magic methods, but unfortunately it does happen sometimes ):

<?php
final class Content {
    protected array $data = array('sequence'=>1);

    public function &__get(string $property) : mixed {
        if(isset($this->data[$property]) === false):
            $this->data[$property] = null;
        endif;
        return $this->data[$property];
    }
    public function __set(string $name,mixed $value) : void {
        $this->data[$name] = $value;
    }
}

$content = new Content();
$result = $content->sequence * 2 + 1;
$content->sequence += 1;
print $result;

Result:

3

But sometimes when the number of such requests increases, I receive the following warning :

Creation of dynamic property Content::$sequence is deprecated 18

PHP Version

PHP 8.2.11

Operating System

Linux

iluuu1994 commented 1 year ago

@Tak-Pesar Is it possible that Content in your code contains some code that calls something out side of the class, that again assigns to sequence? E.g. https://3v4l.org/smIKL

Tak-Pesar commented 1 year ago

No, this is exactly my code , Without any changes, it works properly, but I don't know under what conditions I suddenly get this warning, the main reason is not clear at all. It is not possible to receive such a change because I am not trying to create a non-existent property from within the class itself or other things like this

iluuu1994 commented 1 year ago

You can try using set_error_handler and debug_backtrace to see where the error occurs. __get and __set have guards, meaning if you call get/set the same property again it will trigger a real property-read/write, even outside of the current class.

Tak-Pesar commented 1 year ago

In fact, I do this : https://3v4l.org/L7GIR

I create an object and save it and call it later in my other class, I don't put it in a function And so I do the same thing multiple times but I only get one warning

iluuu1994 commented 1 year ago

@Tak-Pesar Can you try using set_error_handler and debug_backtrace and report what the stack trace says? It's hard to understand this issue without a reproducer, or more information.

Tak-Pesar commented 1 year ago

I used set_error_handler to get this result also use debug_backtrace for save some logs

iluuu1994 commented 1 year ago

@Tak-Pesar What I need is something like this: https://3v4l.org/YtnAhT

#0 /in/YtnAhT(4): {closure}(8192, 'Creation of dyn...', '/in/YtnAhT', 4)
#1 /in/YtnAhT(16): assign_foo(Object(Content))
#2 /in/YtnAhT(28): Content->__set('foo', 1)
#3 {main}

Where it's visible whether the error occurs somewhere inside a Content->__set call. Can you provide that? If not, then I'm afraid I cannot help, because your report is too vague.

Tak-Pesar commented 1 year ago
Creation of dynamic property Tak\Liveproto\Utils\Content::$last_msg_id is deprecated -> 173
#0 src/Utils/Session.php(173): {closure}()
#1 src/Network/Sender.php(64): Tak\Liveproto\Utils\Session->getNewMsgId()
#2 revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(422): Tak\Liveproto\Network\Sender->sendPacket()
#3 revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(558): Revolt\EventLoop\Internal\AbstractDriver->invokeMicrotasks()
#4 [internal function]: Revolt\EventLoop\Internal\AbstractDriver->Revolt\EventLoop\Internal\{closure}()
#5 revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(495): Fiber->start()
#6 revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(549): Revolt\EventLoop\Internal\AbstractDriver->invokeCallbacks()
#7 [internal function]: Revolt\EventLoop\Internal\AbstractDriver->Revolt\EventLoop\Internal\{closure}()
#8 revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(93): Fiber->resume()
#9 revolt/event-loop/src/EventLoop/Internal/DriverSuspension.php(99): Revolt\EventLoop\Internal\AbstractDriver->Revolt\EventLoop\Internal\{closure}()
#10 amphp/amp/src/Future.php(251): Revolt\EventLoop\Internal\DriverSuspension->suspend()
#11 src/Network/Sender.php(90): Amp\Future->await()
#12 src/Tl/Caller.php(138): Tak\Liveproto\Network\Sender->receive()
#13 src/Tl/Methods/Dialog.php(10): Tak\Liveproto\Tl\Properties->__call()
#14 src/Tl/Caller.php(33): Tak\Liveproto\Tl\Properties->get_difference()
#15 run/run.php(87) : eval()'d code(28): Tak\Liveproto\Tl\Caller->__call()
#16 run/run.php(87): eval()
Creation of dynamic property Tak\Liveproto\Utils\Content::$sequence is deprecated -> 178
#17 {main}#0 src/Utils/Session.php(178): {closure}()
#1 src/Network/Sender.php(67): Tak\Liveproto\Utils\Session->generateSequence()
#2 revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(422): Tak\Liveproto\Network\Sender->sendPacket()
#3 revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(558): Revolt\EventLoop\Internal\AbstractDriver->invokeMicrotasks()
#4 [internal function]: Revolt\EventLoop\Internal\AbstractDriver->Revolt\EventLoop\Internal\{closure}()
#5 revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(495): Fiber->start()
#6 revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(549): Revolt\EventLoop\Internal\AbstractDriver->invokeCallbacks()
#7 [internal function]: Revolt\EventLoop\Internal\AbstractDriver->Revolt\EventLoop\Internal\{closure}()
#8 revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(93): Fiber->resume()
#9 revolt/event-loop/src/EventLoop/Internal/DriverSuspension.php(99): Revolt\EventLoop\Internal\AbstractDriver->Revolt\EventLoop\Internal\{closure}()
#10 amphp/amp/src/Future.php(251): Revolt\EventLoop\Internal\DriverSuspension->suspend()
#11 src/Network/Sender.php(90): Amp\Future->await()
#12 src/Tl/Caller.php(138): Tak\Liveproto\Network\Sender->receive()
#13 src/Tl/Methods/Dialog.php(10): Tak\Liveproto\Tl\Properties->__call()
#14 src/Tl/Caller.php(33): Tak\Liveproto\Tl\Properties->get_difference()
#15 run/run.php(87) : eval()'d code(28): Tak\Liveproto\Tl\Caller->__call()
#16 run/run.php(87): eval()
#17 {main}

That's all I got

#1 src/Network/Sender.php(64): Tak\Liveproto\Utils\Session->getNewMsgId()

#1 src/Network/Sender.php(67): Tak\Liveproto\Utils\Session->generateSequence()

Tak-Pesar commented 1 year ago

What other modes are there for this error to be checked ?

final class Session {

    private object $content;

    public function __construct(){
        $this->content = new Content;
    }

/* some functions */
    public function generateSequence() : int {
        $result = $this->content->sequence * 2 + 1;
        $this->content->sequence += 1; // line 178
        return $result;
    }
}

The magic of the get method works correctly, the problem is in setting the variable, it is very strange

iluuu1994 commented 1 year ago

Fibers can complicate things. If you suspend a fiber within __set, the property on the given object will remain locked. If something else writes to the objects property before the fiber is resumed, the property will still be locked. For example: https://3v4l.org/KH2oE As you can see, the warning occurs on line 17 due to $foo->bar = 'baz';, with no indication that the property would be locked. Do you reckon something like this might be happening?

Tak-Pesar commented 1 year ago

Yes, it may be, because I use fibers, can you solve this problem ? Although how can I fix it ?

Tak-Pesar commented 1 year ago

But not exactly like this, I just pass an object of the object into a fiber and try to make changes from them, and my suspension is not in the __set.

iluuu1994 commented 1 year ago

@Tak-Pesar We need a reproducer to solve this problem, it's very hard to solve a problem through guesswork (when it's not even clear if here is a problem). Suspending from __get/__set is a bit weird, but this would be expected behavior. I'm not sure whether that's exactly what's going on with you from your description, but again it's essentially impossible to tell without a consistent reproducer.

bwoebi commented 1 year ago

Side-note: @iluuu1994 While the behaviour shown in your 3v4l link from last week is expected from the current implementation (the guard is globally attached as long as getter and setters are evaluated), I would consider it a bug (or shortcoming of the current implementation), that getters and setters persist across fiber boundaries (at least of fibers which are not created within the current context).

It also has side-effects with longjumps, like: https://3v4l.org/Q7nSo#v8.2.0

Tak-Pesar commented 1 year ago

@iluuu1994

I think the problem is that I use fibers, of course I use the Revolt/EventLoop library , so I don't use fibers directly myself , but these two interface libraries , I'm not trying to suspend in the set method, but some of my functions that use that object will no doubt run through fibers. All I do is create a loop with fibers and in the running function I try to use the object I have and store a new value. Then I get this error, I use this :

$object = new Session;
Revolt\EventLoop::delay(5,function(string $id) use($object) : void {
print $object->generateSequence();
});

I run this several times almost in a loop but only get this error once or twice.

iluuu1994 commented 11 months ago

I don't know how to more forward here. IMO this is expected behavior at this point in time. We may decide that guards should be restricted to a particular environment (e.g. the current fiber) and/or released on certain events (bailout). That's not currently how they are implemented, and I think this isn't a trivial change. This change also comes with a BC break, although it's unlikely many people rely on it.

It's also not completely clear whether that's OPs actual problem.

@bwoebi How would you like to proceed?

Tak-Pesar commented 11 months ago

Well, the problem is exactly here. I am currently using ArrayAccess instead of magic functions (set & get) and I'm waiting for this problem to be solved. When I wrote the same previous codes use object as an array, a value will be received from the object.I had no more errors , but it's a bit annoying that I can't get the values from it as a property call

final class Content implements \ArrayAccess {
    public function __construct(public array $data){
    }
    public function &__get(string $property) : mixed {
        if(isset($this->data[$property]) === false):
            $this->data[$property] = null;
        endif;
        return $this->data[$property];
    }
    public function __set(string $name,mixed $value) : void {
        $this->data[$name] = $value;
    }
    public function __unset(string $name) : void {
        unset($this->data[$name]);
    }
    public function __isset(string $name) : bool {
        return isset($this->data[$name]);
    }
    public function &offsetGet(mixed $offset) : mixed {
        if(isset($this->data[$offset]) === false):
            $this->data[$offset] = null;
        endif;
        return $this->data[$offset];
    }
    public function offsetSet(mixed $offset,mixed $value) : void {
        $this->data[$offset] = $value;
    }
    public function offsetUnset(mixed $offset) : void {
        unset($this->data[$offset]);
    }
    public function offsetExists(mixed $offset) : bool {
        return isset($this->data[$offset]);
    }
}
Tak-Pesar commented 10 months ago

@iluuu1994 Is this problem still not solved? I am really facing a lot of problems

iluuu1994 commented 10 months ago

You haven't shared a simple reproducible example that confirms we're actually talking about the same problem. If we are, you should try to avoid suspending fiber within magic methods. I don't consider this a bug but a feature request. Magic accessors are not recursive by design (even across fibers). There's not a simple fix that we can apply to current versions.