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.
Combines #1987608: Convert ajax_test_dialog_close() to a new style controller, #1987614: Convert ajax_test_error() to a new style controller, #1987616: Convert ajax_test_order() to a new style controller, #1987620: Convert ajax_test_render() to a new style controller.
Part of #1971384: [META] Convert page callbacks to controllers.
Beta phase evaluation
Disruption | Not disruptive, because it merely refactors deprecated callback code into a controller. Reduces fragility by being less of a DX WTF. |
---|
Comment | File | Size | Author |
---|---|---|---|
#63 | 2066445-63.patch | 3.4 KB | andypost |
#62 | interdiff_57_58.txt | 969 bytes | Mile23 |
#58 | 2066445-58.patch | 3.41 KB | tkuldeep17 |
Comments
Comment #1
mparker17Try this...
Comment #2
disasm CreditAttribution: disasm commentedlets add a blank line between each route. Makes it more legible
If you pass $request as an argument to this, these can be accessed via the request object.
Comment #3
mparker17Try this...
Comment #5
mparker17Whoops... that
empty()
language construct gets me every time.Let's try this instead.
Comment #6
mparker17Comment #8
mparker17#5: drupal8.system-module.2066445-5.patch queued for re-testing.
Comment #10
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #11
vijaycs85Re-rolling with current conversion changes...
Comment #13
vijaycs85Seems valid fails... seems test trying to see the js data loaded by drupal_add_js() but this has been changed. Not sure how to proceed further here... may need to update test case?
Comment #14
YesCT CreditAttribution: YesCT commentedneeds a reroll, instructions: https://drupal.org/contributor-tasks/reroll
Comment #15
rahulbile CreditAttribution: rahulbile commentedRe-rolling with current conversion changes.
Comment #17
alvar0hurtad0Rerolled patch
Comment #18
alvar0hurtad0Comment #20
ianthomas_uk#1971384: [META] Convert page callbacks to controllers has this as replacing ajax_test_dialog_close and other callbacks, but they aren't removed by the patch and there are no changes to \Drupal\ajax_test\Controller\AjaxTestController which I'd expect to see.
If this is the right issue to update that class, please can we also correct the docblock for ajax_test_dialog that was missed in #1987606: Convert ajax_test_dialog() to a new style controller
Comment #21
gokulnk CreditAttribution: gokulnk commentedComment #22
tkuldeep17 CreditAttribution: tkuldeep17 commentedComment #23
xjm22: ajax_test_controller-2066445-22.patch queued for re-testing.
Comment #25
xjmComment #26
mparker17Straight re-roll so no interdiff.
Comment #27
mparker17Comment #28
mparker17Also, unassigned.
Comment #31
rpayanmRerolled with a fresh git pull.
Comment #32
Mile23Comment #33
rpayanmComment #34
Mile23Thanks, @rpayanm.
Seems like we just need coding standards for method docblocks here. https://www.drupal.org/coding-standards/docs#functions
Needs @return in the docblock.
Needs @return in the docblock.
Needs @return in the docblock.
Needs @return in the docblock.
Needs @return in the docblock.
Needs @return in the docblock.
Needs @return in the docblock.
Comment #35
rpayanmPlease review.
Comment #36
Mile23Thanks, @rpayanm.
Still needs some coding standards love: https://www.drupal.org/coding-standards/docs#functions
The format should be one line summary, then the big paragraph.
Same here... One line and then supporting paragraph. In this case, just add an empty line after
* Returns an AjaxResponse; settings command set last.
The first line of the docblock should be indented by one space, not two.
Comment #37
Mile23Comment #38
adci_contributor CreditAttribution: adci_contributor commentedPlease review.
Comment #39
Mile23Addresses all the coding standards issues within scope... Great. :-)
Thanks, @adci_contributor and @rpayanm!
Comment #40
Mile23Added beta evaluation.
Comment #41
alexpottLooks good one minor nit...
We should inject the render service into the AjaxTestController.
Comment #42
rpayanmPlease review
Comment #46
rpayanmWhat is wrong with my last patch, why failed?
Comment #47
mparker17@rpayanm If you click on the
[ View ]
link under your patchfile, it will take you to the Drupal QA site where you can see the results of the automated testing: (currently the result is https://qa.drupal.org/pifr/test/946083, but next time the patch gets re-tested, that link will change).Comment #48
rpayanmI check the
[ View ]
link, but I don't know why the testing failed :(Comment #49
valthebaldIn Non-pass section:
Probably array is empty?
Comment #50
DevElCuy CreditAttribution: DevElCuy commentedComment #51
DevElCuy CreditAttribution: DevElCuy commentedRemoved tag by mistake.
Comment #52
aspilicious CreditAttribution: aspilicious commentedThings got easier :)
Comment #53
Mile23It keeps getting smaller and smaller. :-)
Couple things...
@return annotation shouldn't have a variable name. https://www.drupal.org/coding-standards/docs#return
Also just below that method, we see this:
That could use a better docblock, don't you think? Fortunately there's an old crawl of the api docs here, for copypasting ease: https://drupalapi.alphanodes.com/api/drupal/core!core!modules!system!tes...
Note that this docblock is sort of in scope and sort of not in scope. But there you go.
Comment #54
aspilicious CreditAttribution: aspilicious commentedComment #55
Mile23Assuming the testbot passes, I say yay.
Comment #56
alexpottI think you can get the request injected by just doing
public function renderError(\Symfony\Component\HttpFoundation\Request $request)
Comment #57
aspilicious CreditAttribution: aspilicious commentedLet's see, needed a reroll anyway
Comment #58
tkuldeep17 CreditAttribution: tkuldeep17 commentedhi aspilicious,
Just correcting coding standard in patch.
Comment #61
Mile23Comment #62
Mile23Attached is the interdiff for that last patch.
ajax_test.module
has been refactored away.AjaxTestController::renderError()
now accepts injection of the request, in order to get its query and return the properJsonResponse
with anAlertMessage
.Other concerns have been refactored away elsewhere.
Looks good except for one thing....
This isn't actually true any more.
Comment #63
andypostFixed all doc-blocks, suppose patch ready
Comment #64
Mile23Nice. All the docblock concerns are satisfied, and @andypost found more and fixed them.
Comment #65
alexpottTest code changes are not blocked by beta. Committed af1ea7b and pushed to 8.0.x. Thanks!