Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jlindsey15’s picture

Status: Active » Needs review
FileSize
681 bytes
phiit’s picture

Assigned: jlindsey15 » Unassigned
Status: Needs review » Needs work

This looks wrong. The patch removes the unused variable as well as the call to the ::startTransaction method. Marking as needs work, correct me if I'm wrong.

lokapujya’s picture

Assigned: Unassigned » lokapujya
Status: Needs work » Needs review
FileSize
725 bytes

Yes, we still need the transaction.

jlindsey15’s picture

Status: Needs review » Reviewed & tested by the community

Seems good to me.

catch’s picture

Title: Remove Unused local variable $txn from /core/lib/Drupal/Core/Routing/MatcherDumper.php » /core/lib/Drupal/Core/Routing/MatcherDumper.php will never roll back its transaction
Priority: Minor » Major
Status: Reviewed & tested by the community » Active

The existing code looks wrong to me. When you start a transaction you should roll it back if something goes wrong, that's not happening here.

lokapujya’s picture

So, it was an unused variable that should have been used. I'll take a shot at it.

lokapujya’s picture

Status: Active » Needs review
Kartagis’s picture

FileSize
620 bytes
620 bytes

Status: Needs review » Needs work

The last submitted patch, patch.patch, failed testing.

sandipmkhairnar’s picture

catch’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, matchdumper-missing-rollback-2067551-10.patch, failed testing.

sandipmkhairnar’s picture

YesCT’s picture

Status: Needs work » Needs review

@Kartagis 's interdiff in #8 looked good. but the patch was not a diff against the 8.x branch, so it was just a repeat of the interdiff.

@sandipmkhairnar in #13 ... I'm not sure what you did differently. are you redoing the patch by hand? Putting a description of the steps you did when posting a comment is good. :) and interdiffs to whatever patch you used as a basis. So maybe you might have posted an interdiff-6-13.txt if you had done something just a bit different based on 6.

the testbot will only run if the status is needs review, so when posting a patch, change it back to needs review. (I forgot that alot. That is why a lot of my patch posts are followed with a comment that just changes the status to needs review. *grin*)

Status: Needs review » Needs work

The last submitted patch, matchdumper-missing-rollback-2067551-13.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
    @@ -119,18 +119,20 @@ public function dump(array $options = array()) {
    +    ¶
    

    Warning: nitpick review ... this has some trailing whitespace

  2. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
    @@ -119,18 +119,20 @@ public function dump(array $options = array()) {
    +      $this->connection->delete($this->tableName)
    +        ->condition('route_set', $options['route_set'])
    +        ->execute();
    +      $insert->execute();
    +      // We want to reuse the dumper for multiple route sets, so on dump, flush
    +      // the queued routes.
    +      $this->routes = NULL;
    

    It would be nice to keep the empty lines as they have been before, because this improved readability.

  3. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
    @@ -119,18 +119,20 @@ public function dump(array $options = array()) {
    +    } catch (\Exception $e) {
    

    The catch should be on a new line

lokapujya’s picture

Status: Needs work » Needs review
FileSize
973 bytes
1.25 KB
lokapujya’s picture

Status: Needs review » Needs work
lokapujya’s picture

lokapujya’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
@@ -120,17 +120,22 @@ public function dump(array $options = array()) {
+      $transaction->rollback();

The $transaction variable actually does not exists.

sandipmkhairnar’s picture

Status: Needs work » Needs review
FileSize
1.14 KB

Thansk @YesCT for comment.

remove $transaction variable.

dawehner’s picture

Wait ... so you want to rollback the transaction created before:

$txn = $this->connection->startTransaction();

so you want to call $txn->rollback() in the catch.

Kartagis’s picture

FileSize
621 bytes
621 bytes

Another whitespace fix

Status: Needs review » Needs work

The last submitted patch, patch.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

Everyone is being very patient on this issue. Thank you.

@Kartagis is trying to figure out why the patch is not being created with git right... so that's ok. lets see if we can help with that.

On another note, #19 got a review.

so @sandipmkhairnar the patch you posted, would be could to have an interdiff between that and the one in 19, so reviewers can follow the changes.
Interdiffs mean you will get quicker reviews. :) I usually name interdiffs with numbers to help explain what they are the diff between. So, @sandipmkhairnar you can make an interdiff and if it is between 19 and 22, you might call it interdiff-19-22.txt (or interdiff-2067551-19-22.txt).

And.. we should try and coordinate with @lokapujya as the issue is assigned to them. (Sorry @lokapujya)
We can go into #drupal or #drupal-contribute and see if they are there also, and ask them if they have any work they want to post first before we start to work on the issue. (Or we can ask here in a comment on the issue.)

Kartagis’s picture

Another attempt to make a patch and interdiff

Kartagis’s picture

Another attempt to make a patch and interdiff.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
    @@ -119,7 +119,7 @@ public function dump(array $options = array()) {
    -    
    + ¶
    

    This change introduces some whitespace

  2. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
    @@ -129,7 +129,8 @@ public function dump(array $options = array()) {
    +      $txn = $this->connection->startTransaction();
    

    Just remove this line and we are done!

  3. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
    @@ -145,5 +146,4 @@ public function dump(array $options = array()) {
    -
     }
    

    The empty line here is totally fine.

lokapujya’s picture

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
@@ -118,19 +118,21 @@ public function dump(array $options = array()) {
+      $txn = $this->connection->startTransaction();

I think we should move this to above the try instead of inside the catch.

lokapujya’s picture

Also, search Core for startTransaction. Everywhere else we assign to the variable name $transaction.

lokapujya’s picture

Status: Needs review » Needs work
lokapujya’s picture

Assigned: Kartagis » lokapujya
lokapujya’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
1.38 KB
lokapujya’s picture

FileSize
1.02 KB

The interdiff between #28 and #35. Basically, the StartTransction() needed to be moved out of the catch().

lokapujya’s picture

The last submitted patch, 34: dumperunusedvar-2067551-34.patch, failed testing.

lokapujya’s picture

Issue summary: View changes
FileSize
1.37 KB

Re-rolled.

areke’s picture

Status: Needs review » Reviewed & tested by the community

The try and catch looks good now, and is being correctly implemented. The patch applies cleanly, so RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Not a full review, but...

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
@@ -109,19 +109,20 @@ public function dump(array $options = array()) {
+      watchdog_exception('Routing', $e);

WAT? :) Can we get a more descriptive error message here?

lokapujya’s picture

So, "Routing" is the category, and since we don't supply a custom message in the third parameter, the information from the exception is used in the message. Are you saying we should provide a custom message?

lokapujya’s picture

Status: Needs work » Needs review

Maybe the exception should be good enough for a developer reading the error message?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Most other places in core where we use watchdog_exception, we don't specify an extra message, given that the exception already has one, especially in transaction codes.

Here is the bit from entity saving:

    catch (\Exception $e) {
      $transaction->rollback();
      watchdog_exception($this->entityType, $e);
      throw new EntityStorageException($e->getMessage(), $e->getCode(), $e);
    }
catch’s picture

Status: Reviewed & tested by the community » Fixed

Yep, the error message in the exception is enough there.

Committed/pushed to 8.x!

Also nice to see a real bug flushed out from the unused variables issues.

Status: Fixed » Closed (fixed)

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