Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix summarizer connection resolver on non-default connections #15407

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

saulens22
Copy link
Contributor

Description

Laravel model method resolveConnection, when called without arguments, returns default database connection, even if model has another connection set.

Usually it doesn't matter if syntax is the same between connections but in our case it was returning MariaDbConnection instead of SqlServerConnection, which has different char escaping.

Laravel itself manually passes connection name to resolveConnection like here: https://github.com/laravel/framework/blob/6e0e2cbd584a2988348409fe15bb2ead87aeafec/src/Illuminate/Database/Eloquent/Model.php#L1817

I believe it is better to use getConnection as it sets connection properly as in example above.

Visual changes

Functional changes

  • Code style has been fixed by running the composer cs command.
  • Changes have been tested to not break existing functionality.
  • Documentation is up-to-date.

@danharrin
Copy link
Member

Hey @eele94, in #15293 you ensured that summarisers were compatible with Sushi models. Does this PR break the compatibility again?

@danharrin danharrin added the bug Something isn't working label Jan 23, 2025
@danharrin danharrin added this to the v3 milestone Jan 23, 2025
@saulens22
Copy link
Contributor Author

I checked Sushi model file, and it doesn't have getConnection function, so it breaks.

I have changed the lines to $query->getModel()->resolveConnection($query->getModel()->getConnectionName()) as these methods exist on both Laravel and Sushi

@eele94
Copy link
Contributor

eele94 commented Jan 23, 2025

@danharrin thanks for mentioning. I believe @saulens22 has found the solution😃

@danharrin danharrin merged commit 635933b into filamentphp:3.x Jan 23, 2025
10 checks passed
@danharrin
Copy link
Member

Thanks

@saulens22 saulens22 deleted the fix-connection-summarizer branch January 24, 2025 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants