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

[8.x] Move primary after collate #37815

Merged
merged 2 commits into from
Jun 25, 2021
Merged

[8.x] Move primary after collate #37815

merged 2 commits into from
Jun 25, 2021

Conversation

Synchro
Copy link
Contributor

@Synchro Synchro commented Jun 25, 2021

This simply changes the clause order in the MySQL grammar, and appears to fix #37811 in a similar way to the fix that resulted in the 8.48.1 release (which failed for me). I don't know what the definitive order of clauses should be, but this change fixes the issue I found.

@driesvints driesvints changed the title Move primary after collate, fixes #37811 [8.x] Move primary after collate Jun 25, 2021
@driesvints
Copy link
Member

Ping @khalilst

@khalilst
Copy link
Contributor

khalilst commented Jun 25, 2021

@driesvints , @Synchro I have checked the problem and it was related to charset ordering and not collate.
Line 18 in the commit could be like this:

'Unsigned', 'Charset', 'Primary', 'Collate', 'VirtualAs', 'StoredAs', 'Nullable',

However the current ordering in this PR has no problem, but putting the Primary after Collate is NOT necessary.
I will create another test for this fix.

@driesvints
Copy link
Member

@khalilst we got another issue about the original PR: #37820

I'm sending in a PR to revert all of this.

@khalilst
Copy link
Contributor

khalilst commented Jun 25, 2021

@driesvints Just give me an hour to fix this: #37820 .

@Synchro
Copy link
Contributor Author

Synchro commented Jun 25, 2021

FWIW I've updated this PR to move it to after charset instead of collate.

@khalilst
Copy link
Contributor

khalilst commented Jun 25, 2021

@khalilst we got another issue about the original PR: c

I'm sending in a PR to revert all of this.

@driesvints
I have checked issue #37820 and it was working without any problem.
Probably, it was a wrong bug report or there is something missing there.

@driesvints
Copy link
Member

We're gonna revert all of this.

@driesvints driesvints closed this Jun 25, 2021
@khalilst
Copy link
Contributor

@driesvints I think these bugs are very tiny ones. Just need more test cases to prevent a similar situation.

@taylorotwell Please consider this issue #33238

@driesvints driesvints reopened this Jun 25, 2021
@taylorotwell taylorotwell merged commit d27132e into laravel:8.x Jun 25, 2021
@taylorotwell
Copy link
Member

@khalilst we will give this a bit longer but we don't particularly care about DigitalOcean's problems. They aren't ours to fix. Complain to them.

@Synchro Synchro deleted the primarykey-37811 branch June 25, 2021 15:15
@Synchro
Copy link
Contributor Author

Synchro commented Jun 25, 2021

Thanks for merging

@khalilst
Copy link
Contributor

@taylorotwell Thanks and I do agree.
Please consider it will result in a single query and faster execution. however, it is very small.

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.

MySQL custom primary key creation still broken in 8.48.1
4 participants