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

[10.x] MySQL Grammar and Blueprint binary column fix #50354

Closed
wants to merge 1 commit into from
Closed

[10.x] MySQL Grammar and Blueprint binary column fix #50354

wants to merge 1 commit into from

Conversation

julesgraus
Copy link
Contributor

The Blueprint class has a binary method that creates a blob column in your database.

I added a method called blob that creates a blob field in the database, and I did modify the binary method to create a binary field instead.

I've also modified the typeBinary method in the MySQLGrammar class so it returns 'binary' instead of 'blob' and added a typeBlob method that returns 'blob'

As a finishing touch I've modified the testAddingBinary in DatabaseMySqlSchemaGrammarTest.php and added the testAddingBinary methods to ensure the quality.

@julesgraus
Copy link
Contributor Author

Some background info on WHY I do want to fix this.

I am experimenting with EventSauce. An event sourcing package. It requires you to add specific binary fields to your database table. When I did use the binary method on the blueprint class, I was surprised I did get a blob field instead.

Of course one can fix this by executing raw statements, but I think this is less confusing if we solve it like this.

@julesgraus julesgraus changed the title MySQL Grammar and Blueprint binary column fix [10.x] MySQL Grammar and Blueprint binary column fix Mar 3, 2024
@hafezdivandari
Copy link
Contributor

hafezdivandari commented Mar 3, 2024

This is a breaking change. Instead you may try ->charset('binary') to achieve what you want:

Schema::table('test', function (Blueprint $table) {
    $table->char('binary_column', 16)->charset('binary');       // binary(16)
    $table->string('varbinary_column', 16)->charset('binary');  // varbinary(16)
    $table->tinyText('tinyblob_column')->charset('binary');     // tinyblob
    $table->text('blob_column')->charset('binary');             // blob
    $table->mediumText('mediumblob_column')->charset('binary'); // mediumblob
    $table->longText('longblob_column')->charset('binary');     // longblob
});

MySQL Docs:

Specifying the CHARACTER SET binary attribute for a character string data type causes the column to be created as the corresponding binary string data type: CHAR becomes BINARY, VARCHAR becomes VARBINARY, and TEXT becomes BLOB.

@crynobone
Copy link
Member

@hafezdivandari

Wouldn't it be useful if we can have asBinary() as a helper method?

-$table->char('binary_column', 16)->collation('binary');
+$table->char('binary_column' 16)->asBinary();

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Mar 4, 2024

@crynobone I think the main problem is that user expects binary type when using binary method, but gets blob on MySQL. Alternatively, we can add length and fixed to binary method on Laravel 11.x. I sent a PR for this #50355

@julesgraus
Copy link
Contributor Author

Appending something to char would not be as easy as for example like adding a text column.

And you could see this as a breaking change, but this functionality in my point of view was broken anyway. Simply because calling the binary method, does not add a binary field to your database. Therefore I consider it a bug. And it only could break users websites after they migrate. Upgrading would not be good enough to break your site right?

@taylorotwell
Copy link
Member

Not going to take any action on L10 with L11 so close to release (this week or next).

@julesgraus
Copy link
Contributor Author

julesgraus commented Mar 4, 2024

Ok. I will create the same kind of PR for Laravel 11.

Oh, never mind. I see it's fixed! Thanks.
Is it strange to ask for a blob method too? Since it kinda feels a bit strange if the binary method can create 3 different types of database fields now? Depending on the params passed into it.

@julesgraus julesgraus deleted the feature/improve-binary-and-blob branch March 4, 2024 18:03
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