CommentFileSizeAuthor
#51 2848815-51.patch4.17 KBmondrake
#43 2848815-43.patch4.16 KBvoleger
#40 2848815-40.patch4.38 KBvoleger
#37 interdiff_35-37.txt584 bytesmondrake
#37 2848815-37.patch4.1 KBmondrake
#35 2848815-35.patch4.09 KBmondrake
#35 interdiff_33-35.txt1.08 KBmondrake
#33 interdiff_31-33.txt1.6 KBmondrake
#33 2848815-33.patch4.07 KBmondrake
#31 interdiff_27-31.txt3.98 KBmondrake
#31 2848815-31.patch3.73 KBmondrake
#27 replace_db_next_id_calls-2848815-27.patch839 bytesvoleger
#23 interdiff.txt1.18 KBjeetendrakumar
#23 replace_all_calls_to_db_next_id-2848815-23.patch2.04 KBjeetendrakumar
#20 interdiff-2848815-13-20.txt2.29 KBSharique
#20 replace_all_calls_to_db_next_id-2848815-20.patch1.06 KBSharique
#13 drupal-db_next_id-replaced-with-new-syntax-2848815-10.patch1.14 KBgaurav.kapoor
#11 drupal-db_next_id-replaced-with-new-syntax-2848815-10.patch1.14 KBgaurav.kapoor
#9 drupal-db_next_id-replaced-with-new-syntax-2848815-9.patch1.14 KBvidhatanand
#2 drupal-db_next_id-replaced-with-new-syntax-2848815-2.patch1.14 KBpritamprasun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vidhatanand created an issue. See original summary.

pritamprasun’s picture

vidhatanand’s picture

Issue summary: View changes
pritamprasun’s picture

Status: Active » Needs review
vidhatanand’s picture

cilefen’s picture

Status: Needs review » Needs work
+++ b/core/includes/form.inc
@@ -815,7 +815,7 @@ function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = N
-    $batch['id'] = db_next_id();
+    $batch['id'] = \Drupal::database()->nextId(0);

Why is "0" being passed when it was not with db_next_id()?

vidhatanand’s picture

The db_next_id() has a default argument passed as 0, which probably needs to be passed here now explictly. https://api.drupal.org/api/drupal/core!includes!database.inc/function/db...

cilefen’s picture

Why?

vidhatanand’s picture

Got the point, it's actually not needed.
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%... too has a $existing_id = 0
Updated patch attached.

Status: Needs review » Needs work

The last submitted patch, 9: drupal-db_next_id-replaced-with-new-syntax-2848815-9.patch, failed testing.

gaurav.kapoor’s picture

gaurav.kapoor’s picture

gaurav.kapoor’s picture

gaurav.kapoor’s picture

Status: Needs work » Needs review
JayKandari’s picture

JayKandari’s picture

Status: Needs review » Reviewed & tested by the community
cilefen’s picture

+++ b/core/scripts/generate-d7-content.sh
@@ -51,7 +51,7 @@
-  $query->values(array(db_next_id(), $name, user_hash_password($pass), $mail, 1, $now, $now));
+  $query->values(array(\Drupal::database()->nextId(), $name, user_hash_password($pass), $mail, 1, $now, $now));

See #2232391: Revert drupal 8 changes made to generate-d7-content.sh and generate-d6-content.sh. This file should not be changed.

cilefen’s picture

Status: Reviewed & tested by the community » Needs work
xjm’s picture

Issue tags: +DrupalCampNJ2017
Sharique’s picture

Updated patch.

akashkrishnan01’s picture

Did a quick test, above patch is getting applied successfully

Status: Needs review » Needs work

The last submitted patch, 20: replace_all_calls_to_db_next_id-2848815-20.patch, failed testing.

jeetendrakumar’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
1.18 KB

Here's updated patch file.

KarlKedrovsky’s picture

Tested by doing the following:

  1. Reviewed patch - looked good to me.
  2. Applied patch to 8.4.x-dev cleanly.
  3. Ran all core unit tests from the command line successfully.
  4. Manually tested by trying to run tests using the Testing module. I think this tests the batch bits that the change is part of

Some tests from the Testing module failed but it looks like the batch process that runs them worked fine.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

voleger’s picture

Reroll for 8.6.x.
Reverted changes in db_next_id tests.
So this patch contains only 1 replacement in core/includes/form.inc.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Status: Needs review » Postponed
+++ b/core/includes/form.inc
@@ -819,7 +820,7 @@ function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = N
+    $batch['id'] = Database::getConnection()->nextId();

I think this should be \Drupal::database(), but there is some contention, so I'm postponing this on #2991337: Document the recommended ways to obtain the database connection object

Mile23’s picture

mondrake’s picture

Status: Needs review » Needs work

The last submitted patch, 31: 2848815-31.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Needs review
FileSize
4.07 KB
1.6 KB

Need to install the sequences schema in the deprecation test to test db_next_id.

Status: Needs review » Needs work

The last submitted patch, 33: 2848815-33.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
4.09 KB

$modules visibility.

Status: Needs review » Needs work

The last submitted patch, 35: 2848815-35.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Needs review
FileSize
4.1 KB
584 bytes

Fix for deprecation test.

Status: Needs review » Needs work

The last submitted patch, 37: 2848815-37.patch, failed testing. View results

mondrake’s picture

Issue tags: +Needs reroll
voleger’s picture

Status: Needs work » Needs review
FileSize
4.38 KB
voleger’s picture

Issue tags: -Needs reroll
mondrake’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/KernelTests/Core/Database/NextIdTest.php
@@ -26,14 +28,14 @@ protected function setUp() {
+    $this->assertEquals($first + 1, $second, 'The second call from a sequence provides a number increased by one.');
...
+    $this->assertEquals($result, 1001, 'Sequence provides a larger number than the existing ID.');

Please do not change these from assertEqual to assertEquals - not in the scope of this issue. In any case, assertEquals requires the expected value as first argument and the actual as second, so in the case of $this->assertEquals($result, 1001, ...) they would need to be swapped.

EDIT: also, it would probably be better to use assertSame here to check the type of the returned variable too

voleger’s picture

Status: Needs work » Needs review
FileSize
4.16 KB
andypost’s picture

Why not replace its usage in core/scripts/generate-d7-content.sh?

core8$ git grep db_next_id
core/includes/database.inc:486:function db_next_id($existing_id = 0) {
core/includes/form.inc:827:    $batch['id'] = db_next_id();
core/scripts/generate-d7-content.sh:54:  $query->values(array(db_next_id(), $name, user_hash_password($pass), $mail, 1, $now, $now));
core/tests/Drupal/KernelTests/Core/Database/NextIdTest.php:29:    $first = db_next_id();
core/tests/Drupal/KernelTests/Core/Database/NextIdTest.php:30:    $second = db_next_id();
core/tests/Drupal/KernelTests/Core/Database/NextIdTest.php:35:    $result = db_next_id(1000);
mondrake’s picture

@andypost core/scripts/generate-d7-content.sh generates d7 code so that should stay as is. See #2232391: Revert drupal 8 changes made to generate-d7-content.sh and generate-d6-content.sh.

andypost’s picture

@mondrake thanx for pointer, other then test change it looks good to go

+++ b/core/tests/Drupal/KernelTests/Core/Database/NextIdTest.php
@@ -2,21 +2,23 @@
-class NextIdTest extends KernelTestBase {
+class NextIdTest extends DatabaseTestBase {
...
-  public static $modules = ['system'];
+  public static $modules = ['database_test', 'system'];

@@ -26,13 +28,13 @@ protected function setUp() {
-    $first = db_next_id();
-    $second = db_next_id();
+    $first = $this->connection->nextId();
+    $second = $this->connection->nextId();
...
-    $result = db_next_id(1000);
+    $result = $this->connection->nextId(1000);

Not sure the change makes sense in context #2991337: Document the recommended ways to obtain the database connection object

mondrake’s picture

#46 changing that test class to inherit from DatabaseTestBase (like all the other test classes in the namespace, except SchemaTest) allows us to leverage the $connection property defined in setUp. I think that make sense.

If your question is 'how' the $connection property is initialised - fine that will be decided in #2991337: Document the recommended ways to obtain the database connection object, then we can change the setUp method in test base class accordingly and all the extending classes will benefit.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Then it looks good to go!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 43: 2848815-43.patch, failed testing. View results

mondrake’s picture

Issue tags: +Needs reroll
mondrake’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
4.17 KB

Rerolled.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6b61e10 and pushed to 8.7.x. Thanks!

  • catch committed 6b61e10 on 8.7.x
    Issue #2848815 by mondrake, voleger, gaurav.kapoor, jeetendrakumar,...

Status: Fixed » Closed (fixed)

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