Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Part of #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#37 | drupal-1987686-37.patch | 10.6 KB | dawehner |
#33 | drupal-dblog-event-1987686-33.patch | 5.23 KB | kmoll |
#23 | 1987686-dblog_event-controller-23.patch | 7.56 KB | Anonymous (not verified) |
#21 | 1987686-dblog_event-controller-21.patch | 7.58 KB | Anonymous (not verified) |
#19 | 1987686-dblog_event-controller-19.patch | 4.21 KB | Anonymous (not verified) |
Comments
Comment #1
marcingy CreditAttribution: marcingy commentedComment #2
marcingy CreditAttribution: marcingy commentedComment #3
ParisLiakos CreditAttribution: ParisLiakos commentedthanks, this looks mostly good, some notices:
i think this is not needed right?
i believe the class name should be DbLogController see http://drupal.org/node/608152#naming point 3
the empty string part is no longer true, right?
Comment #4
sidharthapComment #6
CBPlease note also that this needs to work with the changes in #1982954: Convert dblog_top() to a controller and #1977254: Convert dblog_overview() to a Controller.
It might be worth waiting until those two have been committed before working any more on this, otherwise we'll end up with a big dependency mess.
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedlets wait for this #1977254: Convert dblog_overview() to a Controller
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commentedComment #9
vijaycs85Re-rolling...
Comment #11
stella CreditAttribution: stella commentedPatch changes a few things:
Comment #13
stella CreditAttribution: stella commented#11: 1987686-dblog_event-controller-11.patch queued for re-testing.
Comment #14
dawehnerJust for codestyle reasons let's use $event_id
I am a bit confused about the test changes. Doesn't the test ensure that there are logs created for login in a user etc. so the test below should not use the new generated log entry.
Comment #15
stella CreditAttribution: stella commentedThe original test was checking for an event with event_id = 1, however that event didn't exist whenever I tested it. That particular test is to ensure that the event details page is working I think, but this code change probably isn't needed as the logging in of the user would generate an entry.
Comment #16
stella CreditAttribution: stella commentedPatch reload to change eventId to event_id, and to remove the unneeded call to generateLogEntries().
Comment #17
dawehnerCool
Comment #18
alexpottuse String::checkPlain()
Needs reroll due to #2008990: Replace theme() with drupal_render() in dblog module - we need to make sure we replace all the calls to theme with drupal_render or a render array...
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedReroll the #16 patch and added the #18 suggestions.Please needs review
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedneeds a newline here
extra spaces should be removed
needs public visibility
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedReroll the patch and removed whitespaces also.Please needs review
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedlooks ready to go. Just one small thing:
we remove those now, since controllers dont have much to do with menus
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedSorry I missed this small thing.Added the #22 suggestions.Please needs review
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedthank you!
Comment #26
ParisLiakos CreditAttribution: ParisLiakos commented#23: 1987686-dblog_event-controller-23.patch queued for re-testing.
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedComment #28
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #29
YesCT CreditAttribution: YesCT commented@naveenvalecha
It's not required, but it is helpful to make interdiffs.
interdiff?
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff | Microbranching workflow: http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...
Keep that in mind for future. :)
Comment #30
Anonymous (not verified) CreditAttribution: Anonymous commented@YesCT
Thanks for your suggestions regarding the interdiff. I know regarding the interdiff but the #19 patch has the space errors.So that's why I not created the interdiff.
Comment #31
alexpottNeeds a reroll
Like @dawehner I don't get why this change is necessary
Comment #32
kmoll CreditAttribution: kmoll commentedComment #33
kmoll CreditAttribution: kmoll commentedre rolled against head
Comment #34
kmoll CreditAttribution: kmoll commentedComment #36
kmoll CreditAttribution: kmoll commentedComment #37
dawehnerThis fixes the failures, removes the old code and fixes some small points.
Comment #38
vijaycs85This patch applies fine and looks good to me. +1 for RTBC.
Comment #39
dawehner#37: drupal-1987686-37.patch queued for re-testing.
Comment #40
Crell CreditAttribution: Crell commentedThere's actually quite a bit more refactoring that can/should be done here, but this at least gets the page out of the old router so let's take care of that first, as that's the release-blocking part. :-) Thanks all.
Comment #41
catchCommitted/pushed to 8.x, thanks!