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.

Sign up for GitHub

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

Jump to bottom

Workaround php bug #81634 in PHP 8.0-8.1.0 #2353

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Workaround php bug #81634 in PHP 8.0-8.1.0 #2353

merged 1 commit into from
Nov 20, 2023

Conversation

cataphract
Copy link
Contributor

Description

Workaround for php bug #81634 in PHP 8.0-8.1.0

Readiness checklist

  • (only for Members) Changelog has been added to the release document.
  • Tests added for this feature/bug.

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.

@cataphract cataphract requested a review from a team as a code owner November 10, 2023 08:10
@cataphract cataphract force-pushed the glopes/workaround-tls branch 2 times, most recently from 307b7cf to 1c6b50d Compare November 10, 2023 08:22
Anilm3
Anilm3 reviewed Nov 10, 2023
View reviewed changes
Copy link
Contributor

@Anilm3 Anilm3 left a comment

Choose a reason for hiding this comment

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

There seems to be some header order / inclusion dependency with compatibility.h (specifically around zend_get_closure_method_def). Adding zend_closure.h to compatibility.h should fix it...

@cataphract cataphract requested a review from a team as a code owner November 10, 2023 12:22
@Anilm3
Copy link
Contributor

Anilm3 commented Nov 10, 2023

Looks like now the profiler is failing because of the inclusion of ddtrace.h in handlers_api.c, maybe the best approach is to separate the tsrmls cache setup into its own file like you do in zend_abstract_interface.

@cataphract cataphract force-pushed the glopes/workaround-tls branch 2 times, most recently from d21accc to 495649f Compare November 17, 2023 13:44
Anilm3
Anilm3 reviewed Nov 17, 2023
View reviewed changes
Copy link
Contributor

@Anilm3 Anilm3 left a comment

Choose a reason for hiding this comment

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

Looking good although don't forget to rebase so that appsec tests are executed in CI as well.

@bwoebi
Copy link
Collaborator

bwoebi commented Nov 19, 2023

I've rebased it myself on master now, so that we may be able to merge on Monday morning.

bwoebi
bwoebi approved these changes Nov 20, 2023
View reviewed changes
Copy link
Collaborator

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Looks good :-)

@bwoebi bwoebi merged commit 002a5c3 into master Nov 20, 2023
600 of 601 checks passed
@bwoebi bwoebi deleted the glopes/workaround-tls branch November 20, 2023 15:23
@bwoebi bwoebi added this to the 0.94.0 milestone Nov 20, 2023
@cataphract cataphract mentioned this pull request Nov 21, 2023
Merged
5 tasks
@Anilm3 Anilm3 mentioned this pull request Nov 23, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Anilm3 Anilm3 Anilm3 left review comments

@bwoebi bwoebi bwoebi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.94.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cataphract @Anilm3 @bwoebi