Skip to content

Try: Reduce the C API to just phpwasm_run #106

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 3 commits into from

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Jan 9, 2023

These explorations reduce the surface of the C API from ["_phpwasm_init_context", "_phpwasm_destroy_context", "_phpwasm_run", "_phpwasm_refresh", "_phpwasm_init_uploaded_files_hash", "_phpwasm_register_uploaded_file", "_phpwasm_destroy_uploaded_files_hash"] to just ["_phpwasm_run"]. This PR is draft-y, exploratory, and isn't meant for merging. It's just my way of sharing some thoughts in public.

The changes proposed mostly work, however this problem is blocking:

018c5aa6:0x48482c Uncaught (in promise) RuntimeError: memory access out of bounds
    at dlfree (018c5aa6:0x48482c)
    at zend_hash_destroy (018c5aa6:0x2700c2)
    at destroy_uploaded_files_hash (018c5aa6:0x1226b9)
    at sapi_deactivate (018c5aa6:0x1250a2)
    at invoke_v (php-8.0.js?159776a7a8ad4cc94cd1fc412c1de07ac89eb96850a44a8c81a9fb25fb4e1b0c:6865:29)
    at php_request_shutdown (018c5aa6:0x1aeaf1)
    at php_embed_shutdown (018c5aa6:0x276e89)
    at invoke_v (php-8.0.js?159776a7a8ad4cc94cd1fc412c1de07ac89eb96850a44a8c81a9fb25fb4e1b0c:6865:29)
    at phpwasm_run (018c5aa6:0x2eac19)
    at Object.ccall (php-8.0.js?159776a7a8ad4cc94cd1fc412c1de07ac89eb96850a44a8c81a9fb25fb4e1b0c:6350:22)

More prominently, there's a problem of shaping the API:

  • Parsing the request in PHP would mean reimplementing methods already provided by the browser, and deriving again the same informatino JS already has readily avialable.
  • Not parsing the request in PHP means JavaScript needs to re-serialize the parsed raw data to populate php://input. JS could reuse the raw request data, but that would potentially process large multipart-encoded files twice.

I'm not sure what's the best way to proceed yet.

@adamziel
Copy link
Collaborator Author

Closing in favor of #107

@adamziel adamziel closed this Jan 12, 2023
bgrgicak added a commit that referenced this pull request May 7, 2025
## Motivation for the change, related issues

See https://github.com/Automattic/studio/security/dependabot/40

## Implementation details

I propose to upgrade octokit from 3.1.1 to 3.1.2.

## Testing Instructions (or ideally a Blueprint)

- Export to GitHub using the site manager
- Import from GitHub using the site manager

---------

Co-authored-by: Brandon Payton <[email protected]>
Co-authored-by: Bero <[email protected]>
bgrgicak added a commit that referenced this pull request May 7, 2025
## Motivation for the change, related issues

We have a lot of dependencies which makes upgrading difficult. See #106
for more context. This PR removes the ones that seemed unused and moves
a few entries over to the devDependencies list.

After this PR, we could see which deps give us the most headache and
look into removing them in one way or another.

## Testing Instructions (or ideally a Blueprint)

Cross your fingers and wait for CI.

---------

Co-authored-by: Brandon Payton <[email protected]>
Co-authored-by: Wojtek Naruniec <[email protected]>
Co-authored-by: Bero <[email protected]>
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.

1 participant