Skip to content

Conversation

chschneider
Copy link
Contributor

This PR fixes some annoyances with var_export:

  • No more trailing spaces after => followed by newline
  • The indentation of nested object keys was fixed to match rest of indentation

Some PHP tests had to be adapted.
This is a minor BC break as it could break other projects depending on var_export formatting, e.g. in tests.

@chschneider
Copy link
Contributor Author

The related discussion can be found at https://marc.info/?t=161374346900003&r=1&w=2

@ramsey
Copy link
Member

ramsey commented May 8, 2021

Changes to output of var_export() have been discussed at length:

It seems like a minor BC break, but it can have major repercussions.

If you want to update the var_export() output, I'd recommend putting together an RFC, include some of the other changes requested (square brackets for arrays, etc.), and target PHP 9. It's still years away, but this would give folks a chance to put together PHP 9 polyfills and switch to the new format (if they use it for testing, etc.).

@ramsey ramsey added the Feature label May 8, 2021
@chschneider
Copy link
Contributor Author

It seems like a minor BC break, but it can have major repercussions.

If you want to update the var_export() output, I'd recommend putting together an RFC, include some of the other changes requested (square brackets for arrays, etc.), and target PHP 9. It's still years away, but this would give folks a chance to put together PHP 9 polyfills and switch to the new format (if they use it for testing, etc.).

I did not really want to update the output, just fix 1 1/2 bugs (the indentation is obviously a very old but, the trailing space was brought by by Sara) and after asking on internals I was encouraged to provide a PR as the var_export() output was slightly changed not too long ago and I was told tests are supposed to use other means.

But the issue is not important enough to me for an RFC ;-)

@ramsey
Copy link
Member

ramsey commented May 9, 2021

I did not really want to update the output

But this change does update the output, right? Am I missing something?

@chschneider
Copy link
Contributor Author

But this change does update the output, right? Am I missing something?

Yes, sure, it does change the output. But not as part of a bigger plan.
First of all I just wanted to fix the indentation which is clearly a bug. Then Sara suggested that the trailing spaces are an annoyance as well and might be fixed with it, so I added that.

But if any change of output is off limits (even though a change was done not too long ago) and needs a bigger discussion then let's just forget about this PR, no problem.

@cmb69
Copy link
Member

cmb69 commented Dec 2, 2021

I think to make progress here, there should better be an RFC. Doesn't have to be a large document, and I see not much need for a lengthy discussion (I mean the pros and cons are pretty obvious).

@iluuu1994
Copy link
Member

Closing as there was no response. Feel free to reopen once discussion continues.

@iluuu1994 iluuu1994 closed this Apr 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants