Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of meta-issue #2002650: [meta, no patch] improve maintainability by removing unused local variables
File /core/lib/Drupal/Core/Routing/MatcherDumper.php
Line 121: Unused local variable $txn
Comment | File | Size | Author |
---|---|---|---|
#38 | dumperunusedvar-2067551-38.patch | 1.37 KB | lokapujya |
#35 | interdiff-2067551-28.txt | 1.02 KB | lokapujya |
#34 | dumperunusedvar-2067551-34.patch | 1.38 KB | lokapujya |
#34 | interdiff-2067551-28.txt | 1.36 KB | lokapujya |
#28 | interdiff-2067551-6-27.txt | 1.07 KB | Kartagis |
Comments
Comment #1
jlindsey15 CreditAttribution: jlindsey15 commentedComment #2
phiit CreditAttribution: phiit commentedThis 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.
Comment #3
lokapujyaYes, we still need the transaction.
Comment #4
jlindsey15 CreditAttribution: jlindsey15 commentedSeems good to me.
Comment #5
catchThe 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.
Comment #6
lokapujyaSo, it was an unused variable that should have been used. I'll take a shot at it.
Comment #7
lokapujyaComment #8
KartagisComment #10
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedComment #11
catchComment #13
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedComment #14
YesCT CreditAttribution: YesCT commented@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*)
Comment #16
dawehnerWarning: nitpick review ... this has some trailing whitespace
It would be nice to keep the empty lines as they have been before, because this improved readability.
The catch should be on a new line
Comment #17
lokapujyaComment #18
lokapujyaComment #19
lokapujyawhitespace fix.
Comment #20
lokapujyaComment #21
dawehnerThe $transaction variable actually does not exists.
Comment #22
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedThansk @YesCT for comment.
remove $transaction variable.
Comment #23
dawehnerWait ... so you want to rollback the transaction created before:
so you want to call $txn->rollback() in the catch.
Comment #24
KartagisAnother whitespace fix
Comment #26
YesCT CreditAttribution: YesCT commentedEveryone 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.)
Comment #27
KartagisAnother attempt to make a patch and interdiff
Comment #28
KartagisAnother attempt to make a patch and interdiff.
Comment #29
dawehnerThis change introduces some whitespace
Just remove this line and we are done!
The empty line here is totally fine.
Comment #30
lokapujyaI think we should move this to above the try instead of inside the catch.
Comment #31
lokapujyaAlso, search Core for startTransaction. Everywhere else we assign to the variable name $transaction.
Comment #32
lokapujyaComment #33
lokapujyaComment #34
lokapujyaComment #35
lokapujyaThe interdiff between #28 and #35. Basically, the StartTransction() needed to be moved out of the catch().
Comment #36
lokapujya34: dumperunusedvar-2067551-34.patch queued for re-testing.
Comment #38
lokapujyaRe-rolled.
Comment #39
areke CreditAttribution: areke commentedThe try and catch looks good now, and is being correctly implemented. The patch applies cleanly, so RTBC.
Comment #40
webchickNot a full review, but...
WAT? :) Can we get a more descriptive error message here?
Comment #41
lokapujyaSo, "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?
Comment #42
lokapujyaMaybe the exception should be good enough for a developer reading the error message?
Comment #43
dawehnerMost 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:
Comment #44
catchYep, 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.