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

[12.x] Fix accessing Connection property in Grammar classes #54487

Merged

Conversation

hafezdivandari
Copy link
Contributor

@hafezdivandari hafezdivandari commented Feb 6, 2025

The Illuminate\Database\Grammar class has required a Connection $connection property since Laravel 10.x (via #46558), but it was passed via setConnection() method to avoid signature changes in a patch release. Since then, many methods in the Grammar classes have needed access to the Connection property (mainly to retrieve config properties and compare driver versions). This PR does:

  • Adding Connection $connection to the Grammar constructor, instead of calling setConnection() when instantiating.
  • Fixing table prefix issues by using Connection::getTablePrefix() instead of having the $prefix property in the Grammar and Blueprint classes.

P.S. 1: This PR doesn’t require any upgrade guide for end-users.

P.S. 2: The $connection property was added to the Blueprint constructor in Laravel 12.x via #51821, so now is a good time to fix it in the Grammar class as well.

Summary

  • Adds missing prefix_indexes config property to sqlite connection to be consistent with other DB driver connections.
  • Adds Connection $connection property to Illuminate/Database/Grammar::__construct() method.
  • Removes Illuminate\Database\Connection::withTablePrefix() method.
  • Removes Illuminate\Database\Grammar::setConnection() method.
  • Removes Illuminate\Database\Schema\Builder::setConnection() method.
  • Removes $prefix property from Illuminate\Database\Schema\Blueprint::__construct() method.
  • Deprecates Illuminate\Database\Grammar::getTablePrefix() method (Use Illuminate\Database\Connection::getTablePrefix() instead).
  • Deprecates Illuminate\Database\Grammar::setTablePrefix() method (Use Illuminate\Database\Connection::setTablePrefix() instead).
  • Deprecates Illuminate\Database\Schema\Blueprint::getPrefix() method (Use Illuminate\Database\Connection::getTablePrefix() instead).
  • Fix a bug on wrapping aliased tables with prefix.
  • Fix a bug where table prefix was totally ignored on Blueprint when prefix_indexes is set to false.
  • Fix a bug on defining starting value for a column of a table with non-default schema on PostgreSQL.
  • Moves overridden createDatabase() and dropDatabaseIfExists() methods to Illuminate\Database\Schema\Builder parent class (The same method was overridden on all inherited classes.)

Copy link

github-actions bot commented Feb 6, 2025

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@hafezdivandari hafezdivandari marked this pull request as ready for review February 6, 2025 03:32
@tpetry
Copy link
Contributor

tpetry commented Feb 7, 2025

I've implemented passing Connection as a parameter and not a constructor value to simplify work for package maintainers. There are many packages that extend the different Eloquent drivers to add some stuff which is not available in Laravel.

Moving the Connection to the constructor makes writing packages that target multiple Laravel versions impossible. Thats why I did it that way - and would prefer it being kept like that 😉 My PG driver for example is compatible back to Laravel 6 (!) because of that design.

@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented Feb 7, 2025

@tpetry Thank you for reviewing and dislike😅. But this PR isn't just a styling fix, accessing the connection on Grammar classes needs to be addressed sooner or later IMHO. However:

  1. As I mentioned in the PR description, passing $connection to the Blueprint constructor has been merged in [12.x] feat: configure default datetime precision on per-grammar basis #51821. So, the signature changes are already unavoidable?

  2. The current implemented way of passing $connection to Grammar classes is problematic, leading to hidden bugs and decoupling issues. For example, this PR addresses issues like accessing/manipulating the table prefix. Here's a sample from your package (amazing work by the way): laravel-postgresql-enhanced/src/Schema/Grammars/GrammarTypes.php#L20-L23

  3. This change helps avoid BC breaks in the future. For example, the intention of [12.x] feat: configure default datetime precision on per-grammar basis #51821 was a small change, but needing access to Connection made it large and hard to review, it could have been done in a patch release!

  4. Code style fixers complain about accessing $this->connection in Grammar classes, as the property may be null.

@taylorotwell
Copy link
Member

@tpetry any further comments based on above? 👆

@taylorotwell taylorotwell marked this pull request as draft February 11, 2025 15:50
@tpetry
Copy link
Contributor

tpetry commented Feb 11, 2025

Not a big fan, but the package authors will find a way to make it work with the bc break here. And with the blueprint changes already merged (which I didn't see at that time) it'll already be some work. So break more stuff if it really simplifies some stuff in the future.

@hafezdivandari hafezdivandari marked this pull request as ready for review February 11, 2025 16:07
@taylorotwell taylorotwell merged commit c78fa3d into laravel:master Feb 12, 2025
27 checks passed
@hafezdivandari hafezdivandari deleted the master-fix-connection-references branch February 12, 2025 17:07
GromNaN added a commit to GromNaN/laravel-mongodb-fork that referenced this pull request Feb 24, 2025
GromNaN added a commit to GromNaN/laravel-mongodb-fork that referenced this pull request Feb 24, 2025
GromNaN added a commit to GromNaN/laravel-mongodb-fork that referenced this pull request Feb 26, 2025
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.

4 participants