Fixed exportVariable multiple backslash problem

[shabbyrobe]

When a global variable contains an object which contains a string which contains multiple backslashes (phew!) and you are using @runTestsInSeparateProcesses and @preserveGlobalState, PHPUnit_Util_GlobalState modifies the result of serialize in a way that breaks the unserialize.

This gist contains the smallest demonstration I could cook up of what happens: https://gist.github.com/shabbyrobe/cf0b2fd76220ecd6e991

I wrote a test case which showed that all cases except the multiple backslash object one passed, then replaced the call to str_repace with a call to var_export which fixes the problem without breaking anything I could think of.

[whatthejeff]

Thanks, nice find!

Would you mind fixing this in the stable branch (4.1)? Also, I think the test could be improved. You could basically port your gist to a .phpt test in our tests/Regression/GitHub directory. Check out some of the other tests in that directory to get a feel for how it works. I think this is preferable to testing a method that's not intended to be part of the public API.

[shabbyrobe]

Cool, thanks! I've added an extra test in to the regression folder mimicking the surrounding style and have tested it works by re-introducing the bug and re-fixing it.

I think the test of the non-public method should stay though - I used it to make sure I wasn't breaking any of the existing scenarios I knew worked (scalars, arrays, etc)

Should I submit another PR with all of the changes and a separate one for the 4.1 fix?

[shabbyrobe]

Sorry, I misunderstood. I thought you meant fixing it in the stable branch as well as the master, now I realise you meant to do it in 4.1 and only 4.1! My bad, I'll sort it out now.

[shabbyrobe]

Sorry, another question - which number should I give the regression test? Should I open a Github issue for it?

[whatthejeff]

@shabbyrobe

I think the test of the non-public method should stay though - I used it to make sure I wasn't breaking any of the existing scenarios I knew worked (scalars, arrays, etc)

It's a bad idea to test non-public methods in this way. One of the benefits of testing is to ease refactoring. Directly testing non-public methods actually makes refactoring more difficult. Generally speaking, the non-public methods are covered indirectly through the testing of the public APIs. Why not add scalars, arrays, etc. to your regression test?

Sorry, another question - which number should I give the regression test? Should I open a Github issue for it?

1335 works for me :)