HP-2667: Do not prevent exception when name is empty

[bladeroot]

Summary by CodeRabbit

• Bug Fixes
  • Prevented invalid file-path operations when non-string or empty names are supplied.
  • Existence checks and deletions now short-circuit for invalid/missing inputs to avoid errors.
  • Reduced redundant path computations for slightly improved reliability and efficiency.
  • No changes to public API signatures.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Added input validation to src/FileStorage.php: getFullPath() returns null for non-string/empty names; has() and remove() compute the full path once and short-circuit when that result is null.

Changes

Cohort / File(s)	Summary
FileStorage guards src/FileStorage.php	getFullPath() now returns null for non-string or empty names. has() computes the full path once and returns false if null; otherwise checks file_exists(). remove() computes the full path once and returns true if null; otherwise calls unlink() on the computed path.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant FileStorage
    Note right of FileStorage: New/changed behavior: getFullPath may return null

    Caller->>FileStorage: getFullPath(name)
    alt name is empty or non-string
        FileStorage-->>Caller: null
        Caller->>FileStorage: has(name)
        FileStorage-->>Caller: false
        Caller->>FileStorage: remove(name)
        FileStorage-->>Caller: true
    else valid name
        FileStorage-->>Caller: fullPath
        Caller->>FileStorage: has(name)
        FileStorage-->>Caller: file_exists(fullPath) result
        Caller->>FileStorage: remove(name)
        FileStorage-->>Caller: unlink(fullPath) result
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

• Single-file, small defensive changes.
• Spot-check:
  • getFullPath() semantics for non-string vs. empty string.
  • Call sites expecting previous non-null returns and behavior of remove() returning true when path is null.

Poem

A rabbit peeks where paths begin,
It sniffs each name before a spin.
Empty trails — it gently stays,
Safe hops on well-formed ways.
🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the specific issue being addressed: handling empty names in the FileStorage class to allow exceptions rather than preventing them.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 24c21a3 and 5801629.

📒 Files selected for processing (1)

• src/FileStorage.php (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/FileStorage.php

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/FileStorage.php (2)

30-37: Reuse $path in has() and align null-handling with get()

has() computes $path and then ignores it, calling getFullPath($name) again. Also, get() doesn’t short‑circuit on null like has()/remove(), which means it still calls is_file(null) for empty names. Reusing $path and adding the same null guard in get() keeps behaviour consistent and avoids redundant work and warnings.

     public function has($name)
     {
-        $path = $this->getFullPath($name);
-        if ($path === null) {
-            return false;
-        }
-        return is_file($this->getFullPath($name));
+        $path = $this->getFullPath($name);
+        if ($path === null) {
+            return false;
+        }
+
+        return is_file($path);
     }
 
     public function get($name)
     {
-        $path = $this->getFullPath($name);
-
-        return is_file($path) ? file_get_contents($this->getFullPath($name)) : null;
+        $path = $this->getFullPath($name);
+        if ($path === null) {
+            return null;
+        }
+
+        return is_file($path) ? file_get_contents($path) : null;
     }

Also applies to: 39-44

---

74-83: remove() returning true for empty/invalid names may hide bugs

With the new guard, remove(null) / remove('') now returns true, signalling success even though nothing could be deleted and the input is arguably invalid. That can mask caller bugs and seems at odds with the idea of not hiding exceptional situations for empty names.

You might want either to fail (e.g. throw InvalidArgumentException) or at least return false so callers can detect the problem.

Example options:

-        if ($path === null) {
-            return true;
-        }
+        if ($path === null) {
+            return false;
+        }

or, if you prefer to treat this as programmer error:

-        if ($path === null) {
-            return true;
-        }
+        if ($path === null) {
+            throw new \InvalidArgumentException('Name cannot be empty');
+        }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f9f52f and cdc764e.

📒 Files selected for processing (1)

• src/FileStorage.php (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/FileStorage.php (1)

src/StorageInterface.php (1)

• has (15-15)

[VadymHrechukha]

It is hard to read

if (is_string($name)  && !empty(name)) {
   return $this->path . '/' . $name[0] . '/' . $name;
}