fix(teams): declare membershipId after the existing createMembership params by ChiragAgg5k · Pull Request #14423 · appwrite/appwrite · GitHub
Skip to content

fix(teams): declare membershipId after the existing createMembership params - #14423

Merged
ChiragAgg5k merged 2 commits into
mainfrom
fix/membership-id-last-param
Oct 13, 2026
Merged

fix(teams): declare membershipId after the existing createMembership params#14423
ChiragAgg5k merged 2 commits into
mainfrom
fix/membership-id-last-param

Conversation

@ChiragAgg5k

@ChiragAgg5k ChiragAgg5k commented Oct 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

SDKs build positional arguments from the declared param order. membershipId (added in e4d7aef, not yet in a released SDK) was declared second, so a positional call shifts:

teams.createMembership(teamId, roles, email)
// before: email → membershipId
// after:  membershipId is the last optional param, existing calls unchanged

Moved the ->param() after name and the action() argument to match (route values are passed positionally).

Closes #14425

@tenki-reviewer

tenki-reviewer Bot commented Oct 13, 2026 •
edited
Loading

Copy link
Copy Markdown

@hansi-codes

hansi-codes Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

🔵 Tier A · Mergeable after minor fixes

The code change is correct, but the added unit test directly tests an HTTP route action contrary to the repository testing rule.

Moves membershipId after the existing membership parameters and aligns the action callback signature with that order. Adds a regression check for the registered parameter and callback argument ordering.

Verdict New comments Fixed Still open
✅ Approved 1 0 0
Finding Where
🟡 Keep route-action checks out of unit tests tests/unit/Platform/Modules/Teams/Http/Memberships/CreateTest.php:21
Fix with agent prompt 
### Issue 1
tests/unit/Platform/Modules/Teams/Http/Memberships/CreateTest.php:21-22
**Keep route-action checks out of unit tests**

This directly instantiates `Create` and inspects its HTTP action parameter metadata and callback signature. `AGENTS.md` says not to unit-test HTTP route actions and reserves this surface for E2E; please keep this regression check at the SDK/API contract layer instead.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
📂 Walkthrough · 2
File Change
src/Appwrite/Platform/Modules/Teams/Http/Memberships/Create.php Moves the optional membership ID parameter after existing create-membership parameters and updates the callback signature.
tests/unit/Platform/Modules/Teams/Http/Memberships/CreateTest.php Adds a test that pins parameter registration and callback argument order.

Reviewed 5fed332 · Details · Comment @hansi-codes review to re-run, or mention @hansi-codes with a question.

hansi-codes[bot]
hansi-codes Bot approved these changes Oct 13, 2026
View reviewed changes

@hansi-codes hansi-codes Bot left a comment

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.

🔵 Tier A · Looks good to merge. Summary

Comment thread tests/unit/Platform/Modules/Teams/Http/Memberships/CreateTest.php Outdated
Comment on lines +21 to +22
$action = new Create();
$params = \array_keys($action->getParams());

@hansi-codes hansi-codes Bot Oct 13, 2026

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.

Keep route-action checks out of unit tests

This directly instantiates Create and inspects its HTTP action parameter metadata and callback signature. AGENTS.md says not to unit-test HTTP route actions and reserves this surface for E2E; please keep this regression check at the SDK/API contract layer instead.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: tests/unit/Platform/Modules/Teams/Http/Memberships/CreateTest.php
Line: 21-22

Comment:
**Keep route-action checks out of unit tests**

This directly instantiates `Create` and inspects its HTTP action parameter metadata and callback signature. `AGENTS.md` says not to unit-test HTTP route actions and reserves this surface for E2E; please keep this regression check at the SDK/API contract layer instead.

---

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

🟡 Minor · testing · Reply if this doesn't apply.

@ChiragAgg5k ChiragAgg5k mentioned this pull request Oct 13, 2026
Closed
2 tasks
@ChiragAgg5k ChiragAgg5k self-assigned this Oct 13, 2026
@github-actions

github-actions Bot commented Oct 13, 2026

Copy link
Copy Markdown

Security rules

No new WARNING or ERROR findings from security rules.

19 existing findings tracked in .semgrep/baseline.json
  • php.appwrite.guest-write-without-abuse-limit (12)
  • php.appwrite.secret-compare-timing (5)
  • php.appwrite.weak-secret-env-default (2)

Posted by Checks / Rules. Re-runs update this comment in place. Rule details and baseline: .semgrep/README.md.

@ChiragAgg5k
ChiragAgg5k merged commit 9195620 into main Oct 13, 2026
32 checks passed
@ChiragAgg5k
ChiragAgg5k deleted the fix/membership-id-last-param branch October 13, 2026 10:44
@github-actions

github-actions Bot commented Oct 13, 2026

Copy link
Copy Markdown

✨ Benchmark results

Comparing main (before) → fix/membership-id-last-param (after).

Metric Before After Change
🚀 Requests/sec 172.98 163.25 🔴 -5.6%
⏱️ Latency P50 99.78 ms 106.91 ms 🔴 +7.1%
⏱️ Latency P95 230.08 ms 241.59 ms ⚪ +5%
Per-scenario breakdown & investigation details

Metrics below reflect the current branch (after). Δ P95 compares against the base.

Scenario P50 (ms) P95 (ms) Requests RPS Δ P95 (ms)
API total 106.91 241.59 10,260 163.25 +11.5
Account 204.04 353.89 540 9.29 -14.6
TablesDB 103.27 180.86 5,580 90.87 +10.63
Storage 101.44 208.53 2,700 45.76 +6.81
Functions 160.33 285.42 1,440 25.13 +5.59

Top API waits (after)

API request Max wait (ms)
account.name.update 549.75
tablesdb.rows.increment 456.2
functions.variables.update 427.5
account.prefs.update 413.89
functions.variables.create 406.22

This was referenced Oct 13, 2026
Merged
Merged
Merged
Merged
Closed
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hansi-codes hansi-codes[bot] hansi-codes[bot] approved these changes

Assignees

@ChiragAgg5k ChiragAgg5k

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Declare membershipId after the existing createMembership params

1 participant

@ChiragAgg5k