php / php-src

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

var_export on anonymous classes #10066

Open KapitanOczywisty opened 1 year ago

KapitanOczywisty commented 1 year ago

Description

I couldn't find that reported, so I'll open a new issue.

The following code:

<?php
var_export(new class{});

Resulted in this output: https://3v4l.org/CQ6qK

\class@anonymous/in/CQ6qK:3$0::__set_state(array(
))

But I expected this output instead:

NULL

or

new class {}

PHP Version

PHP 8.2 and older

Operating System

No response

stramunin commented 1 year ago

this code new class{} create new object, so var_export same as var_dump:

$ php -r "var_dump(new class{});"
object(class@anonymous)#1 (0) {
}
cmb69 commented 1 year ago

Yeah, it doesn't make much sense to var_export() that; neither is ::__set_state() defined, nor would that class name be accepted. So the premise of var_export() is violated: "the returned representation is valid PHP code."

KapitanOczywisty commented 1 year ago

@cmb69 I suspect trying to rebuild anonymous class is not worth the hustle and in edge cases (e.g. static) could introduce some errors, so NULL would be ok like for resource? And probably no warning like in other cases?

cmb69 commented 1 year ago

@KapitanOczywisty, yes, I agree.

stramunin commented 1 year ago

Why expect NULL? Anonymous classes are an edge case. Just like anonymous functions, eval throws a warning because method __set_state is not defined. Also, anonymous classes create objects (usually for one time) and contain some data. Therefore, the output of the data of these objects is useful. For example, instead of displaying on the screen, you need to save the contents of the variable for debugging or logging. And there will be something like this:

<?php
$a = new class { public $pi = 3.14; function foo () {} };
// something broken
$s = var_export($a, true);
error_log($s, 3, 'error.log');
$ cat error.log
class@anonymous/index.php:3$4::__set_state(array(
   'pi' => 3.14,
))
cmb69 commented 1 year ago

var_export() guarantees valid PHP code; this is currently not valid. If you want to log something, use var_dump() (use an output buffer if necessary).

stramunin commented 1 year ago

I know it.

Read descripition:

It is similar to var_dump() with one exception: the returned representation is valid PHP code.

And this note:

To be able to evaluate the PHP generated by var_export(), all processed objects must implement the magic __set_state method.

Also note from __set_state:

When exporting an object, var_export()) does not check whether __set_state() 
is implemented by the object's class, so re-importing objects will result in an Error exception, 
if __set_state() is not implemented.
Particularly, this affects some internal classes. 
It is the responsibility of the programmer to verify that only objects will be re-imported, 
whose class implements __set_state().

Now the description of this function, together with the notes, does not conflict its behavior.

The question is why only var_export on anonymous class should output NULL in your opinion. Maybe it's worth checking all cases in runtime, because you said _varexport() guarantees valid PHP code?

cmb69 commented 1 year ago

[…] nor would that class name be accepted.

stramunin commented 1 year ago

Excellent. You just need to add an edge case with an anonymous class to the description of the function and that's it.

stramunin commented 1 year ago

I think the simplest and most logical thing is to fix the documentation. And you previously suggested a more complicated way to output a variable, but why if everything already works like that? :)

KapitanOczywisty commented 1 year ago

There are 3 options: A) We could put anonymous class name into the quotes (with \0 in mind) and it would be a valid code https://3v4l.org/RXBTu

('class@anonymous' . "\0" . '/in/RXBTu:3$0')::__set_state(array(
   'x' => 12,
))

However:

  1. Import would be successful only after first use of that class
  2. Autoloading is technically possible, but It would be a hack in the best case.
  3. Any change to the position of anonymous class definition and export is useless.
  4. Code have \0 character, which have to be escaped making more mess

So only viable for logging purposes, not being the best at it at the same time.

B) As mentioned before: Trying to rebuild anonymous class, would have undefined behavior with static properties, and I suspect It wouldn't be possible at all with opcache, so this is out the window.

C) The other option is to return NULL and don't even pretend. I'd consider only adding a warning, like there is for circular reference. I'd also kill output for anonymous functions, afik they are always returning empty state, so these are even less useful than anonymous classes.

stramunin commented 1 year ago

Why do you want to prevent the output of these objects? The documentation says that internal objects may not implement a static method and therefore var_export will always produce invalid code. Generators cannot be created manually, but they are the same objects as closures. If you want to change the behavior of this function just because the documentation says it should return valid code, then you need to check all edge cases, not just these. If a future version of PHP introduces yet another internal class that cannot be instantiated, you will need to discuss this problem again and every time.

KapitanOczywisty commented 1 year ago

The documentation says that internal objects may not implement a static method and therefore var_export will always produce invalid code.

Will produce NOT WORKING, but still VALID code! Anonymous classes are currently producing INVALID code - trying to use it will result in PARSE_ERROR of the whole file, but NOT WORKING code can be handled with surrounding try/catch. NOT WORKING code can be also processed with tokenize, ast etc..

If a future version of PHP introduces yet another internal class that cannot be instantiated, you will need to discuss this problem again and every time.

I'm ok with that. However, we're not talking about some internal classes, but about the whole groups of structures, which all happen to be represented by a class. Functions and Generators will never be exportable, there is no chance that __set_state will be implemented for them - though these are producing valid code, so I don't care that much, this is not the case with anonymous classes, and these have to go - one way or the other.

stramunin commented 1 year ago

I don't understand you, can you explain again with a code example? For example:

$a = new class {};
$sa = var_export($a, true);
try {
    eval('$aa='.$sa.';');
}
catch (Throwable $e) {}
echo 1;

Outputs now on my version: 1

$ echo $?
0

What should change in the behavior of the PHP script after your fixes?

ktomk commented 1 year ago

@KapitanOczywisty: You'll face the same problem already with VALID (again) PHP code that is NOT WORKING (but despite PARSING (again) VALID (but still) _NOTWORKING) when the autoloader is unable to summon the definition of a STANDARD-CLASS (non-anonymous but not internal as stdClass).

TLDR: the export under the promise of being valid code that fails to parse on "importing" seems legit to me, with the benefit to fail earlier than triggering an autoloader even. Or compared with exporting an anonymous function: imagine what would happen if the actual code would be exported. some would deem it "handy" but the practical outcome would be to have an additional sink of eval. eval =/= var -- and eval should never be exported/imported (just in case you need that, use include etc. and yes, you can eval(php) it, too). you only want to export/import the static data (at least for 99% percent of the cases, there are always exclusion to such rules, but let's adhere to shared nothing here for the benefit of us all - if not only to keep backwards/forward compatibility).