Increase delivery_instructions column length in address-related tables#2247
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughA new migration updates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5-10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/core/database/migrations/2025_08_05_083800_update_delivery_instructions_columns.php (1)
🔇 Additional comments (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/database/migrations/2025_07_18_081937_update_delivery_instructions_columns.php (2)
14-24: Condense repetitiveSchema::table()calls & honour custom connectionThe three almost-identical blocks are classic duplication.
Wrapping the table names in an array and iterating keeps the intent crystal-clear, prevents copy-paste drift, and makes future changes (e.g. adding another address table) one-liner simple.
While touching this, callSchema::connection($this->getConnection())so the migration respectslunar.database.connection.- Schema::table($this->prefix.'addresses', function (Blueprint $table) { - $table->text('delivery_instructions')->nullable()->change(); - }); - - Schema::table($this->prefix.'cart_addresses', function (Blueprint $table) { - $table->text('delivery_instructions')->nullable()->change(); - }); - - Schema::table($this->prefix.'order_addresses', function (Blueprint $table) { - $table->text('delivery_instructions')->nullable()->change(); - }); + foreach (['addresses', 'cart_addresses', 'order_addresses'] as $tbl) { + Schema::connection($this->getConnection()) + ->table($this->prefix.$tbl, function (Blueprint $table) { + $table->text('delivery_instructions')->nullable()->change(); + }); + }
32-42: Apply the same DRY principle indown()Mirror the loop used in
up()so the rollback path stays in sync and equally maintainable.- Schema::table($this->prefix.'addresses', function (Blueprint $table) { - $table->string('delivery_instructions', 255)->nullable()->change(); - }); - - Schema::table($this->prefix.'cart_addresses', function (Blueprint $table) { - $table->string('delivery_instructions', 255)->nullable()->change(); - }); - - Schema::table($this->prefix.'order_addresses', function (Blueprint $table) { - $table->string('delivery_instructions', 255)->nullable()->change(); - }); + foreach (['addresses', 'cart_addresses', 'order_addresses'] as $tbl) { + Schema::connection($this->getConnection()) + ->table($this->prefix.$tbl, function (Blueprint $table) { + $table->string('delivery_instructions', 255)->nullable()->change(); + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/database/migrations/2025_07_18_081937_update_delivery_instructions_columns.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/database/migrations/2025_07_18_081937_update_delivery_instructions_columns.php (1)
packages/core/src/Base/Migration.php (1)
Migration(7-38)
🔇 Additional comments (1)
packages/core/database/migrations/2025_07_18_081937_update_delivery_instructions_columns.php (1)
30-42: Rollback may silently truncate data longer than 255 charsIf anyone has already stored >255-char instructions,
down()will coerce them intoVARCHAR(255)without warning, leading to silent data loss or migration failure (depending on SQL mode).Mitigate by:
• Logging a warning or throwing if rows exist with overlength data before altering,
• or documenting the risk in the migration description / release notes.
There is no need to increase from VARCHAR(255) to TEXT, could be just 500 |
|
@wychoong Why do you think TEXT wouldn't be need here? Do you see any drawback to using it, or do you think 500 characters would be enough for all future cases? |
yes. depends on database engine (read/write performance, storage)
will it be enough for all future cases? maybe, maybe not. but it needs to be reasonable. refer to your issue
it's apparent there is no validation and limit on user input. are we going to increase from TEXT 65,535 characters to LONGTEXT? (well, this is exaggerating) |
|
We actually had a user (in production) who entered a longer instruction (over 255 characters) and encountered a server error due to the column size constraint. That’s what prompted us to consider increasing the limit. If we do decide to go beyond 255, I believe it makes more sense to raise it to at least 1,000 or even 2,000 characters. Increasing it only slightly (e.g., to 500) doesn’t provide much practical benefit, as longer inputs would still be cut off and result in a poor user experience. So if you suggest enforcing a limit, @wychoong, I’d propose setting it to 2,000 characters. BUT Storage-wise, there’s not much difference between VARCHAR(2000) and TEXT for this use case. Both handle ~2000 chars efficiently, and the impact on performance or disk space is minimal. The choice mainly depends on whether we want a fixed length (VARCHAR) or more flexibility (TEXT), and so I think the second option is better. |
|
I would suggest we use Re: the server error - was this on the frontend or the admin panel? |
|
Alright, I’ll update it as suggested. |
|
@glennjacobs I’ve made the requested changes. |
5 participants
This pull request updates the
delivery_instructionscolumn in thelunar_addresses,lunar_cart_addresses, andlunar_order_addressestables fromVARCHAR(255)toTEXT.This change ensures that users can provide longer delivery instructions without encountering SQL errors such as:
SQLSTATE[22001]: Data too long for column 'delivery_instructions'Fixes #2245
Summary by CodeRabbit