close
The Wayback Machine - https://web.archive.org/web/20220327215100/https://github.com/php/php-src/pull/8233
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

Fix #8232 - always reference classes in var_export() via their FQCN #8233

Open
wants to merge 1 commit into
base: PHP-8.1
Choose a base branch
from

Conversation

Ocramius
Copy link
Contributor

@Ocramius Ocramius commented Mar 21, 2022

This fix corrects a behavior of var_export() that was mostly "hidden" until PHP 8.1 introduced:

  • properties with object initializers
  • constants containing object references
  • default values of class properties containing enums

Since var_export(..., true) is mostly used in conjunction with code generation,
and we cannot make assumptions about the generated code being placed in the root
namespace, we must always provide the FQCN of a class in exported code.

For example:

<?php

namespace MyNamespace { class Foo {} }

namespace { echo "<?php\n\nnamespace Example;\n\n" . var_export(new \MyNamespace\Foo(), true) . ';'; }

produces:

<?php

namespace Example;

MyNamespace\Foo::__set_state(array(
));

This code snippet is invalid, because Example\MyNamespace\Foo::__set_state() (which
does not exist) is called.

With this patch applied, the code looks like following (valid):

<?php

namespace Example;

\MyNamespace\Foo::__set_state(array(
));

Fixes #8232
Ref: Ocramius/ProxyManager#754

@Ocramius Ocramius changed the base branch from master to PHP-8.1 Mar 21, 2022
Zend/tests/bug73350.phpt Outdated Show resolved Hide resolved
ext/intl/tests/dateformat_format.phpt Outdated Show resolved Hide resolved
ext/intl/tests/dateformat_format_variant2.phpt Outdated Show resolved Hide resolved
ext/intl/tests/dateformat_format_variant3.phpt Outdated Show resolved Hide resolved
ext/spl/tests/fixedarray_023.phpt Outdated Show resolved Hide resolved
ext/standard/tests/array/007.phpt Show resolved Hide resolved
main/php_version.h Outdated Show resolved Hide resolved
@iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Mar 21, 2022

Related. doctrine/orm#9371 IMO this change is too controversial for a patch for sure.

EDIT: Just adding the \ for enums would be less controversial and OK from my side.

EDIT 2: IN any case, you might want to see what the mailing list thinks about this change.

@Ocramius Ocramius force-pushed the fix/#8232-ensure-var-export-always-produces-namespaced-code branch from 1e45b50 to 8e0a680 Compare Mar 21, 2022
@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Mar 21, 2022

Nothing controversial about it: var_export() exports values for usage somewhere else, so IMO this is fine as-is. If other maintainers want to bring this up to RFC level, that's up to them, but IMO this is a clear bugfix.

@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Mar 21, 2022

Note: a userland fix is not viable, because values need to be referenced via FQCN even in nested structures like: [[Foo::BAR]] being var_export()-ed.

@Ocramius Ocramius force-pushed the fix/#8232-ensure-var-export-always-produces-namespaced-code branch from 8e0a680 to a53ad90 Compare Mar 21, 2022
@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Mar 21, 2022

Well, poop. @JetBrains seems to drop all whitespace on commit (even after I disabled all the tooling around it).

Guess editing from plaintext editor it is :D

@iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Mar 21, 2022

@Ocramius You can revert all test changes and run the tests with --bless.

@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Mar 21, 2022

Can't run tests locally due to https://bugs.php.net/bug.php?id=80585 - manual editing required for now.

@IonBazan
Copy link

@IonBazan IonBazan commented Mar 21, 2022

@Ocramius to me it also looks quite controversial. Especially that serialize(), $object::class and get_class() output it without the leading slash so it seems a bit inconsistent from there. I understand this is the easiest solution but I am a bit worried about existing code that relies on current behaviour.

Also found another related issue: doctrine/orm#9371
See also some ancient bug: https://bugs.php.net/bug.php?id=64554

@Ocramius Ocramius force-pushed the fix/#8232-ensure-var-export-always-produces-namespaced-code branch from a53ad90 to 8b12af4 Compare Mar 21, 2022
@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Mar 21, 2022

  • serialize() produces a string that is designed to be used with unserialize()
  • var_export() produces a code snippet of executable PHP code, to be used with include, require or eval()

Different use-cases, no intended consistency across the two.

@Ocramius Ocramius force-pushed the fix/#8232-ensure-var-export-always-produces-namespaced-code branch from 8b12af4 to bca7f7f Compare Mar 21, 2022
Ocramius added a commit to Ocramius/doc-en that referenced this issue Mar 21, 2022
…ort()` docs

`var_export()` needs to prefix classes it references with `\`, because its code could be transplanted in
any source location/namespace, so the assumption of it being used only in the context of the root
namespace is not sufficient.

For example, in a code snippet like following ( https://3v4l.org/4mONc ):

```php
<?php

class SomeObject {}
var_export([new SomeObject]);
```

This should produce:

```
array (
  0 => 
  \SomeObject::__set_state(array(
  )),
)
```

Userland should not concern itself with the contents of the `var_export()`-produced code
snippets, and use them as-is instead.

Ref: php/php-src#8232
Ref: php/php-src#8233
Ref: Ocramius/ProxyManager#754
@Ocramius
Copy link
Contributor Author

@Ocramius Ocramius commented Mar 21, 2022

Since I never (ever) read PHP docs, I went there and checked for the ancient bug found by @IonBazan, and went to actually fix the problem at its root there too.

Docs PR is therefore at php/doc-en#1472

Also: holy crap, people post-processing var_export() output with regular expressions are totally nuts - some comments in there really need to be deleted, before they lay eggs :|

@Ocramius Ocramius force-pushed the fix/#8232-ensure-var-export-always-produces-namespaced-code branch 2 times, most recently from b517e42 to 52d28ad Compare Mar 21, 2022
This fix corrects a behavior of `var_export()` that was mostly "hidden" until PHP 8.1 introduced:

* properties with object initializers
* constants containing object references
* default values of class properties containing `enum`s

Since `var_export(..., true)` is mostly used in conjunction with code generation,
and we cannot make assumptions about the generated code being placed in the root
namespace, we must always provide the FQCN of a class in exported code.

For example:

```php
<?php

namespace MyNamespace { class Foo {} }

namespace { echo "<?php\n\nnamespace Example;\n\n" . var_export(new \MyNamespace\Foo(), true) . ';'; }
```

produces:

```php
<?php

namespace Example;

MyNamespace\Foo::__set_state(array(
));
```

This code snippet is invalid, because `Example\MyNamespace\Foo::__set_state()` (which
does not exist) is called.

With this patch applied, the code looks like following (valid):

```php
<?php

namespace Example;

\MyNamespace\Foo::__set_state(array(
));
```

Ref: php#8232
Ref: Ocramius/ProxyManager#754
@Ocramius Ocramius force-pushed the fix/#8232-ensure-var-export-always-produces-namespaced-code branch from 52d28ad to 4f5a3b0 Compare Mar 21, 2022
@cmb69
Copy link
Contributor

@cmb69 cmb69 commented Mar 21, 2022

IN any case, you might want to see what the mailing list thinks about this change.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants