Skip to content

Conversation

@neverything
Copy link
Contributor

tl;dr Adds more tests and changes some tests to cover Postgres support. Explicit orderBy('type') for Tag::getTypes()

Note

The package does already work with Postgres. I must have either used a very old version or had something else wrong in my custom queries. See discussion here: #532

Changes in this PR:

  • Add MySQL 8, PgSQL 16 and PgSQL 17 to the run-tests workflow
  • Change scripts in composer.json to allow running tests for mysql and pgsql drivers
  • Changes tests that return TestModel (the one using HasTags) from toEqual() to toContain(), because the returned sorting in pgsql is different
  • Adds more tests for the tag order_column
  • Adds more tests to check withAnyTags and withAllTags that work with both name and slug of the tags
  • ⚠️ (only change in package) adds explicit orderBy('type') to Tag::getTypes() to satisfy the different order in pgsql

If the order from Tag::getTypes() doesn't matter, we could remove this change and alter the test in https://github.com/spatie/laravel-tags/blob/main/tests/TagTest.php#L175-L186 to not expect an explicit order.

Happy to make changes if needed.

- adds mysql8 and pgsql16 & pgsql17 tests

- additional tests for slugs and order_column

- adds explicit orderBy to getTypes in Tag.php
@freekmurze
Copy link
Member

The package does already work with Postgres.

Should we still review this PR, or should it be closed?

@neverything
Copy link
Contributor Author

The package does already work with Postgres.

Should we still review this PR, or should it be closed?

@freekmurze only if you want to expand the tests to be run on MySQL 8, PgSQL 16 & 17 too imho. To me, it's fine either way.

@freekmurze
Copy link
Member

Always nice to have more tests, and it all looks good. Thank you!

@freekmurze freekmurze merged commit 2b36cb3 into spatie:main Mar 5, 2025
73 checks passed
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