Skip to content

Conversation

@lohanidamodar
Copy link
Contributor

@lohanidamodar lohanidamodar commented Dec 19, 2025

Summary by CodeRabbit

  • Refactor
    • Expanded the SQL Adapter API surface by making previously internal utility methods publicly accessible. This change enables third-party integrations and extensions to access collection and attribute management functionality directly, supporting more flexible extension and customization of the audit system without behavioral changes.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

Walkthrough

Five protected methods in the SQL class within src/Audit/Adapter/SQL.php have been changed to public visibility: getCollectionName(), getAttributes(), getAttributeDocuments(), getIndexes(), and getIndexDocuments(). The methods retain their original return types and implementation logic; only the access modifier has been broadened from protected to public. No changes to control flow, error handling, or behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Single file with 5 homogeneous visibility changes
  • No logic modifications or behavioral alterations
  • Review should focus on verifying the scope of access level changes and confirming these methods are intended for public use

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: converting protected methods to public in SQL.php, which is clear and specific.
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 feat-public-attributes

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/Audit/Adapter/SQL.php (1)

24-176: Consider documenting the public API design rationale.

The selective exposure of methods (making collection-level getters public while keeping singular/implementation methods protected) suggests a thoughtful design. Consider adding class-level documentation explaining:

  • When external consumers should use these public methods
  • The intended use cases for schema inspection
  • Why certain methods remain protected (implementation details vs. public API)

This will help maintainers understand the design intent and prevent unintended future changes.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bac717c and adaf99a.

📒 Files selected for processing (1)
  • src/Audit/Adapter/SQL.php (5 hunks)
🔇 Additional comments (2)
src/Audit/Adapter/SQL.php (2)

142-166: Methods are safe to expose publicly - no subclass overrides exist.

These methods are implementation-specific to the SQL adapter and are not part of the abstract Adapter base class. No subclasses override these methods, so making them public poses no compatibility concerns.


24-27: Schema methods are now part of the public API.

Making getCollectionName(), getAttributes(), getAttributeDocuments(), getIndexes(), and getIndexDocuments() public expands the API surface to allow external inspection of audit schema structure. Concrete subclasses do not override these methods, so the visibility change is compatible with existing implementations.

@lohanidamodar
Copy link
Contributor Author

closed in favor of
#96

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.

2 participants