Skip to content

Commit

Permalink
formatting and schema handling
Browse files Browse the repository at this point in the history
  • Loading branch information
@taylorotwell
taylorotwell committed Jan 22, 2021
1 parent 97f6f21 commit 7be50a5
Showing 1 changed file with 6 additions and 3 deletions.
9 changes: 6 additions & 3 deletions src/Illuminate/Database/Schema/PostgresSchemaState.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,16 @@ class PostgresSchemaState extends SchemaState
*/
public function dump(Connection $connection, $path)
{
$schema = $connection->getConfig('schema');
$schema = $connection->getConfig('schema', 'public');

This comment has been minimized.

Sign in to view

Copy link
@cbj4074

cbj4074 Jan 23, 2021

Contributor

Doesn't getConfig() accept only one argument?

$schema = $schema === 'public' ? '' : $schema.'.';

$excludedTables = collect($connection->getSchemaBuilder()->getAllTables())
->map->tablename
->reject(function ($table) {
return $table === $this->migrationTable;
})->map(function ($table) use ($schema) {
return '--exclude-table-data='.$schema.'.'.$table;
return '--exclude-table-data='.$schema.$table;
})->implode(' ');

$this->makeProcess(
Expand All @@ -40,7 +43,7 @@ public function dump(Connection $connection, $path)
*/
public function load($path)
{
$command = 'PGPASSWORD=$LARAVEL_LOAD_PASSWORD pg_restore --no-owner --no-acl --clean --if-exists --host=$LARAVEL_LOAD_HOST --port=$LARAVEL_LOAD_PORT --username=$LARAVEL_LOAD_USER --dbname=$LARAVEL_LOAD_DATABASE $LARAVEL_LOAD_PATH';
$command = 'PGPASSWORD=$LARAVEL_LOAD_PASSWORD pg_restore --no-owner --no-acl --host=$LARAVEL_LOAD_HOST --port=$LARAVEL_LOAD_PORT --username=$LARAVEL_LOAD_USER --dbname=$LARAVEL_LOAD_DATABASE $LARAVEL_LOAD_PATH';

if (Str::endsWith($path, '.sql')) {
$command = 'PGPASSWORD=$LARAVEL_LOAD_PASSWORD psql --file=$LARAVEL_LOAD_PATH --host=$LARAVEL_LOAD_HOST --port=$LARAVEL_LOAD_PORT --username=$LARAVEL_LOAD_USER --dbname=$LARAVEL_LOAD_DATABASE';
Expand Down

2 comments on commit 7be50a5

@cbj4074
Copy link
Contributor

Choose a reason for hiding this comment

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

Just some thoughts in reference to #35463 (comment) :

I can appreciate the intent in e5e1ea8 , and it makes sense.

However, none of this is going to work in Laravel 9, because $this->connection->getConfig('schema') will return null, as the schema property on the connection will have been removed. (Note that this has not actually been done in laravel/laravel yet, but it needs to be prior to the 9.x release.)

And in fact, the dump/restore features don't work currently for anyone who uses more than one schema, let alone more than one database.

So, ultimately, a PR should be made against 9.x (in which all of the schema/search_path problems are ironed-out) that fixes dump() and any other methods that rely on a single schema in this way. I'll offer my suggestions as to how we might fix this long-term, in 9.x and later, in a separate thread/PR; there's a lot to it.

With regard to this commit and the behavior in 8.x, there are a couple of issues with this commit:

  1. getConfig() only accepts one argument, so the 'public' passed as a second argument has no effect and can be removed.
  2. $connection->getConfig('schema') can return any number of things, unfortunately, due to how it has evolved over time: a string that represents a single schema; null (if it's not set among the connection's properties); or an array of strings (each of which represents an individual schema). For this reason, this method, as of this commit, will fail in all but the first scenario. Fortunately, Laravel's default 'schema' value is 'public', so this will work for most people (i.e., anyone who uses only one schema, even if it's not named 'public').
  3. For the people who have defined schema as an array, we really have no choice but to use the first value therein. If this doesn't yield the behavior they want or expect, they will need to change their configurations accordingly, or upgrade to 9.x, in which this should be "proper fixed", with multi-schema and multi-database support.
  4. There is no need to remove the public schema qualifier when $connection->getConfig('schema') returns 'public', as doing so doesn't change PostgreSQL's behavior, and it complicates the concatenation.

I think something like this (untested), in place of lines 19-21, would fix this, to the extent that it can be "fixed", in 8.x:

$schema = $connection->getConfig('schema');

if (is_array($schema)) {
    $schema = $schema[0];
}
else {
    $schema = $schema === null ? 'public' : $schema;
}

And we would also need to revert line 28 back to what it was before, because $schema will never be empty:

return '--exclude-table-data='.$schema.'.'.$table;

With regard to how best to handle similar situations in the future, the approach we're discussing here is the best we can do for 8.x and earlier. Given that non-security updates end for 8.x on April 6th, 2021, more complete fixes related to PostgreSQL should be targeted only to 9.x or later, in which we have the tools to handle this and similar scenarios more elegantly.

I can rough-out the conceptual aspects of a PR designed to fix this in 9.x in the next couple of days, if that's helpful.

Just let me know if I misunderstood anything here, or if further clarification would be helpful, and I'll be glad to respond.

@cbj4074
Copy link
Contributor

Choose a reason for hiding this comment

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

Upon further consideration, I drafted a PR that I believe solves the attendant challenges more elegantly: #36046 .

Please sign in to comment.