Skip to content

Generate appsec php stub with all exposed function by extension#3858

Open
estringana wants to merge 2 commits intomasterfrom
estringana/update-stub
Open

Generate appsec php stub with all exposed function by extension#3858
estringana wants to merge 2 commits intomasterfrom
estringana/update-stub

Conversation

@estringana
Copy link
Copy Markdown
Contributor

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

  • Add ddappsec.stub.php as the single source of truth for the public datadog\appsec PHP API, with proper typed signatures for all 11 exposed functions.
  • Generate ddappsec_arginfo.h from the stub using PHP's gen_stub.php tool; this replaces all hand-written ZEND_BEGIN_ARG_* / ZEND_END_ARG_* blocks and consolidates function registration into a single ext_functions[] table.
  • Wire stub regeneration into CMake so ddappsec_arginfo.h is automatically rebuilt whenever ddappsec.stub.php changes.

Issue where this is reported: #3855

@datadog-datadog-prod-us1-2
Copy link
Copy Markdown

datadog-datadog-prod-us1-2 Bot commented May 6, 2026

Tests

Fix all issues with BitsAI or with Cursor

⚠️ Warnings

🧪 1 Test failed

Login failure automated event() from com.datadog.appsec.php.integration.Laravel8xTests   View in Datadog   (Fix with Cursor)
Assertion failed: 

assert span.meta."appsec.events.users.login.failure.track" == "true"
       |    |    |                                         |
       |    |    null                                      false
       |    ['_dd.appsec.s.req.cookies':'[[],{"len":0}]', '_dd.appsec.s.req.body':'[[],{"len":0}]', '_dd.p.dm':'-1', 'http.url':'http://docker:32772/authenticate?email=nonExisiting@email.com', 'env':'integration', 'laravel.route.name':'unnamed_route', 'http.client_ip':'172.17.0.1', 'http.status_code':'403', 'http.request.headers.user-agent':'Java-http-client/17.0.18', 'component':'laravel', 'http.response.headers.content-type':'text/html; charset=UTF-8', '_dd.appsec.event_rules.version':'1.4.3', '_dd.p.ksr':'1', 'span.kind':'server', '_dd.appsec.helper_runtime':'rust', 'http.route':'authenticate', '_dd.runtime_family':'php', 'runtime-id':'389c3e39-eef8-44a3-a7cd-c1402df1cc2e', '_dd.appsec.s.req.params':'[[[8]],{"len":1}]', 'http.method':'GET', '_dd.appsec.s.req.query':'[{"email":[8]}]', 'http.useragent':'Java-http-client/17.0.18', 'laravel.route.action':'\\App\\Http\\Controllers\\LoginController@authenticate']
       com.datadog.appsec.php.model.Span@6fa2a006

Assertion failed: 

...

ℹ️ Info

No other issues found (see more)

❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 60.68% (+0.04%)

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 079d514 | Docs | Datadog PR Page | Give us feedback!

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented May 6, 2026

Benchmarks [ appsec ]

Benchmark execution time: 2026-05-06 16:07:39

Comparing candidate commit 079d514 in PR branch estringana/update-stub with baseline commit 8ff00ed in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics.

Copy link
Copy Markdown
Contributor

@cataphract cataphract left a comment

Choose a reason for hiding this comment

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

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.

Comment thread appsec/cmake/extension.cmake
Comment thread appsec/cmake/extension.cmake Outdated
Comment thread appsec/cmake/extension.cmake Outdated
Comment on lines +25 to +26
${CMAKE_CURRENT_SOURCE_DIR}/src/extension/ddappsec.stub.php
${CMAKE_CURRENT_SOURCE_DIR}/src/extension/entity_body.stub.php
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doesn't make sense to have one file that's specific to one compilation unit and then one named "ddappsec.stub.php". The one you created is specific to tags.c, so you can named it tags.stub.php.

Comment thread appsec/cmake/extension.cmake
Comment thread appsec/src/extension/ddappsec.c Outdated
}

static PHP_FUNCTION(datadog_appsec_is_enabled)
PHP_FUNCTION(datadog_appsec_is_enabled)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

these can stay static

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

but they will be references from outside

Comment thread appsec/src/extension/ddappsec.c Outdated
@estringana estringana force-pushed the estringana/update-stub branch 2 times, most recently from 712d21e to 2cae42e Compare May 6, 2026 14:17
@estringana estringana force-pushed the estringana/update-stub branch from 2cae42e to 8e0f7dc Compare May 6, 2026 14:21
@estringana estringana force-pushed the estringana/update-stub branch from db09385 to 079d514 Compare May 6, 2026 15:26
@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented May 6, 2026

Benchmarks [ tracer ]

Benchmark execution time: 2026-05-06 16:45:45

Comparing candidate commit 079d514 in PR branch estringana/update-stub with baseline commit 8ff00ed in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 194 metrics, 0 unstable metrics.

@estringana estringana marked this pull request as ready for review May 7, 2026 08:36
@estringana estringana requested a review from a team as a code owner May 7, 2026 08:36
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 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})
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Hook stub generation into the extension build

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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants