Multiple issues when PostgreSQL is used with non-public schema

mogtofu33 → closed merge request !1401

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Added reroll of patch #167 on Drupal 10.1.x.

Reroll failed.

Fixed Drupal CS issue of patch #171.

Back to RTBC.

1. +++ b/core/modules/pgsql/src/Driver/Database/pgsql/Connection.php
   @@ -73,6 +73,10 @@ class Connection extends DatabaseConnection implements SupportsTemporaryTablesIn
   +    $this->connectionOptions = $connection_options;
   

   Should we do the sanitizing of the schema name here (the preg_replace), we're doing it twice (once in ::setPrefix and again in Schema::__construct).

   Doing it here would also save any caller of ::getConnectionOptions having to do the same sanitization

2. +++ b/core/modules/pgsql/src/Driver/Database/pgsql/Schema.php
   @@ -116,16 +128,18 @@ protected function ensureIdentifiersLength($table_identifier_part, $column_ident
   -    if (!str_contains($key, '.') && !str_contains($table, 'db_temporary_')) {
   -      $key = 'public.' . $key;
   +    if (strpos($table, 'db_temporary_') !== FALSE) {
   

   this looks like a bad merge, we moved to using str_contains/str_starts_with/str_ends_with in 10.1.x

@larowlan I've adjusted the code according to your suggestions and also some changes due to the latest commits.

The points of @larowlan have been addressed.
Back to RTBC.

Issue credits

Committed to 10.1.x

I think this is eligible for backport, but will ask for a second opinion

Setting to patch (To be ported) in the meantime

I also think this is eligible but we're now approaching the rc of 10.1 and the last bugfix release of 10.0.x so might be easier to leave it in 10.1.x. It will mostly benefit new installs.

If it's going to be ported, I need for 9.5.x as this is issue is holding me back on 9.4.x

@intrafusion can you explain how it's preventing you from updating from 9.4.x to 9.5.x, and also are you able to apply the patch?

@catch the last working patch for me is 🐛 Multiple issues when PostgreSQL is used with non-public schema Fixed and none tagged with 9.5.x, etc. apply cleanly. I haven't had an opportunity to review why

@intrafusion so do you mean you have the patch applied to 9.4.x, and will need to re-apply the patch if it's not backported to 9.5.x? In that case I would suggest trying to work from https://www.drupal.org/project/drupal/issues/1060476#comment-14995059 🐛 Multiple issues when PostgreSQL is used with non-public schema Fixed to create a new 9.5-compatible patch, or that version might even apply without changes. Moving back to fixed.

@catch but it's not fixed, the patch still needs to be ported

@intrafusion it's fixed in Drupal 10.1.x, and the change won't be committed to 10.0.x, or 9.5.x - therefore from the point of view of core development the issue should be 'fixed'.

You are welcome to check if the latest patch applies to 10.0.x and 9.5.x, and to post a backport of that patch to this issue for other people to use, but none of these require changing the issue status.

Automatically closed - issue fixed for 2 weeks with no activity.

Published change record