Export stdClass objects using (object) cast #2420

hikari-no-yume · 2017-03-14T16:53:11Z

Before this change, var_export()'s output for stdClass objects calls the non-existent stdClass::__set_state method, and is therefore useless.

This commit makes var_export() output an (object) cast from an array instead, which when evaluated, will produce a stdClass object. Other classes see unchanged output.

kelunik · 2017-03-14T16:55:08Z

Did you think about adding stdClass::__set_state instead?

I don't care how it's implemented, but 👍

hikari-no-yume · 2017-03-14T17:02:44Z

I didn't! But that's an interesting idea, and we might want to also implement that.

Using (array) adds forwards-compatibility: output produced on 7.2 (say) would still eval() right on older versions. Unlikely to be much of a concern in practice, but who knows, maybe something like WordPress could benefit.

Adding ::__set_state() would add backwards-compatibility: output produced on ≤7.1 (say) which happened to include stdClass would now eval() properly. Also unlikely to be much of a concern in practice.

I wonder if any userland code uses ::__set_state(), too. There could be a benefit to that code if it doesn't already special-case stdClass.

One argument against adding it to stdClass might be that we've so far kept it a completely blank class. Is that useful somehow? I doubt defining ::__set_state() would break anything.

One benefit of (object) here is that it might be slightly faster, though that's just a guess, I haven't benchmarked this. It's certainly more concise.

hikari-no-yume · 2017-03-14T17:30:37Z

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

sgolemon · 2017-03-14T17:55:09Z

Is there an RFC for this? I don't forsee issues, but it's non-trivial enough to merit discussion IMO.

krakjoe · 2017-03-14T17:57:32Z

I agree. If overwhelming consensus emerges from a discussion, it may be possible to skip the actual RFC.

Discussion is where we should start ...

kelunik · 2017-03-14T17:57:34Z

I don't think it requires an RFC, a heads up on internals should be enough.

hikari-no-yume · 2017-03-14T18:59:16Z

@sgolemon @krakjoe @kelunik I've pinged internals here: http://news.php.net/php.internals/98521

smalyshev · 2017-03-14T22:43:15Z

What happens of some class extends stdclass? Without adding __set_state I imagine any class extending stdclass without defining __set_state is still broken. If we added __set_state though, it'd be fixed too.

JanTvrdik · 2017-03-15T10:18:10Z

ext/standard/var.c

-			smart_str_appendl(buf, "::__set_state(array(\n", 21);
+			/* stdClass has no __set_state method, but can be casted to */
+			if (Z_OBJCE_P(struc) == zend_standard_class_def) {
+				smart_str_appendl(buf, "(object)array(\n", 15);


Not sure if relevant, but PHP documentation uses space after cast, i.e. (object) array(...).

It's better to read and I thought about commenting that, too. But on the other hand it saves bytes in storage that way.

I prefer how it looks without the space.

I think it would be good to change this bit prior to merging. We get PRs fixing the var_export style from time to time (you know, fixing broken/inconsistent indentation, using short array syntax, that kind of stuff). While we have to reject these for BC reasons, it would be preferable if we at least did not introduce unorthodox formatting for new functionality.

Alright, I decided to not be stubborn and accept the apparent overwhelming consensus here, and changed this when I rebased today. The manual is a particularly good argument.

hikari-no-yume · 2017-03-15T18:56:12Z

CI's doing something weird here. The second commit is marked as failing, but the failing Travis build is for a different commit.

marcioAlmada · 2017-03-15T20:39:50Z

@TazeTSchnitzel try to nudge Travis with an empty commit

hikari-no-yume · 2017-03-16T11:01:56Z

Given this is a new addition, and 5.3 is out of support, we could perhaps use short array syntax ([] instead of array()). But that would be inconsistent. That could be its own pull-request.

nikic · 2017-03-16T11:44:59Z

ext/intl/tests/dateformat_format.phpt failure looks legit.

nikic · 2017-03-16T11:50:26Z

@TazeTSchnitzel From Travis docs:

Rather than test the commits that have been pushed to the branch the pull request is from, we test the merge between the origin and the upstream branch.

That's why you're seeing a different commit ID.

hikari-no-yume · 2017-03-16T11:52:26Z

Oh, I'm silly. I fixed one ext/intl test, but not the other. >.>

hikari-no-yume · 2017-08-25T20:30:25Z

I completely forgot about this. I don't think there's been any opposition… do @remicollet @sgolemon have any interest in merging this for 7.2? Otherwise I'll put it in master.

hikari-no-yume · 2018-07-04T21:10:44Z

@smalyshev @cmb69 Since it's 7.3 soon and nobody has touched this for a year, do you have any interest in this change?

cmb69 · 2018-07-04T21:30:47Z

@hikari-no-yume I certainly like this PR (and IMHO https://bugs.php.net/48016 is more bug than feature request). However, it's not the role of the RMs to decide what features go into a release.

Since this discussion as well as the internals discussion showed some appriciation and no objections, I'm pragmatic:

If nobody objects, I'll merge this PR into master on 2018-07-12, so that it goes into PHP 7.3.

hikari-no-yume · 2018-07-04T21:34:59Z

Sounds good. I should have merged this into master a while ago, to be honest.

smalyshev · 2018-07-05T02:57:21Z

I would add a note in UPGRADING though.

cmb69 · 2018-07-05T09:25:50Z

I would add a note in UPGRADING though.

Sure. :)

hikari-no-yume · 2018-07-05T20:05:32Z

I rebased it, squashed it, tested it still worked, changed the syntax used from (object)array( to (object) array(, and added NEWS and UPGRADING notes. For the former's purposes I'm counting it as fixing bug #48016.

Before this change, var_export()'s output for stdClass objects calls the non-existent stdClass::__set_state method, and is therefore useless. This commit makes var_export() output an (object) cast from an array instead, which when evaluated, will produce a stdClass object. Other classes see unchanged output.

hikari-no-yume · 2018-07-05T20:41:15Z

ext/standard/var.c

-			smart_str_appendl(buf, "::__set_state(array(\n", 21);
+			/* stdClass has no __set_state method, but can be casted to */
+			if (Z_OBJCE_P(struc) == zend_standard_class_def) {
+				smart_str_appendl(buf, "(object) array(\n", 16);


The hardest two things in computer science are, of course, off-by-one errors. This was briefly smart_str_appendl(buf, "(object) array(\n", 15); and I was confused why there was no newline…

sizeof(…)-1 to the rescue?

I wanted to do one better and use ZEND_STRL, but I can't because smart_str_appendl is a macro. I could use smart_str_appends, but that it uses strlen annoys me at some level, even though any reasonable compiler™ will do the right thing…

Thanks, I was not aware of either! ZEND_STRL is … fascinating! Anyway, smart_str_appends might still be preferable here – after all, it's just about a potentially missing optimization, and I prefer clean code over any optimization, unless the latter proves to be worth the trouble. I'm fine with your current solution, though, since it doesn't make things worse.

php-pulls · 2018-07-12T11:26:00Z

Comment on behalf of cmb at php.net:

Thanks! Applied via e4e9cd8.

krakjoe added the RFC label Mar 14, 2017

JanTvrdik reviewed Mar 15, 2017

View reviewed changes

hikari-no-yume force-pushed the var_export_stdClass branch from f52e6f4 to 29a21b8 Compare March 16, 2017 11:54

smalyshev added the Waiting on RM label Jul 5, 2018

hikari-no-yume force-pushed the var_export_stdClass branch from 29a21b8 to 74c9275 Compare July 5, 2018 20:04

hikari-no-yume force-pushed the var_export_stdClass branch from 74c9275 to b934e62 Compare July 5, 2018 20:08

hikari-no-yume force-pushed the var_export_stdClass branch from b934e62 to c7a37e1 Compare July 5, 2018 20:39

hikari-no-yume commented Jul 5, 2018

View reviewed changes

nikic added PHP 7.3 Nominated and removed Waiting on RM labels Jul 5, 2018

php-pulls closed this Jul 12, 2018

hikari-no-yume mentioned this pull request Feb 24, 2020

var_export: Mention 7.3.0 change for stdClass php/doc-en#49

Closed

Export stdClass objects using (object) cast #2420

Export stdClass objects using (object) cast #2420

Uh oh!

Conversation

hikari-no-yume commented Mar 14, 2017

Uh oh!

kelunik commented Mar 14, 2017

Uh oh!

hikari-no-yume commented Mar 14, 2017

Uh oh!

hikari-no-yume commented Mar 14, 2017

Uh oh!

sgolemon commented Mar 14, 2017

Uh oh!

krakjoe commented Mar 14, 2017

Uh oh!

kelunik commented Mar 14, 2017

Uh oh!

hikari-no-yume commented Mar 14, 2017

Uh oh!

smalyshev commented Mar 14, 2017

Uh oh!

JanTvrdik Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hikari-no-yume commented Mar 15, 2017

Uh oh!

marcioAlmada commented Mar 15, 2017

Uh oh!

hikari-no-yume commented Mar 16, 2017

Uh oh!

nikic commented Mar 16, 2017

Uh oh!

nikic commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hikari-no-yume commented Mar 16, 2017

Uh oh!

hikari-no-yume commented Aug 25, 2017

Uh oh!

hikari-no-yume commented Jul 4, 2018

Uh oh!

cmb69 commented Jul 4, 2018

Uh oh!

hikari-no-yume commented Jul 4, 2018

Uh oh!

smalyshev commented Jul 5, 2018

Uh oh!

cmb69 commented Jul 5, 2018

Uh oh!

hikari-no-yume commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-pulls commented Jul 12, 2018

Uh oh!

Uh oh!

JanTvrdik Mar 15, 2017 •

edited

Loading

nikic commented Mar 16, 2017 •

edited

Loading