Generate appsec php stub with all exposed function by extension#3858
Conversation
This comment has been minimized.
This comment has been minimized.
Benchmarks [ appsec ]Benchmark execution time: 2026-05-12 16:33:53 Comparing candidate commit a89bb60 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. |
cataphract
left a comment
cataphract
left a comment
There was a problem hiding this comment.
I'm not against having a target to regenerate arginfo, but I think we need to do validation on the version of gen_stub.php and call it in the appropriate compatibility mode.
| } | ||
|
|
||
| static PHP_FUNCTION(datadog_appsec_is_enabled) | ||
| PHP_FUNCTION(datadog_appsec_is_enabled) |
There was a problem hiding this comment.
but they will be references from outside
There was a problem hiding this comment.
they are referenced in the generated arginfo header file, which is only included in the same compilation unit. So static should be fine.
estringana
force-pushed
the
estringana/update-stub
branch
4 times, most recently
from
db09385 to
079d514
Compare
Benchmarks [ tracer ]Benchmark execution time: 2026-05-06 16:45:45 Comparing candidate commit 079d514 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 194 metrics, 0 unstable metrics. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 079d514e06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| list(APPEND STUB_ARGINFO_HEADERS ${_arginfo}) | ||
| endforeach() | ||
| add_custom_target(generate_arginfo DEPENDS ${STUB_ARGINFO_HEADERS}) |
There was a problem hiding this comment.
When a stub changes, a normal cmake --build of the extension will keep compiling the checked-in *_arginfo.h because this custom target is not part of ALL and nothing depends on it; CMake's own help for add_custom_target states that, by default, nothing depends on the custom target. In the context of the new stub workflow, this means edits to tags.stub.php or entity_body.stub.php can silently ship stale reflection/function-entry metadata unless a developer remembers to build generate_arginfo manually. Add a dependency from the extension target (or otherwise list the generated headers as generated sources) so the command runs before compiling the files that include these headers.
Useful? React with 👍 / 👎.
|
Needs to be updated in light of #3857 |
estringana
force-pushed
the
estringana/update-stub
branch
from
079d514 to
4c5389a
Compare
cataphract
force-pushed
the
estringana/update-stub
branch
from
efca45a to
05b4aff
Compare
estringana
force-pushed
the
estringana/update-stub
branch
from
168b7a7 to
c079283
Compare
estringana
force-pushed
the
estringana/update-stub
branch
from
a89bb60 to
b4ec754
Compare
2 participants
Description
Migrate appsec extension to stub-based function registration
Replaces the scattered, hand-maintained arginfo blocks and per-file zend_function_entry tables in ddappsec.c and tags.c with the standard PHP stub workflow.
Changes
Issue where this is reported: #3855