Skip to content

Observer: fake closures #6607

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

Closed
wants to merge 1 commit into from
Closed

Conversation

SammyK
Copy link
Contributor

@SammyK SammyK commented Jan 15, 2021

Methods are not observed from closures created via Closure::fromCallable() because it creates a fake closure. The observer API explicitly cannot observe fake closures because the observer handlers are stored in the runtime cache of op_arrays.

I haven't figured out an obvious solution yet so I thought I'd throw this out there in case anyone has any good ideas. :)

@derickr: @morrisonlevi shared your use case with me and I think this is the root issue as to why RouterListener->onKernelFinishRequest isn't being observed. It looks like it's being called after being converted into a fake closure in the EventDispatcher.

@SammyK
Copy link
Contributor Author

SammyK commented Jan 15, 2021

TIL that the ext/zend_test tests are no longer running in CI. 🤔

@SammyK
Copy link
Contributor Author

SammyK commented Jan 15, 2021

It looks like we might have access to the original runtime cache if we "unwrap" the fake closure in _zend_observe_fcall_begin. I'll tinker with this a bit more next week.

@nikic
Copy link
Member

nikic commented Jan 18, 2021

TIL that the ext/zend_test tests are no longer running in CI.

It might be that they never actually ran ... The problem seems to be that the extension is located in ext/zend_test but reports its name as zend-test.

@nikic
Copy link
Member

nikic commented Jan 18, 2021

I've opened #6613 to change the extension name, which should also result in the tests being run.

@SammyK
Copy link
Contributor Author

SammyK commented Jan 19, 2021

It might be that they never actually ran

They were running in TravisCI but I think exclusively so.

I'll be rebasing this now that #6613 is merged (thanks again Nikita!) and start digging into this again later in the week (most likely on Friday).

@dstogov
Copy link
Member

dstogov commented Jan 26, 2021

I also got into the same problem with skipping fake closures.
May be, I suggested to do this myself, but I can't remember the reason or reproduce any problems.

@dstogov
Copy link
Member

dstogov commented Jan 27, 2021

After some testing, I think we may remove ZEND_ACC_FAKE_CLOSURE limitation.

@SammyK SammyK force-pushed the observer/fake-closures branch 2 times, most recently from 92b603e to da89066 Compare January 27, 2021 19:40
@SammyK SammyK force-pushed the observer/fake-closures branch from da89066 to 3fd35de Compare January 27, 2021 23:06
@SammyK SammyK marked this pull request as ready for review January 28, 2021 00:22
@SammyK
Copy link
Contributor Author

SammyK commented Jan 28, 2021

@dstogov Thanks for checking this out. I tried to track back why we avoided fake closures when adding the API but couldn't find the source. At any rate, the tests are passing now so I think this should be good to go. :)

@php-pulls php-pulls closed this in 17142ea Jan 28, 2021
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.

3 participants