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] Revert primary key stuff #37826

Merged
merged 3 commits into from
Jun 25, 2021
Merged

[8.x] Revert primary key stuff #37826

merged 3 commits into from
Jun 25, 2021

Conversation

driesvints
Copy link
Member

This reverts #37715 & #37782.

Fixes #37820 #37811 #37781

@khalilst
Copy link
Contributor

@driesvints With all due respect, I think you are in a hurry to revert.
The bugs related to that PR could be easily fixed.

Please consider this issue #33238 and @taylorotwell invited to solve that problem.

@khalilst
Copy link
Contributor

khalilst commented Jun 25, 2021

@taylorotwell
This bug #37820 related to the third-party package overriding the Blueprint.

The package name: bosnadev/database
File path: Bosnadev/Database/Schema/Blueprint.php
It is required by mstaack/laravel-postgis.

Also, there is an issue and PR in that package to solve this problem.
bosnadev/database#48
bosnadev/database#49

The two other bugs are fixed with tiny changes.
I think reverting these PRs #37715 & #37782 is not fair.

@Shkeats
Copy link

Shkeats commented Jun 25, 2021

I had to lock back to 8.47.0 to avoid SQLSTATE[42000]: Syntax error or access violation: 1068 Multiple primary key defined with our existing mysql migrations. I think it's right to revert this stuff as its a breaking change.

@khalilst
Copy link
Contributor

@Shkeats , If you would share the failed migration, I may fix it.

@Shkeats
Copy link

Shkeats commented Jun 25, 2021

@khalilst I don't think it's really a scalable approach for you to rewrite everyone's migrations, although the offer is appreciated!

@Shkeats
Copy link

Shkeats commented Jun 25, 2021

We don't have bosnadev/database in our codebase and I suspect my issue is already covered by the other ones reported. However, if it's helpful at all @driesvints I can open a new issue with our specific case.

@driesvints
Copy link
Member Author

@Shkeats can you try the new release we just made?

Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@Shkeats
Copy link

Shkeats commented Jun 25, 2021

@driesvints I'm still seeing 8.48.1 as latest from what I can tell? I tried applying this change locally but it didn't help in my case.

@taylorotwell
Copy link
Member

@khalilst we have to revert this. We have more people in this thread reporting breakages that aren't related to a package. Laravel is used by many many thousands of applications and I can't just leave them broken. Sorry.

@taylorotwell taylorotwell merged commit 6651d7e into 8.x Jun 25, 2021
@taylorotwell taylorotwell deleted the revert-primary-key-stuff branch June 25, 2021 23:57
@taylorotwell
Copy link
Member

@khalilst would be willing to explore this again at a later time but it clearly needs more thorough exploration of what these breaking changes are.

@khalilst
Copy link
Contributor

khalilst commented Jun 26, 2021

@taylorotwell I'm just trying to fix the issues, but as you can see here @Shkeats even did not provide enough information and I think it was not a good idea to consider it as a breaking change!

BWT, Thank you so much and I think you have good reasons to do this.

@khalilst
Copy link
Contributor

@khalilst would be willing to explore this again at a later time but it clearly needs more thorough exploration of what these breaking changes are.

@taylorotwell Sure, I will create a package and after resolving all bugs, I will create another PR for this thread.

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.

Postgres String Primary Key Migration Fail
5 participants