Skip to content

Conversation

morrisonlevi
Copy link
Contributor

This PR fixes bug 66622. It does three things:

  • Moves Zend/tests/closure_*.phpt to Zend/tests/closure/*.phpt. I was running these tests a lot and didn't have time to run the full Zend/tests suite. Some minor cleanup associated with the file renaming.
  • Fixes static closures to use late static binding and adds tests for it.
  • Automatically promotes non-static closures to be static when defined in a static method and adds tests for it.

@Tyrael
Copy link
Contributor

Tyrael commented May 14, 2014

a little bit offtopic, fyi you can do stuff like this:
TESTS=Zend/tests/closure_*.phpt make test
which will only execute those tests.

@smalyshev
Copy link
Contributor

I think moving tests around is not necessary, you can always run run-tests.php Zend/tests/closure_*.phpt.

@morrisonlevi
Copy link
Contributor Author

Is there any reason not to move the tests? It's a small detail either way but it seems better to move them in my opinion, even if only for the fact so I can name a test closure/bug66622.phpt instead of closure_bug66622.phpt.

@smalyshev
Copy link
Contributor

Yes, three reasons: 1) less disruption, easier to check the patch 2) if it ain't broken, don't fix it :) 3) since all other tests are still there, inconsistency is created - now closure is in subdir, but nowdocs and exceptions are not.

This also adds some smaller, isolated tests related to bug 66622.
@morrisonlevi
Copy link
Contributor Author

I've rebased the commits to not change the location of the *.phpt files, even though I think they really do belong in a subdirectory.

@smalyshev
Copy link
Contributor

I'll have to split the tests for 5.4, since 5.4 does not have ::class.

@morrisonlevi
Copy link
Contributor Author

Stas,

I was thinking this would only go into 5.6; it is a bug fix but it does alter behavior (although a rarely used one, in my experience). If you think it is fine to get backported to 5.4 and 5.5 that's fine with me, but I wasn't expecting it.

@smalyshev
Copy link
Contributor

The new behavior makes sense, I don't think anybody relied on closures ignoring the scope and still used static:: there.

@php-pulls
Copy link

Comment on behalf of stas at php.net:

merged

@php-pulls php-pulls closed this Jun 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants