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.
Convert this page callback to a new-style Controller, using the instructions on http://drupal.org/node/1800686
Comments
Comment #1
larowlanAdding tag
Comment #2
CBPatch attached.
Comment #3
larowlanSome first observations, mostly just nit-picks
Safe to remove
Needs a blank line at end of the file
needs leading \
Can go?
These need to be type hinted 'Connection' and 'ModuleHandler'. Also $moduleHandler should be $module_handler in the arguments.
We seem to be removing the 'Page callback:'
We've been removing the references to hook_menu() too
Comment #4
CBUpdated with larowlans changes.
Comment #6
kim.pepperLooking good!
You need to add "use Drupal\Core\Database\Connection"
use Drupal\Core\Database\Connection
Also, please post an interdiff.txt when making changes between patches, so we can track what has changed. See http://drupal.org/documentation/git/interdiff
Comment #7
kim.pepperYou accidentally added a patch within your patch.
Comment #8
kim.pepperShould program to interfaces instead of class implementations. This should be ModuleHandlerInterface instead.
Indentation of array over 80 chars should follow standards. See http://drupal.org/coding-standards#array
Need a spacing newline before class closing bracket
Comment #9
CBThanks kimb0. Not sure why my sniffer didn't comment on those style issues. I had some issues with larowlan's sniffer mentioning issues that mine didn't. How embarrassment.
Heh.
git add .
failed me it seems. :)Patch and interdiff attached.
Comment #10
kim.pepperAlmost there.
Still need to 'use' ModuleHandlerInterface.
Comment #11
kim.pepperSummoning the bot.
Comment #13
greyrhino CreditAttribution: greyrhino commentedHi, just made the amends as per comment #10.
Passes tests locally so hopefully the bot see's it the same way.
Comment #14
alexpottWe can remove dblog_overview() function and you'll need to update the comment in Drupal\dblog\Tests\DBLogTest that references this function.
Comment #15
greyrhino CreditAttribution: greyrhino commentedThanks - have updated the patch as per comment #14.
Comment #16
alexpottShould be
// Reverse array from \Drupal\dblog\Controller\DBLogController::overview.
But actually what might be nice here is declare this array as the return value of a public static on the controller and then call it from here...
Comment #17
greyrhino CreditAttribution: greyrhino commentedUpdated patch as per comment #16.
Comment #18
greyrhino CreditAttribution: greyrhino commentedRe-updated as per comment #16 to remove unnecessary comment and rename the getLogLevelsClassMap method.
Also including an interdiff this time.
Comment #20
greyrhino CreditAttribution: greyrhino commented#18: Drupal8-Convert-dblog_overview-to-a-controller-1977254-18.patch queued for re-testing.
Comment #21
CBHey greyrhino, you might want to take notice of the 'assigned' field in future eh. Taking over on somebody elses patch is a bit poor form, especially as it wasn't a long time between review and update (ie, issue was far from dead.)
Comment #22
larowlanI think this is ready, great job @cbiggins and @greyrhino
Comment #23
greyrhino CreditAttribution: greyrhino commentedHi cbiggins - really sorry about this!
I was on a Drupal sprint weekend and as it was my first time it was suggested that I finish off this one as it was nearly there and would give me a good understanding of the Symfony routing changes.
Fully understand why you are annoyed about it and next time I will take note of the assigned field to avoid this happening on my next patch.
Comment #24
CBIts all good mate!
I spoke to Alex today and he told me the situation.
Between us we got it done. :)
Comment #25
alexpottPatch needs a re-roll :)
And whilst you are at it...
Should be alphabetical...
Comment #26
CBI was just about to do this, but it might be handy and helpful for greyrhino to re-roll?
Comment #27
greyrhino CreditAttribution: greyrhino commentedHappy to make this change - but to confirm is the re-roll just rebuilding the patch file (with an interdiff) to include the alphabetical listing change shown above?
Just wanted to check as you say 'Whilst you are at it' as though there might be more to a re-roll than just that.
Comment #28
alexpott@greyrhino the patch no longer applies to head... so a re-roll is to sort that out... see https://drupal.org/patch/reroll
Comment #29
greyrhino CreditAttribution: greyrhino commentedHi Alex/CBiggins,
There seemed to be nothing wrong with the old patch file - it patched cleanly against a clean checkout of 8.x and all dblog tests passed - so I've made the amend for alphabetical import listing and rebuilt the patch with an interdiff.
Is this all that you would usually do for a re-roll - if the tests come back with nothing?
Cheers
James
Comment #30
CBLooks good to me. Test passed, so it applies cleanly.
Will wait for somebody else to RTBC.
Comment #31
ParisLiakos CreditAttribution: ParisLiakos commentedname should be DbLogController, see http://drupal.org/node/608152#naming point 3
Needs documentation for the parameters
lets fix the weird spacing here. just one space needed
also weird identation.
should be two spaces
Comment #32
greyrhino CreditAttribution: greyrhino commentedSorry, just seen this - will pick it up hopefully tonight.
Comment #33
greyrhino CreditAttribution: greyrhino commentedAttached is a new patch file with interdiff.
The only thing that I haven't changed from comment #31 is the class name - the reason for this is that there are existing classes in 8.x that are named this way - so with DBLog in their name rather than DbLog:
core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/DBLogResource.php
core/modules/rest/lib/Drupal/rest/Tests/DBLogTest.php
If it is a must fix then I am happy to make the changes to this patch.
Comment #34
ParisLiakos CreditAttribution: ParisLiakos commentedthose names are wrong then, we should fix them some time, but at least let the new classes have the correct name in the dblog module.
more people will check there than the rest module, if they are wondering about how to name something:)
Comment #35
greyrhino CreditAttribution: greyrhino commentedI've updated the patch to include the name change for controller class and test class.
Comment #36
ParisLiakos CreditAttribution: ParisLiakos commentedcool, thanks! but
please include
-C -M
in your git diff arguments to keep the patch size down.Comment #37
ParisLiakos CreditAttribution: ParisLiakos commentedhere it is
also postponed
#1987686: Convert dblog_event() to a new style controller and #1982954: Convert dblog_top() to a controller to this one
Comment #38
dawehnerMissing empty line ...
Needs @return
Contains
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentedah, thanks totally missed those:)
Comment #40
dawehnerI would like to give it a manual try but i'm to tired to trust myself now.
Dismissed some prodecurals which could call the classes directly.
Comment #41
ParisLiakos CreditAttribution: ParisLiakos commentedoh you are right, how did i miss that..no alternative for filter_xss though
fixed some other doc stuff as well
Comment #43
ParisLiakos CreditAttribution: ParisLiakos commented#41: Drupal8-Convert-dblog_overview-to-a-controller-1977254-41.patch queued for re-testing.
Comment #44
dawehnerPlayed a bit with the dblog listing and both filtering and clearing still worked.
Comment #45
alexpottCommitted fdfad94 and pushed to 8.x. Thanks!