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.2] Fix postgres Schema::hasTable(); #13008

Merged
merged 1 commit into from
Apr 25, 2016
Merged

[5.2] Fix postgres Schema::hasTable(); #13008

merged 1 commit into from
Apr 25, 2016

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Apr 4, 2016

As reported in #12856, the current query looks for tables in the information_schema regardless of the TABLE_SCHEMA.

@@ -28,7 +28,7 @@ class PostgresGrammar extends Grammar
*/
public function compileTableExists()
{
return 'select * from information_schema.tables where table_name = ?';
return 'select * from information_schema.tables where table_schema = ? and table_name = ?';
Copy link
Member Author

Choose a reason for hiding this comment

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

@GrahamCampbell changing the sql considered breaking? It's a public method. Should I submit to master?

Copy link
Member

Choose a reason for hiding this comment

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

Probably fine.

@themsaid themsaid closed this Apr 4, 2016
@themsaid themsaid reopened this Apr 4, 2016
@taylorotwell
Copy link
Member

I'm a little confused because "database" is not the same as "schema" in Postgres?

@themsaid
Copy link
Member Author

themsaid commented Apr 8, 2016

Yes, this fix searches for the existence of a table in a schema regardless of the database. When you compare Postgres to MySQL, if I'm on MySQL and would like to create a new DB for purpose (x) it's equivalent to creating a new schema in Postgres for the same purpose (x) not a new database.

Maybe this PR is a bit opinionated, if you'd like I can close it and seek a more general solution that allows searching for a table with regards to the schema (and) the database.

Or maybe change the syntax of $table to accept table_name or schema_name.table_name, to make it more obvious that the look up is checking for a table in a schema, not a database in Postgres terms.

What do you think?

@pelim
Copy link

pelim commented May 25, 2016

@taylorotwell: can this fix also be merged into the laravel 5.1 branch

@taylorotwell
Copy link
Member

Please send a PR.

On May 25, 2016, at 4:27 AM, Peter Limbach notifications@github.com wrote:

@taylorotwell https://github.com/taylorotwell: can this fix also be merged into the laravel 5.1 branch


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #13008 (comment)

GrahamCampbell pushed a commit that referenced this pull request May 25, 2016
@GrahamCampbell
Copy link
Member

I've backported this to 5.1 for you.

@zogot
Copy link

zogot commented Jun 6, 2016

@GrahamCampbell I think that has caused an issue for 5.1

https://github.com/laravel/framework/blob/5.1/src/Illuminate/Database/Schema/Builder.php#L55 Still only passes in 1 parameter to the statement, causing my migrate commands to fail

@GrahamCampbell
Copy link
Member

I'll revert the backport on 5.1.

GrahamCampbell added a commit that referenced this pull request Jun 6, 2016
@zogot
Copy link

zogot commented Jun 6, 2016

@GrahamCampbell umm might also be an issue in 5.2, since that too only gives 1 parameter. https://github.com/laravel/framework/blob/5.2/src/Illuminate/Database/Schema/Builder.php#L55

The comment in here also details adding schema to the parameters: #12856 but that is also not in 5.2

GrahamCampbell added a commit that referenced this pull request Jun 6, 2016
@GrahamCampbell
Copy link
Member

Feel free to send a fix to L5.1 and re-revert my revert if you can fix it.

@GrahamCampbell
Copy link
Member

Ping @themsaid.

@zogot
Copy link

zogot commented Jun 6, 2016

@GrahamCampbell gosh I'm very sorry, it appears to be an issue on my extension of the Postgres library. The PR does correctly have it. Again very sorry.

@taylorotwell
Copy link
Member

Cool, thanks for letting us know.

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.

5 participants