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

[5.7] Fix prefixed table indexes #25867

Merged
merged 5 commits into from
Oct 2, 2018
Merged

[5.7] Fix prefixed table indexes #25867

merged 5 commits into from
Oct 2, 2018

Conversation

laurencei
Copy link
Contributor

@laurencei laurencei commented Oct 2, 2018

This is an attempt to fix #7889. This issue is the oldest outstanding Laravel Framework issue at over 3.5 years old (March 2015) - so I thought I'd give it a crack. 😄

Also fixes duplicates of the original issue - #23142, #13683 and #9040

Essentially the problem boils down that the Blueprint class prepares the SQL commands without taking into account the database connection prefix. While the table prefix stuff is handled further up, we need to do the table indexes inside the Blueprint class.

This is a non-breaking and full backwards compatible change; I've set default behavior for existing projects to maintain current behavior. This is because migrations and foreign keys etc could break mid-project, so they need to maintain existing behavior unless they want to manually change.

All new projects will default to the new behavior - but it is fully configurable either way.

I've done laravel/laravel#4783 as a related PR with the new config option if this one is accepted.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@taylorotwell taylorotwell merged commit cfec0bf into laravel:5.7 Oct 2, 2018
@franzliedke
Copy link
Contributor

@laurencei It's interesting to see you added the prefix to the Blueprint class, given that no other functions that use the prefix (such as those creating a table) add the prefix this way. Instead, they rely on the grammar classes to do so. All of those inherit from Illuminate\Database\Grammar, and use the various wrap* functions of that base class. Most importantly, wrapTable().

IMO, the fix should have been to use wrapTable() for the index name in the various compileIndex() methods of the schema grammar subclasses.

@laurencei
Copy link
Contributor Author

@franzliedke - thanks. I did consider and look at that at the time. But for some reason I didnt think it would work. I went back and had another look - and your right, that is a better spot. I must have overlooked something the first time.

I'll submit a PR for a refactor shortly. Thanks.

@franzliedke
Copy link
Contributor

@laurencei Awesome! And thanks for tackling this issue in the first place. 👍

Maybe it was harder to do the config thing in the Grammar class? If so, maybe it can be added without the config to 5.8, where breaking changes are allowed?

@ChrisThompsonTLDR
Copy link

ChrisThompsonTLDR commented Oct 17, 2018

@laurencei thanks for taking a crack on fixing this. I appreciate it and look forward to seeing this patched.

@franzliedke
Copy link
Contributor

@laurencei Have you had any luck with that refactoring? 🙈

@laurencei
Copy link
Contributor Author

It's on my "todo" list.

I would technically be a breaking change - so it'll have to go to 5.8.

I'll try and get to it next week...

@franzliedke
Copy link
Contributor

It would be a refactoring affecting only internal classes and no contracts. Why do you consider that breaking? Laravel has always been quite loose about that...

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.

None yet

4 participants