[D7 PHP 8.1] Undefined array key "sequence_name" [#3258185]

Problem/Motivation

We are experiencing a new warning after migration to PHP 8.1:

Warning: Undefined array key "sequence_name" in DatabaseConnection_pgsql->query() (line 120 in /xxx/includes/database/pgsql/database.inc).

PHP: 8.1
Database: PostgreSQL 11
Drupal: 7.84

This warning was not present on PHP 7.3 / PostgreSQL 11.

Steps to reproduce

Proposed resolution

It seems like that this is already fixed in D9, as the code here is:

core\lib\Drupal\Core\Database\Connection.php

case Database::RETURN_INSERT_ID:
          $sequence_name = $options['sequence_name'] ?? NULL;
          return $this->connection->lastInsertId($sequence_name);

However the code in D7 is following:

includes\database\pgsql\database.inc

case Database::RETURN_INSERT_ID:
          return $this->connection->lastInsertId($options['sequence_name']);

It should be sufficient to implement the same change to D7 as it is in D9.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comment	File	Size	Author
#13	3258185-13.patch	727 bytes	poker10
#11	3258185-11.patch	739 bytes	poker10

Comments

Comment #1

12 January 2022 at 11:40

poker10 created an issue. See original summary.

Comment #2

poker10 commented 12 January 2022 at 11:51

Issue summary:

View changes

Comment #3

beatrizrodrigues

She/Her

commented 12 January 2022 at 12:08

Assigned:

Unassigned

» beatrizrodrigues

I will work on that

Comment #4

liam morland

English

Ontario, CA 🇨🇦

commented 12 January 2022 at 14:00

Issue tags:

+PostgreSQL

Comment #5

mustanggb commented 13 January 2022 at 11:37

Just noting that the ?? operator is PHP 7.0 and D7 needs to be PHP 5.3 compliant, so you'll need to assign it manually.

Comment #6

beatrizrodrigues

She/Her

commented 13 January 2022 at 12:54

I'm trying to get that error message, but I'm not being able to. I created my environment using lando and did a recipe using PHP 8.1 and postgres 11 but no message is displayed to me. Do you have any tips for me, please? I would really enjoy get this issue done. thank you!!

Comment #7

poker10 commented 13 January 2022 at 16:38

I have digged into this a little deeper and it seems like that the warning is not consistently appearing.. I tried to simulate and debug it on my local enviroment (with exact copy of the affected system and the same PHP / PostgreSQL configuration) but the warning was not there.

What I can add to this issue is backtrace which leads to this issue on our server. For example this warning appears when I open new watchdog entry (or another URL which is not in menu_cache table):

... (Array, 15 elements)
	0 (Array, 7 elements)
		file (String, 68 characters ) xxx/includes/database/query.inc
		line (Integer) 644
		function (String, 5 characters ) query
		class (String, 24 characters ) DatabaseConnection_pgsql
		object (Object) DatabaseConnection_pgsql
		type (String, 2 characters ) ->
		args (Array, 3 elements)
			0 (String, 88 characters ) INSERT INTO {cache_menu} (cid, serialized, created, expire, data) VALUES (?, ?, ?, ?, ?)
			1 (Array, 5 elements)
				0 (String, 53 characters ) links:main-menu:page:admin/reports/event/16128:sk:1:1
				1 (String, 1 characters ) 1
				2 (String, 10 characters ) 1642082325
				3 (String, 1 characters ) 0
				4 (String, 109 characters ) a:4:{s:9:"min_depth";i:1;s:9:"max_depth";i:1;s:8:"expanded";a:1:{i:0;i:0;}s:12:"active_trail";a:1:{i:0;i:0;}}
			2 (Array, 4 elements)
				return (Integer) 3
				target (String, 7 characters ) default
				fetch (Integer) 5
				throw_exception (Boolean) TRUE
	1 (Array, 7 elements)
		file (String, 68 characters ) xxx/includes/database/query.inc
		line (Integer) 1634
		function (String, 7 characters ) execute
		class (String, 11 characters ) InsertQuery
		object (Object) InsertQuery
		type (String, 2 characters ) ->
		args (Array, 0 elements)
	2 (Array, 7 elements)
		file (String, 59 characters ) xxx/includes/cache.inc
		line (Integer) 477
		function (String, 7 characters ) execute
		class (String, 10 characters ) MergeQuery
		object (Object) MergeQuery
		type (String, 2 characters ) ->
		args (Array, 0 elements)
	3 (Array, 7 elements)
		file (String, 59 characters ) xxx/includes/cache.inc
		line (Integer) 142
		function (String, 3 characters ) set
		class (String, 19 characters ) DrupalDatabaseCache
		object (Object) DrupalDatabaseCache
		type (String, 2 characters ) ->
		args (Array, 3 elements)
			0 (String, 53 characters ) links:main-menu:page:admin/reports/event/16128:sk:1:1
			1 (Array, 4 elements)
			2 (Integer) 0
	4 (Array, 4 elements)
		file (String, 58 characters ) xxx/includes/menu.inc
		line (Integer) 1324
		function (String, 9 characters ) cache_set | (Callback) cache_set();
		args (Array, 3 elements)
			0 (String, 53 characters ) links:main-menu:page:admin/reports/event/16128:sk:1:1
			1 (Array, 4 elements)
			2 (String, 10 characters ) cache_menu
	5 (Array, 4 elements)
		file (String, 58 characters ) xxx/includes/menu.inc
		line (Integer) 1865
		function (String, 19 characters ) menu_tree_page_data | (Callback) menu_tree_page_data();
		args (Array, 2 elements)
			0 (String, 9 characters ) main-menu
			1 (Integer) 1
	6 (Array, 4 elements)
		file (String, 58 characters ) xxx/includes/menu.inc
		line (Integer) 1829
		function (String, 21 characters ) menu_navigation_links | (Callback) menu_navigation_links();
		args (Array, 1 element)
			0 (String, 9 characters ) main-menu
	7 (Array, 4 elements)
		file (String, 59 characters ) xxx/includes/theme.inc
		line (Integer) 2705
		function (String, 14 characters ) menu_main_menu | (Callback) menu_main_menu();
		args (Array, 0 elements)
	8 (Array, 4 elements)
		file (String, 59 characters ) xxx/includes/theme.inc
		line (Integer) 1125
		function (String, 24 characters ) template_preprocess_page | (Callback) template_preprocess_page();
		args (Array, 2 elements)
	9 (Array, 4 elements)
		file (String, 60 characters ) xxx/includes/common.inc
		line (Integer) 6146
		function (String, 5 characters ) theme | (Callback) theme();
		args (Array, 2 elements)
	10 (Array, 4 elements)
		file (String, 60 characters ) xxx/includes/common.inc
		line (Integer) 6008
		function (String, 13 characters ) drupal_render | (Callback) drupal_render();
		args (Array, 1 element)
	11 (Array, 4 elements)
		file (String, 60 characters ) xxx/includes/common.inc
		line (Integer) 2796
		function (String, 18 characters ) drupal_render_page | (Callback) drupal_render_page();
		args (Array, 1 element)
	12 (Array, 4 elements)
		file (String, 60 characters ) xxx/includes/common.inc
		line (Integer) 2656
		function (String, 24 characters ) drupal_deliver_html_page | (Callback) drupal_deliver_html_page();
		args (Array, 1 element)
	13 (Array, 4 elements)
		file (String, 58 characters ) xxx/includes/menu.inc
		line (Integer) 542
		function (String, 19 characters ) drupal_deliver_page | (Callback) drupal_deliver_page();
		args (Array, 2 elements)
	14 (Array, 4 elements)
		file (String, 50 characters ) xxx/index.php
		line (Integer) 21
		function (String, 27 characters ) menu_execute_active_handler | (Callback) menu_execute_active_handler();
		args (Array, 0 elements)

The backtrace from my local enviroment however differs in the second step:

	1 (Array, 7 elements)
		file (String, 68 characters ) xxx/includes/database/query.inc
		line (Integer) 1634
		function (String, 7 characters ) execute
		class (String, 17 characters ) InsertQuery_pgsql
		object (Object) InsertQuery_pgsql
		type (String, 2 characters ) ->
		args (Array, 0 elements)

See the difference which "InsertQuery" class is invoked on each host - on the server it is only "InsertQuery", but on the local enviroment it is correctly "InsertQuery_pgsql". This difference caused that this check (below) was not run, as it is only in InsertQuery_pgsql class, which was not invoked:

    if (!empty($table_information->sequences)) {
      $options['sequence_name'] = $table_information->sequences[0];
    }
    // If there are no sequences then we can't get a last insert id.
    elseif ($options['return'] == Database::RETURN_INSERT_ID) {
      $options['return'] = Database::RETURN_NULL;
    }

And as the cache_menu table does not have sequence, it remains with the Database::RETURN_INSERT_ID, but without $options['sequence_name'] set.

But I do not know, how this is happening. Settings.php is configured properly with pgsql driver, website is running on the PostgreSQL database without any problems (except these warnings and deprecations messages).. It is a little bit weird.

And also it is a little bit unclear to me why this was fixed / changed in D9 codebase - it implies to me that there should have been some issues with this part of the code and the case where this could happen. However I am unable to find issue in Drupal issue queue which caused this change in D8/D9.

Comment #8

liam morland

English

Ontario, CA 🇨🇦

commented 13 January 2022 at 17:01

$sequence_name was added to Drupal 8 in commit cfdf10c for issue #2454625: SQLite: Fix SQLITE_SCHEMA errors in web tests.

The Drupal 7 code was as it was since it was first introduced in commit 69e6f41.

Comment #9

poker10 commented 13 January 2022 at 19:20

Thank you @Liam Morland.

I checked that SQLite issue and there was applied some general approach in which the identical code from core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php query function was also moved out to core/lib/Drupal/Core/Database/Connection.php. And this should (maybe unintentionally) fixed also the issue if (in any case) the "sequence_name" was empty.

So I do not know, if this should be the correct way how to handle this also in D7, or if it will be sufficient to sanitize it in the current place in includes\database\pgsql\database.inc. Or if the core maintainers will allow any fix at all, if this is only some weird use-case...

Comment #10

beatrizrodrigues

She/Her

commented 13 January 2022 at 19:36

Assigned:

beatrizrodrigues

» Unassigned

Comment #11

poker10 commented 13 January 2022 at 23:51

Status	File	Size
new	3258185-11.patch	739 bytes

This patch fixed the warnings for us.

Comment #12

poker10 commented 14 January 2022 at 00:04

Status:

Active

» Needs review

Comment #13

poker10 commented 19 January 2022 at 15:56

Status	File	Size
new	3258185-13.patch	727 bytes

1 file was hidden/shown/deleted

Status	File	Size
hidden	3258185-11.patch	739 bytes

Patch re-test.

Comment #14

poker10 commented 19 January 2022 at 17:30

It seems like that there is another PHP 8.1 deprecation message directly on Drupal 7.85 install on PostgreSQL database, see this test:

https://www.drupal.org/pift-ci-job/2298354

Installation complete.                                               [1;32;40m[1m[ok][0m
Deprecated function: fwrite(): Passing null to parameter #2 ($data)  [31;40m[1m[error][0m
of type string is deprecated in InsertQuery_pgsql-&gt;execute() (line
33 of /var/www/html/includes/database/pgsql/query.inc).

I will create separate issue for this.

Comment #15

mcdruid commented 3 February 2022 at 10:53

Hmm, that patch in #13 looks like it should be mostly harmless.

However, we have a problem in that D7's core tests do not pass with PostgreSQL. In fact, they don't run properly at all at the moment.

It'd be great to fix that so that we can test changes like this properly.

It is helpful to see that the same change has been made in D8/9 though, and that makes me a lot more comfortable with the patch.

Without test coverage we're somewhat reliant on people who actually use PostrgreSQL to confirm that patches work, and @poker10 has done so in #11. AFAICS the only differences between the patches in #11 and #13 are down to whitespace.

Comment #16

mcdruid commented 3 February 2022 at 11:19

Status:

Needs review

» Reviewed & tested by the community

I've queued tests for everything other than PostgreSQL, but this change is in the psql driver in D7 (unlike in D8/9 where it's in the shared db code) so we wouldn't expect it to cause any issues.

As @Liam Morland pointed out the exact same code as in the patch is still in Drupal 8.9 and was unchanged for several years. That suggests it works okay:

https://git.drupalcode.org/project/drupal/-/blob/8.9.20/core/lib/Drupal/...

        case Database::RETURN_INSERT_ID:
          $sequence_name = isset($options['sequence_name']) ? $options['sequence_name'] : NULL;
          return $this->connection->lastInsertId($sequence_name);

The PHP docs confirm that passing NULL to the lastInsertId method is fine:

https://www.php.net/manual/en/pdo.lastinsertid.php

So this looks pretty safe to me. I'll commit it once all the non-PostgreSQL tests have passed.

Comment #17

mcdruid commented 3 February 2022 at 12:03

Confirmed also that sequence_name only seems to exist in the pgsql driver:

drupal-7.x$ grep -rn sequence_name *
includes/database/pgsql/query.inc:85:      $options['sequence_name'] = $table_information->sequences[0];
includes/database/pgsql/database.inc:120:          return $this->connection->lastInsertId($options['sequence_name']);
includes/database/pgsql/database.inc:187:    $sequence_name = $this->makeSequenceName('sequences', 'value');
includes/database/pgsql/database.inc:191:    $id = $this->query("SELECT nextval('" . $sequence_name . "')")->fetchField();
includes/database/pgsql/database.inc:203:    $id = $this->query("SELECT nextval('" . $sequence_name . "')")->fetchField();
includes/database/pgsql/database.inc:210:    $this->query("ALTER SEQUENCE " . $sequence_name . " RESTART WITH " . ($existing + 1));
includes/database/pgsql/database.inc:213:    $id = $this->query("SELECT nextval('" . $sequence_name . "')")->fetchField();

Do any of those other occurrences require any updates?

I think #13 is okay to commit either way.

Comment #18

3 February 2022 at 12:06

mcdruid committed ac86dd5 on 7.x

Issue #3258185 by poker10, beatrizrodrigues, mcdruid, Liam Morland,...

Comment #19

mcdruid commented 3 February 2022 at 12:08

Status:

Reviewed & tested by the community

» Fixed

Thanks everyone.

Incidentally, it doesn't look like we have a parent issue for "fix PostgreSQL tests in D7". There was #710858: Meta issue: fix the remaining PostgreSQL bugs but that seems very outdated now. Anyone want to file one? I'm not saying it will be prioritised any time soon, but seems like it'd be a good issue to have.

Comment #20

liam morland

English

Ontario, CA 🇨🇦

commented 3 February 2022 at 13:03

That issue is old, but it doesn't mean it can't be used. The first step would be for testing to be enabled. The automated testing page for core only tests MySQL and SQLite.

Testing won't work at all until #3259482: [D7 PHP 8.1] fwrite(): Passing null to parameter #2 ($data) of type string is deprecated in InsertQuery_pgsql->execute() is fixed.

Comment #21

poker10 commented 3 February 2022 at 18:30

I think that one of the reasons why testing does not work on PostgreSQL is a new test table introduced by #2802159: [D7] SQL layer: $match_operator is vulnerable to injection attack.

More information here #3262341: [D7] Database test table TEST_UPPERCASE causes PostgreSQL tests to fail.

Comment #22

17 February 2022 at 18:34

Status:

Fixed

» Closed (fixed)

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

[D7 PHP 8.1] Undefined array key "sequence_name"

Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Comment #21

Comment #22

Parent issue

Referenced by

News items

Our community

Documentation

Drupal code base

Governance of community