Skip to content

fix: restore StorageFactory legacy env fallback and S3 bucket root#241

Closed
ChiragAgg5k wants to merge 5 commits into
mainfrom
fix-storage-env-fallback
Closed

fix: restore StorageFactory legacy env fallback and S3 bucket root#241
ChiragAgg5k wants to merge 5 commits into
mainfrom
fix-storage-env-fallback

Conversation

@ChiragAgg5k

Copy link
Copy Markdown
Member

What does this PR do?

Restores remote-storage support for self-hosted Appwrite setups that still configure the executor with legacy OPR_EXECUTOR_STORAGE_* env vars while OPR_EXECUTOR_CONNECTION_STORAGE defaults to local://localhost.

After the StorageFactory refactor, those legacy vars were ignored, so custom S3 (and other providers) no longer received the bucket/credentials. This also prepends the bucket to the root path for generic S3 devices (endpoint/host), matching the previous Adapter behavior.

Test Plan

  1. With OPR_EXECUTOR_CONNECTION_STORAGE=local://localhost and:
    • OPR_EXECUTOR_STORAGE_DEVICE=s3
    • OPR_EXECUTOR_STORAGE_S3_* (access key, secret, region, bucket, endpoint)
  2. Confirm StorageFactory::getDevice('/') returns an S3 device whose root includes the bucket
  3. Deploy a function/site artifact and verify the executor can read/write the remote bucket
  4. With a real DSN (s3://key:secret@host/bucket?region=...), confirm bucket is still applied to the S3 root

Related

  • Thread: Executor with custom S3 doesn't work in appwrite 1.9.5 (executor 0.25.1)
  • Appwrite compose still passes OPR_EXECUTOR_STORAGE_* alongside CONNECTION_STORAGE=local://localhost

Appwrite still configures remote storage via OPR_EXECUTOR_STORAGE_* while
CONNECTION_STORAGE defaults to local://localhost. Fall back to the legacy
env vars in that case, and prepend the bucket to the root for generic S3
devices that encode the bucket in the path.

Co-authored-by: Cursor <cursoragent@cursor.com>
@greptile-apps

greptile-apps Bot commented Jul 10, 2026

Copy link
Copy Markdown

Greptile Summary

This PR restores remote-storage support for self-hosted Appwrite setups that ship OPR_EXECUTOR_CONNECTION_STORAGE=local://localhost alongside legacy OPR_EXECUTOR_STORAGE_* env vars. After the StorageFactory refactor those vars were silently ignored, causing S3 (and other providers) to fall back to local storage.

  • Adds shouldUseEnvFallback(): detects an empty or local/file-scheme connection string when OPR_EXECUTOR_STORAGE_DEVICE is set to a remote device, and redirects to the new getDeviceFromEnv() path.
  • Adds getDeviceFromEnv(): reconstructs the correct storage device from the full set of OPR_EXECUTOR_STORAGE_* env vars, with withBucketRoot() applied only for generic-endpoint/host S3 (matching the pre-refactor Adapter behaviour). The DSN-based S3 branches are left untouched, addressing the concern raised in a previous review comment.

Confidence Score: 5/5

Safe to merge — the new fallback path is additive and only activates when OPR_EXECUTOR_CONNECTION_STORAGE is empty or a local/file DSN while a remote OPR_EXECUTOR_STORAGE_DEVICE is configured. The existing DSN-based branch is unchanged.

The change is narrowly scoped to a single factory class. The env fallback triggers only under the specific condition (local/empty connection + non-local device) that was always broken after the refactor. DSN-based paths are untouched. withBucketRoot is applied only in the legacy env path, not in the DSN branch, so existing DSN-based deployments see no behaviour change.

No files require special attention.

Important Files Changed

Filename Overview
src/Executor/StorageFactory.php Adds legacy env-var fallback (OPR_EXECUTOR_STORAGE_*) when CONNECTION_STORAGE is empty or a local/file DSN, and prepends the S3 bucket to the root for generic endpoint/host S3 paths. Logic is consistent with the pre-refactor behaviour.

Reviews (4): Last reviewed commit: "fix: apply Rector new-method-call-withou..." | Re-trigger Greptile

Comment thread src/Executor/StorageFactory.php Outdated
Comment thread src/Executor/StorageFactory.php
ChiragAgg5k and others added 4 commits July 10, 2026 16:54
PHPStan reports getEnv with a default is non-nullable, so ?? was invalid.

Co-authored-by: Cursor <cursoragent@cursor.com>
Limit withBucketRoot to the legacy env path so existing DSN-based S3
configs keep their previous root behavior. Detect local/file DSN schemes
instead of matching a fixed string list.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
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