Skip to content
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

Auto-flushing (PHP 5) #1189

Merged
merged 3 commits into from
May 10, 2021
Merged

Auto-flushing (PHP 5) #1189

merged 3 commits into from
May 10, 2021

Conversation

SammyK
Copy link
Contributor

@SammyK SammyK commented Mar 26, 2021

Description

This is a followup PR to #819 to add auto-flushing to PHP 5. It uses the new PHP 5 ZAI method-call seam added in #1186 to flush and reset the tracer automatically from a sandboxed environment when the auto-flushing feature is enabled.

Readiness checklist

  • (only for Members) Changelog has been added to the release document.
  • Tests added for this feature/bug. (Uses existing tests that are now enabled on PHP 5.)

Reviewer checklist

  • Appropriate labels assigned.
  • Milestone is set.
  • Changelog has been added to the release document. For community contributors the reviewer is in charge of this task.

@SammyK SammyK added 🏆 enhancement A new feature or improvement c-extension Apply this label to issues and prs related to the C-extension labels Mar 26, 2021
@SammyK SammyK added this to the 0.57.0 milestone Mar 26, 2021
@labbati labbati modified the milestones: 0.57.0, 0.58.0 Apr 12, 2021
@labbati labbati modified the milestones: 0.58.0, 0.59.0 Apr 21, 2021
@SammyK SammyK mentioned this pull request Apr 26, 2021
5 tasks
@SammyK SammyK changed the base branch from master to sammyk/zend-abstract-interface April 30, 2021 15:57
@SammyK SammyK changed the title Add auto-flushing to PHP 5 Auto-flushing (PHP 5) Apr 30, 2021
@SammyK SammyK marked this pull request as ready for review April 30, 2021 17:44
@labbati labbati modified the milestones: 0.59.0, 0.60.0 May 5, 2021
@SammyK SammyK force-pushed the sammyk/zend-abstract-interface branch from ebf6fca to 6bbb941 Compare May 6, 2021 19:41
@morrisonlevi
Copy link
Collaborator

I think you possibly rebased this on master, instead of the newly rebased sammyk/zend-abstract-interface branch, which is why it's showing 5 commits and conflicts, I think?

@SammyK
Copy link
Contributor Author

SammyK commented May 6, 2021

I haven't rebased this one yet. I will rebase once #1186 is merged (due to squashed commits).

Base automatically changed from sammyk/zend-abstract-interface to master May 10, 2021 17:55
@SammyK SammyK force-pushed the sammyk/php5/auto-flushing branch from cca51bf to 78595cf Compare May 10, 2021 17:59
Copy link
Collaborator

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question on the finer semantics of zai_call_static_method_without_args.

ext/php5/auto_flush.c Show resolved Hide resolved
Copy link
Collaborator

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good to me. I just have one directional question:

I think we may want/need to abstract zvals at some point so when we are writing integrations we can program against the ZAI and it works on all PHP versions. We don't want to write each integration for each PHP version; we want to write it once, like we would in .php code.

Do you think it's important to handle this sooner or later?

@SammyK
Copy link
Contributor Author

SammyK commented May 10, 2021

I think we may want/need to abstract zvals at some point so when we are writing integrations we can program against the ZAI and it works on all PHP versions.

Indeed. This is certainly on the wishlist.

Do you think it's important to handle this sooner or later?

I think that depends on the integration API design. I think there might be a way we can write the integration API such that it can still interact with native Zend Engine data structures in an abstract, version-agnostic way. If that's the case, we can hold off on creating the ZAI data structure shims for a bit longer.

At any rate, we'll certainly have to cross that bridge when we design the ZAI integration API.

Thanks for the code review and the thoughts @morrisonlevi!

@SammyK SammyK merged commit 5275a4f into master May 10, 2021
@SammyK SammyK deleted the sammyk/php5/auto-flushing branch May 10, 2021 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-extension Apply this label to issues and prs related to the C-extension 🏆 enhancement A new feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants