Skip to content

feat: add the igbinary extension - #108

Merged
loks0n merged 1 commit into
mainfrom
feat/igbinary-extension
Sep 16, 2026
Merged

loks0n merged 1 commit into
mainfrom
feat/igbinary-extension

Conversation

@loks0n

@loks0n loks0n commented Sep 16, 2026

Copy link
Copy Markdown
Member

Summary

  • Compiles and enables igbinary in the production image, alongside the other extensions.
  • Pins igbinary 3.2.17RC1 by commit. It is the first tag that builds on PHP 8.5, and no final 3.2.17 exists yet. The weekly dependency updater only understands x.y.z pins and throws on anything else, so the commit is inlined in the build stage with a comment rather than declared as PHP_IGBINARY_VERSION/PHP_IGBINARY_COMMIT. Once igbinary tags a final release it should move into the catalog like the others.
  • Adds igbinary to the module list in tests.yaml and a changelog entry under 2.1.0.

Why

Appwrite is switching its Redis cache payloads from JSON to igbinary through the new Igbinary codec in utopia-php/cache 5.1.0. On a page of 50 documents igbinary stores about a quarter of the bytes and loads 2.4x faster through Redis. The Appwrite PR depends on this landing and being released as 2.1.0.

Testing

  • composer verify passes (156 tests).
  • I could not compile-check locally because my Docker daemon has a stuck layer for the PHP 8.5 Alpine image, so the image build in this PR's CI is the compile check.

🤖 Generated with Claude Code

Appwrite is moving its Redis cache payloads from JSON to igbinary through
utopia-php/cache 5.1's Igbinary codec, which needs the extension in the
image. igbinary 3.2.17RC1 is the first tag that builds on PHP 8.5 and no
final 3.2.17 exists yet, so the commit is inlined in the build stage
instead of catalogued for the weekly updater, which only understands
x.y.z pins. Move it into the catalog once igbinary tags a final release.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Sep 16, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The image implementation appears sound, but the repository’s explicit behavior-testing requirement must be satisfied before merging.

Fix All in Claude CodeFindings

  1. P2 Module Test Mirrors Configuration
Fix with agent prompt
### Issue 1
tests.yaml:47
Adding `igbinary` to the `php -m` output list only confirms that the Dockerfile enabled the module; it does not exercise serialization or deserialization. This violates the repository directive to test observable behavior instead of mirroring source configuration and could allow a loaded but nonfunctional extension to pass. This requirement must be satisfied before merging, for example with a command test that round-trips a representative value through `igbinary_serialize()` and `igbinary_unserialize()`.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

This PR compiles a commit-pinned PHP 8.5-compatible igbinary extension, installs and enables it in the production image, documents the addition, and extends the container tests.

  • Adds a dedicated native build stage for igbinary.
  • Copies and enables igbinary.so in the final image.
  • Adds a changelog entry and a module-presence assertion.
  • The new assertion should instead exercise an observable serialization round trip.

Reviews (1) · Last reviewed commit: "feat: add the igbinary extension"

Comment thread tests.yaml
- gd
- hash
- iconv
- igbinary

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 Module Test Mirrors Configuration

Adding igbinary to the php -m output list only confirms that the Dockerfile enabled the module; it does not exercise serialization or deserialization. This violates the repository directive to test observable behavior instead of mirroring source configuration and could allow a loaded but nonfunctional extension to pass. This requirement must be satisfied before merging, for example with a command test that round-trips a representative value through igbinary_serialize() and igbinary_unserialize().

Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests.yaml
Line: 47

Comment:
**Module Test Mirrors Configuration**

Adding `igbinary` to the `php -m` output list only confirms that the Dockerfile enabled the module; it does not exercise serialization or deserialization. This violates the repository directive to test observable behavior instead of mirroring source configuration and could allow a loaded but nonfunctional extension to pass. This requirement must be satisfied before merging, for example with a command test that round-trips a representative value through `igbinary_serialize()` and `igbinary_unserialize()`.

**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex

@loks0n

loks0n commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

CI confirms the pin: the image built on amd64 and arm64, and the structure test's php -m check passed with igbinary in the module list. Ready for review; once merged and released as 2.1.0, appwrite/appwrite#13732 can come out of draft.

@loks0n
loks0n merged commit c168fd1 into main Sep 16, 2026
12 checks passed
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