Support from Acquia helps fund testing for Drupal Acquia logo

Comments

techtud created an issue. See original summary.

vidhatanand’s picture

vidhatanand’s picture

Status: Active » Needs review
JayKandari’s picture

Status: Needs review » Reviewed & tested by the community

Only 1 instance of "db_transaction()" found. #2 Looks good.

Thanks.

cilefen’s picture

Status: Reviewed & tested by the community » Needs work

Read the issue summary carefully about test usage replacements.

xjm’s picture

Issue tags: +DrupalCampNJ2017
jeetendrakumar’s picture

Status: Needs review » Needs work

The last submitted patch, 7: drupal-replace-calls-to-db_transaction-2848820-7.patch, failed testing.

jeetendrakumar’s picture

Sharique’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Database/database.api.php
    @@ -150,7 +150,7 @@
    + * call @code $txn = \Drupal::database()->startTransaction(); @endcode The transaction will
    

    Documentation will be updated in separate ticket.

  2. +++ b/core/lib/Drupal/Core/Database/database.api.php
    @@ -161,7 +161,7 @@
    + *   $txn = \Drupal::database()->startTransaction();
    

    Documentation will be updated in separate ticket.

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/MenuRouterRebuildSubscriber.php
    @@ -27,16 +29,35 @@ class MenuRouterRebuildSubscriber implements EventSubscriberInterface {
    +  public function __construct(LockBackendInterface $lock, MenuLinkManagerInterface $menu_link_manager, Connection $database) {
    

    Not sure, this change is required or not in the context of ticket.

jeetendrakumar’s picture

@Sharique

point 1 and 2: Done
Point 3: I think we need to inject the dependency.

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

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/EventSubscriber/MenuRouterRebuildSubscriber.php
@@ -27,16 +29,35 @@ class MenuRouterRebuildSubscriber implements EventSubscriberInterface {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static(
+      $container->get('database')
+    );
   }

Not correct create() method implementation. Database service should pass through the third argument. Anyway, I guess it is redundant code. Database service injecting already defined in core.services.yml

andypost’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/MenuRouterRebuildSubscriber.php
@@ -27,16 +29,35 @@ class MenuRouterRebuildSubscriber implements EventSubscriberInterface {
+  protected $database;
...
+  public function __construct(LockBackendInterface $lock, MenuLinkManagerInterface $menu_link_manager, Connection $database) {
...
+    $this->database = $database;

@@ -55,7 +76,7 @@ public function onRouterRebuild(Event $event) {
+      $transaction = $this->database->startTransaction();

grep through core shows that *this->connection* is mostly used against *this->database*

andypost’s picture

Status: Needs work » Needs review
FileSize
15.06 KB
15.27 KB

Rerolled, added triggering deprecated error & test
Also converted core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php
I'm sure changes are in scope cos test for inTransaction() should be made from the same connection as startTransaction() requested

Linked to #2947775: Move setting default target out of db_merge() and other deprecated db_* functions cos setting defaults needs solved somehow

There's is only 2 mentions in docs but that's separate issue

$ git grep -F 'this->database'|wc -l
430
$ git grep -F 'this->connection'|wc -l
544

Transactions are per connection but database mostly used in backend specific implementation

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.

mondrake’s picture

FileSize
23.04 KB
22.69 KB

Rerolled, added reference to CR in the deprecation msg, adjusted test to benefit from the $connection property being available from DatabaseTestBase, changed in test the returns to markTestSkipped where necessary.

If it turns back green this should be close to ready now.

Status: Needs review » Needs work

The last submitted patch, 18: 2848820-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

voleger’s picture

Status: Needs work » Reviewed & tested by the community

Looks good.

The last submitted patch, 16: 2848820-16.patch, failed testing. View results

  • catch committed 917a12a on 8.7.x
    Issue #2848820 by jeetendrakumar, andypost, mondrake, vidhatanand:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.7.x, thanks!

Status: Fixed » Closed (fixed)

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