fix: platform defaults by loks0n · Pull Request #10957 · appwrite/appwrite

loks0n · 2025-12-15T15:01:41Z

What does this PR do?

(Provide a description of what this PR does and why it's needed.)

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)

Related PRs and Issues

(Related PR or issue)

Checklist

Have you read the Contributing Guidelines on issues?
If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

coderabbitai · 2025-12-15T15:02:03Z

📝 Walkthrough

Walkthrough

This pull request applies a consistent fallback mechanism for the platform parameter across four event classes in the Appwrite framework: Build, Func, Mail, and Migration. When the $this->platform property is empty, the code now retrieves the platform value from the application configuration using Config::getParam('platform', []). The necessary Utopia\Config\Config import is added to files that require it. The payload structure in each class is updated to use the resolved platform value instead of directly accessing the property.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Verify that the Config::getParam('platform', []) method signature and usage are correct across all affected files
Confirm the fallback logic is consistently applied and maintains backward compatibility in Build, Func, Mail, and Migration classes
Ensure the import statement is properly placed and does not create any circular dependencies

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description contains only the PR template with no actual implementation details filled in; it does not describe any part of the changeset.	Fill in the template sections, particularly 'What does this PR do?' and 'Test Plan', with specific details about the platform defaults implementation.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: platform defaults' directly summarizes the main change across all modified files, which introduce platform fallbacks from Config.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix-platform-defaults

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2025-12-15T15:04:21Z

Security Scan Results for PR

Docker Image Scan Results

Package	Version	Vulnerability	Severity
libpng	1.6.51-r0	CVE-2025-66293	HIGH
libpng-dev	1.6.51-r0	CVE-2025-66293	HIGH

Source Code Scan Results

🎉 No vulnerabilities found!

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/Appwrite/Event/Func.php (1)
5-5: LGTM! Consider refactoring the duplicated pattern.

The platform fallback logic is correctly implemented and consistent with Build, Migration, and Mail event classes.

Since this exact pattern (lines 206-209) is now duplicated across four event classes, consider refactoring it into a protected helper method in the parent Event class:
// In Event.php
protected function resolvePlatform(): array
{
    if (empty($this->platform)) {
        return Config::getParam('platform', []);
    }
    return $this->platform;
}
Then in each child class:
-$platform = $this->platform;
-if (empty($platform)) {
-    $platform = Config::getParam('platform', []);
-}
+$platform = $this->resolvePlatform();
This would reduce duplication and make future changes to the platform resolution logic easier to maintain.

Also applies to: 206-209, 226-226

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e9d9aa6 and 7e6846f.

📒 Files selected for processing (4)

src/Appwrite/Event/Build.php (2 hunks)
src/Appwrite/Event/Func.php (3 hunks)
src/Appwrite/Event/Mail.php (3 hunks)
src/Appwrite/Event/Migration.php (2 hunks)

🧰 Additional context used

🧠 Learnings (3)

📚 Learning: 2025-08-22T06:23:32.026Z

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In Appwrite's codebase, Utopia\Queue\Broker\Pool (aliased as BrokerPool) is a readonly class that implements both Publisher and Consumer interfaces, allowing it to be used directly in constructors expecting either interface type. This is extensively used throughout the codebase for queue operations.

Applied to files:

src/Appwrite/Event/Build.php
src/Appwrite/Event/Migration.php
src/Appwrite/Event/Func.php

📚 Learning: 2025-08-22T06:23:32.026Z

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: The Utopia\Queue\Broker\Pool class (aliased as BrokerPool) implements both Publisher and Consumer interfaces, making it directly compatible with constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher). No wrapping is needed.

Applied to files:

src/Appwrite/Event/Build.php
src/Appwrite/Event/Migration.php
src/Appwrite/Event/Func.php

📚 Learning: 2025-08-22T06:23:32.026Z

Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In the Utopia Queue system, the Pool class implements both Publisher and Consumer interfaces, so BrokerPool (Pool) can be passed directly to constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher).

Applied to files:

src/Appwrite/Event/Build.php
src/Appwrite/Event/Migration.php
src/Appwrite/Event/Func.php

🧬 Code graph analysis (2)

src/Appwrite/Event/Func.php (1)

src/Appwrite/Event/Event.php (1)

getParam (319-322)

src/Appwrite/Event/Mail.php (1)

src/Appwrite/Event/Event.php (3)

Event (10-678)

generateEvents (492-604)

getEvent (131-134)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: Setup & Build Appwrite Image
GitHub Check: Setup & Build Appwrite Image
GitHub Check: scan

🔇 Additional comments (3)

src/Appwrite/Event/Build.php (1)

5-5: LGTM! Clean platform fallback implementation.

The addition of a Config fallback for the platform parameter ensures that the payload always includes a valid platform value, improving consistency across event handling.

Also applies to: 114-117, 125-125

src/Appwrite/Event/Migration.php (1)

5-5: LGTM! Consistent implementation.

The platform fallback logic matches the pattern used in other event classes and ensures the migration payload always includes platform information.

Also applies to: 77-80, 86-86

src/Appwrite/Event/Mail.php (1)

5-5: LGTM! Consistent implementation with good formatting.

The platform fallback logic is correctly implemented, and the trailing comma addition on line 537 improves code maintainability for future modifications.

Also applies to: 520-523, 537-538

github-actions · 2025-12-15T15:12:53Z

✨ Benchmark results

Requests per second: 1,127
Requests with 200 status code: 202,932
P99 latency: 0.17336445

⚡ Benchmark Comparison

Metric	This PR	Latest version
RPS	1,127	1,170
200	202,932	210,599
P99	0.17336445	0.174726173

fix: platform defaults

7e6846f

coderabbitai bot reviewed Dec 15, 2025

View reviewed changes

fogelito approved these changes Dec 15, 2025

View reviewed changes

levivannoort approved these changes Dec 15, 2025

View reviewed changes

loks0n merged commit 2a61cac into 1.8.x Dec 15, 2025
42 of 44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: platform defaults#10957

fix: platform defaults#10957
loks0n merged 1 commit into1.8.xfrom
fix-platform-defaults

loks0n commented Dec 15, 2025

Uh oh!

coderabbitai bot commented Dec 15, 2025 •

edited

Loading

Walkthrough

Estimated code review effort

Uh oh!

github-actions bot commented Dec 15, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

github-actions bot commented Dec 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

loks0n commented Dec 15, 2025

What does this PR do?

Test Plan

Related PRs and Issues

Checklist

Uh oh!

coderabbitai bot commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

github-actions bot commented Dec 15, 2025

Security Scan Results for PR

Docker Image Scan Results

Source Code Scan Results

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 15, 2025

✨ Benchmark results

⚡ Benchmark Comparison

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Dec 15, 2025 •

edited

Loading