Skip to content

fix: create comments with standard keys and make the comment model configurable - #6

Merged
webard merged 1 commit into
2.xfrom
fix/standard-keys
Sep 25, 2026
Merged

webard merged 1 commit into
2.xfrom
fix/standard-keys

Conversation

@webard

@webard webard commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Migration: it used nullableUuidMorphs('commentable'), so the default forced UUID keys on every commentable model. It now uses nullableMorphs() next to the standard id(), with a comment on how to switch to UUID or ULID morphs.
  • Configurable comment model: new comment_model option in config/filament-comments.php, next to author_model. HasComments reads it through Support\CommentModel::resolve(), which throws a LogicException if the class doesn't extend Models\Comment. An app can extend Comment, add HasUuids, adjust the published migration and point the option to its model.
  • Container binding still works: the documented bind(Comment::class, ...) override keeps working when comment_model isn't set. The config option takes precedence.
  • README:
    • The installation section describes the integer default.
    • The config table has the new option.
    • A new "UUID or ULID keys" section shows the migration changes and the custom model.

Impact on existing installs

The migration is published into the app, so apps that already have the comments table are not affected. Only new installs get integer morphs by default.

Tests

  • New CommentModelTest:
    • the default table has integer id and commentable_id;
    • a comment on an integer-keyed model is stored;
    • with comment_model set to a UuidComment (HasUuids, own table), comments on a UuidPost get UUID keys and the deep link finds them;
    • a model that doesn't extend Comment is refused.
  • Checked that the test catches the regression: with nullableUuidMorphs() put back, it fails.
  • The Post fixture now uses integer keys. The UUID setup is covered by the new fixtures.
  • Container binding: used when the config isn't set; the config wins over a binding.
  • vendor/bin/pest passes (117 tests), and phpstan, pint, rector and composer normalize are clean.

…nfigurable

The migration created the commentable relation with nullableUuidMorphs(),
so every app had to give its commentable models UUID keys or edit the
migration. It now uses nullableMorphs() and a standard auto-incrementing
id; apps with UUID or ULID keys change the published migration.

The comment model is set with the new `comment_model` config option,
read by HasComments. An app can extend Comment, add HasUuids and point
the option to its model. This replaces the undocumented container
binding of the Comment class.

The test fixtures use integer keys for Post, and a UuidPost with a
UuidComment model on its own table covers the UUID setup end to end,
deep links included.
Copilot AI lite review requested due to automatic review settings September 25, 2026 09:13
@webard
webard merged commit 444c8ce into 2.x Sep 25, 2026
27 checks passed
@webard
webard deleted the fix/standard-keys branch September 25, 2026 09:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Documentation inconsistencies and the nullable ID annotation remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Low severity

Open (1)
What changed in this PR

Updates comment storage to use standard integer keys by default and supports configurable UUID/ULID-compatible comment models.

Changes:

  • Adds validated comment_model configuration.
  • Replaces UUID morphs with standard morphs.
  • Removes the undocumented container binding.
  • Adds fixtures, tests, and documentation for custom key types.
File Summary
tests/​Fixtures/​Models/​UuidPost.php Adds a UUID commentable fixture.
tests/​Fixtures/​Models/​UuidComment.php Adds a configurable UUID comment fixture.
tests/​Fixtures/​Models/​Post.php Switches the fixture to integer keys.
tests/​Fixtures/​database/​migrations/​0001_01_01_000000_create_fixture_tables.php Adds integer and UUID fixture tables.
tests/​Feature/​CommentModelTest.php Tests integer and UUID comment models.
src/​Support/​CommentModel.php Resolves and validates the configured model.
src/​Models/​Comment.php Updates the commentable ID documentation.
src/​FilamentCommentsServiceProvider.php Removes the container binding.
src/​Concerns/​HasComments.php Uses the configurable comment model.
README.md Documents key configuration and custom models.
database/​migrations/​2026_03_10_100000_create_fm_comments_table.php Uses standard morph keys.
config/​filament-comments.php Adds the comment_model option.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread README.md
* Model of comments. Must extend Happenv\FilamentComments\Models\Comment,
* e.g. to add HasUuids together with a matching migration.
*/
'comment_model' => null,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, and it went further than stale docs: that section documented the container binding, so removing it would have broken apps that use it. Fixed in 841ccd0:

  • CommentModel::resolve() reads comment_model first and falls back to the class the container binds to Comment. The bindIf is back.
  • The "Comment model" section now leads with the config option and keeps bind() as the older way that still works.
  • Two new tests cover it: a container binding is used when the config isn't set, and the config wins over a binding.

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