doc-en icon indicating copy to clipboard operation
doc-en copied to clipboard

Static in combination with anonymous class does not behave as expected

Open marco-a opened this issue 3 years ago • 13 comments

Description

The following code:

<?php

function createTestClass() {
	$test = new class() {
		static public $initialised = false;

		static public function __init() {
			if (self::$initialised) {
				throw new Exception("Already initialised.");
			}

			self::$initialised = true;
		}
	};

	$test::__init();
}

createTestClass();
createTestClass();

Resulted in this output (see here):

PHP Fatal error:  Uncaught Exception: Already initialised. in /Users/marco/Desktop/script.php:9
Stack trace:
#0 /Users/marco/Desktop/script.php(16): class@anonymous::__init()
php/php-src#1 /Users/marco/Desktop/script.php(20): createTestClass()
php/php-src#2 {main}
  thrown in /Users/marco/Desktop/script.php on line 9

Fatal error: Uncaught Exception: Already initialised. in /Users/marco/Desktop/script.php:9
Stack trace:
#0 /Users/marco/Desktop/script.php(16): class@anonymous::__init()
php/php-src#1 /Users/marco/Desktop/script.php(20): createTestClass()
php/php-src#2 {main}
  thrown in /Users/marco/Desktop/script.php on line 9

But I expected this output instead:

I'm not sure whether this is by design or really a bug. The documentation makes no mention of the usage of static with anonymous classes.

However, I do not expect that behaviour since I'm literally creating a new class (new class()), so why should it share its static variables with other "instances" (not class instance here) of the same class?

PHP Version

8.1.11

Operating System

Irrelevant

marco-a avatar Dec 06 '22 23:12 marco-a

Removing the second createTestClass results in the code running without the exception being thrown.

marco-a avatar Dec 06 '22 23:12 marco-a

Incidentally doing the same thing with anonymous functions results in the expected behaviour:

<?php

function createTestClass() {
	$fn = function() {
		static $initialised = false;

		if ($initialised) {
			throw new Exception("Already initialised");
		}

		$initialised = true;
	};

	return $fn;
}

createTestClass();
createTestClass();

Here the two functions are completely isolated from one another. (i.e. code will not thrown exception).

marco-a avatar Dec 06 '22 23:12 marco-a

Anonymous classes are just classes with runtime name assigned. https://3v4l.org/6u8JB

There are some hints in RFC However it's not clearly documented.

Multiple anonymous classes created in the same position (say, a loop) can be compared with ==, but those created elsewhere will not match as they will have a different name. ~ https://wiki.php.net/rfc/anonymous_classes#internal_class_naming

There are some comments pointing static behavior e.g.: https://www.php.net/manual/en/language.oop5.anonymous.php#121055

KapitanOczywisty avatar Dec 06 '22 23:12 KapitanOczywisty

Anonymous classes are just classes with runtime name assigned. https://3v4l.org/6u8JB

That doesn't matter to me. It explains the behaviour, sure, but it's certainly not what I would expect from an "anonymous" class. That's why they're called "anonymous": one "instance" should have nothing to do with the next.

The PHP developers should consider renaming the feature then, because it's clearly not an "anonymous class" we're dealing with here.

marco-a avatar Dec 07 '22 00:12 marco-a

As long as the anonymous class is created once per definition, the behaviour is expected and consistent with standard classes.

Also static would be otherwise more or less useless ;-)

mvorisek avatar Dec 07 '22 00:12 mvorisek

but it's certainly not what I would expect from an "anonymous" class.

"anonymous" for me only means "I don't care about the name". Expectation aren't always right. Comparing to anonymous functions is not helpful - these are entirely different structures. And also creating a new class type is somewhat expensive, so doing that every time new instance is created would be hurting performance. https://3v4l.org/tHFSK

The PHP developers should consider renaming the feature then, because it's clearly not an "anonymous class" we're dealing with here.

This is only case of "undocumented behavior" - nothing more. I'd ask: why do you even trying to use static variables in anonymous class, when you are relying on single instance?

Edit: I wasn't even aware that you can do pretty dumb stuff with these classes :) https://3v4l.org/vn6jm

KapitanOczywisty avatar Dec 07 '22 00:12 KapitanOczywisty

From the docs:

All objects created by the same anonymous class declaration are instances of that very class.

This explains the behavior quite nicely, but maybe we should make that info more prominent?

cmb69 avatar Dec 08 '22 11:12 cmb69

This explains the behavior quite nicely, but maybe we should make that info more prominent?

I've totally blanked that, but there it is.. Gray "note box" would be nice, because red warning could be a bit too much. Also I'd mention static method and properties, to avoid further confusion.

KapitanOczywisty avatar Dec 08 '22 13:12 KapitanOczywisty

As long as the anonymous class is created once per definition, the behaviour is expected and consistent with standard classes.

It's not intuitive at all. If I do a new class() I expect it to create a new class every time I do that.

Also static would be otherwise more or less useless ;-)

I disagree.

I think the behaviour stems from the implementation of "anonymous" classes in PHP.

Might be wrong, but conceptually it doesn't make sense to (see comparison with anonymous functions).

Comparing to anonymous functions is not helpful - these are entirely different structures.

It is actually helpful, both are anonymous. But one acts different than the other. Let me explain:

If I do a

$fn = function() {};

A new function is created. If there were such a thing as get_fnname then it would output different names in that scenario:

function createTestClass() {
     $fn = function() {};

     return $fn;
}

var_dump(get_fnname(createTestClass())); // fn:bae86497b6
var_dump(get_fnname(createTestClass())); // fn:f80731f1b8

And not the same function name twice, because nobody would expect such a behaviour. Why should it be different for "anonymous" classes?

You're telling me that "anonymous" means "I don't care about the name" but there's a lot more to it than just "I don't care about the name" - if that were the case, anonymous function should behave the same way.

It's nothing new that PHP has a lot of inconsistencies and your (excluding @cmb69) behaviour is really unwelcoming.

That's nothing new to me, by the way, because every time I do report something on PHP's bug tracker the response has always been "you're using it wrong" - no I'm not using it wrong, you just can't admit to a bug because it would be too much work to fix it.

I'm not 100% sure, but what you can do with "anonymous" classes you can already do with __set, __get and __call - so what's really "new" about anonymous classes except that they're written differently - now:

Tell me how I can "create" a class that does the same thing (__set, __get etc.) but with static properties/methods. I couldn't find a way (except for eval) so that would be a use case I'd see for "anonymous" classes.

You may think static doesn't make sense here, but let me explain: static can also be used to communicate intend: if I'm declaring a method static, I'm communicating I'm not using any instance state whatsoever. So even if it's meaningless on a technical level, it's very meaningful in the resulting code:

// I can be reasonably sure that "myMethod" doesn't use any instance state
$cls::myMethod();

// I cannot reasonably assume that 'myMethod' will not use instance stace
$cls->myMethod();

In any case, I will just use a wrapper class with __get, __set etc., but I do think it's sad and a missed opportunity for PHP to grow.

marco-a avatar Dec 08 '22 16:12 marco-a

Why should it be different for "anonymous" classes?

I don't know how this is implemented exactly in code, but functions are created with the current scope attached, especially: $this (unless created with static), variables in use (...) and in case of arrow functions all other variables. On the other hand class doesn't share the scope at all, the only shared data are these provided to the constructor.

And also once defined class type stays registered until the whole script is over (only instance is freed from memory), whereas anonymous function (not being really a function, but Closure instance), can be removed from memory as soon as there is no reference to it. Example: https://3v4l.org/vj8Ia

It's nothing new that PHP has a lot of inconsistencies and your (excluding @cmb69) behaviour is really unwelcoming.

That wasn't my intent to sound harsh.

the response has always been "you're using it wrong" - no I'm not using it wrong, you just can't admit to a bug because it would be too much work to fix it.

This is unfortunately the case from time to time, but sometimes "people are using it wrong"

I'm not 100% sure, but what you can do with "anonymous" classes you can already do with __set, __get and __call - so what's really "new" about anonymous classes except that they're written differently

Many goals can be achieved with different tools. Anonymous classes are mostly syntax sugar, for me the main benefit is that for single use you don't need to create class with name which will be clogging IDE suggestions down the road.

KapitanOczywisty avatar Dec 08 '22 18:12 KapitanOczywisty

It's not intuitive at all. If I do a new class() I expect it to create a new class every time I do that.

I would not necessarily expect that; new means create a new instance, regardless of whether it is applied to a class name, or an "anonymous" class declaration.

That's nothing new to me, by the way, because every time I do report something on PHP's bug tracker the response has always been "you're using it wrong" - no I'm not using it wrong, you just can't admit to a bug because it would be too much work to fix it.

It is not necessarily too much work for us to fix it, but rather for thousands of userland developers to change their code. "Anonymous" classes behave that way since their introduction (i.e. PHP 7.0.0), and likely existing code relies on this.

I'm not 100% sure, but what you can do with "anonymous" classes you can already do with __set, __get and __call - so what's really "new" about anonymous classes except that they're written differently

See https://wiki.php.net/rfc/anonymous_classes#use_cases.

Tell me how I can "create" a class that does the same thing (__set, __get etc.) but with static properties/methods.

I wouldn't know how to do that either, but that doesn't necessarily mean that this use-case has to be supported by anonymous classes. Anyhow, I suggest you write to the internals mailing list to gauge support for your use-case.

That wasn't my intent to sound harsh.

And I don't think you did. After all, none of us appears to be a native English speaker, so there might be language barrier issues. :)

cmb69 avatar Dec 09 '22 15:12 cmb69

I would not necessarily expect that; new means create a new instance, regardless of whether it is applied to a class name, or an "anonymous" class declaration.

You're missing the point. I'm not talking about new class() specifically, but more like the class has no name, so I'd expect it to be unique for every instance created.

You can already access static properties on an instance object of a class (if that's a good thing, I don't know, but PHP has support for it):

<?php

class MyClass {
    static public $my_prop = 1;
}

$my_object = new MyClass;

var_dump($my_object::$my_prop);

Now that I'm reading this code, I'd strongly argue that it doesn't make any real sense.

But that's not the point.

Point is, you can access static methods and properties through an instance object.

In the case of an anonymous class, I'd expect every instance to be isolated from the next.

It is not necessarily too much work for us to fix it, but rather for thousands of userland developers to change their code. "Anonymous" classes behave that way since their introduction (i.e. PHP 7.0.0), and likely existing code relies on this.

It actually is. Because you'd have to implement some sort of deprecation notice to make people aware that they would need to change their code. And besides, if "I'm doing it wrong" why should you actually care about breaking people's code? I think @mvorisek said static would be useless - so what's the real problem here? Are you worried about breaking people's "useless" code?

And I don't think you did. After all, none of us appears to be a native English speaker, so there might be language barrier issues. :)

I never made the claim that I was treated "harshly" that's something @KapitanOczywisty said.

What I said is that you're making me feel uncomfortable. Like @KapitanOczywisty I don't need a lecture on how to benchmark my PHP code, alright? Besides that, your benchmark code is pretty bad so I don't even know if you know what you're talking about. Assuming that your results are valid, 10000 is 10'000ns which are 10μs so what are you trying to say? 10μs is nothing for an application that doesn't do things in real time. You can't even microsleep for that amount. But instead you choose to bring up a point that is completely irrelevant to the discussion. Performance isn't a concern if you're using a language like PHP.

Bottom line is: if you think (or argue) that this is not a bug, then I'd have to disagree. I don't think its sane, I don't think it makes sense - but at the end of the day, it's your project so do whatever you want with it.

This is going to be my last comment on this matter and you can decide how to move forward.

marco-a avatar Dec 09 '22 23:12 marco-a

I don't need a lecture on how to benchmark my PHP code, alright?

It's not a benchmark... It's only illustration of how classes are working under the hood. The only thing you should see there is that you cannot fully remove class type and in the earlier one that creating new class type is expensive (even an empty one). I wrote that clearly before the code - and I'm sad you didn't even bother to read that.

Assuming that your results are valid, 10000 is 10'000ns which are 10μs so what are you trying to say? 10μs is nothing for an application that doesn't do things in real time.

I'm not sure which code you're referring, I'm assuming the first one. In both cases these were empty classes, and it was 10x time slower to create the first one. Add some properties, functions, constants and you can go much much slower. Beside that, as I've mentioned before, class type cannot be removed until a script is over, so memory usage would slowly rise over time - we call it memory leak.

Performance isn't a concern if you're using a language like PHP.

If you're writing simple web page it doesn't matter at all, and you shouldn't even care, but scaling up these things can add up really fast.

There are some great articles about PHP internals, It's worth taking a look to understand what happens when PHP creates a new instance and what is kept from class even if no instance is present: https://wiki.php.net/internals/engine/objects

KapitanOczywisty avatar Dec 10 '22 02:12 KapitanOczywisty