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 #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 |
---|---|---|---|
#76 | ajax-1987602-76.patch | 29.52 KB | effulgentsia |
#76 | interdiff.txt | 4.82 KB | effulgentsia |
#74 | ajax-1987602-74.patch | 29.25 KB | tim.plunkett |
#71 | interdiff.txt | 2.01 KB | juampynr |
#71 | drupal-form-ajax-callbacks-to-controller-1987602-71.patch | 29.12 KB | juampynr |
Comments
Comment #1
glennpratt CreditAttribution: glennpratt commentedComment #2
glennpratt CreditAttribution: glennpratt commentedAbusing the test bot with initial patch. Several TODOs on naming and refactoring for Request object.
Comment #3
glennpratt CreditAttribution: glennpratt commentedI merged in #1978892: Convert file_ajax_upload() to a Controller because of a common dependence on ajax_get_form() helper. Then I merged in #1978894: Convert file_ajax_progress() to a Controller because it is a closely related route (upload and progress).
Let me know if I've gone overboard moving ajax_get_form() helper into the controller.
Comment #4
glennpratt CreditAttribution: glennpratt commentedRemoved unused use statement and moved system.ajax into Controller subdir.
Comment #6
glennpratt CreditAttribution: glennpratt commentedWhoops, silly namespace mistake.
Comment #7
glennpratt CreditAttribution: glennpratt commentedComment #8
glennpratt CreditAttribution: glennpratt commentedpdrake asked me to remove the file includes since they don't do anything anymore according to the documentation.
Comment #9
glennpratt CreditAttribution: glennpratt commentedEDIT double submit...
Comment #10
tstoecklerThis looks great overall!!! I have some minor suggestions for improvement:
Here and elsewhere: when specifying a fully namespaced class one should start with a leading backslash, i.e. \Drupal\system\...
I think this should be: "The controller for the route 'system/ajax', \Drupal\...content(), ..."
I'm not sure if, in this case, it should be /system/ajax (with a leading slash), as that is the convention Symfony uses, and by extension Drupal for its route declaration. In the context of paths (not routes) we generally don't add the leading slash.
*to* prevent
In @param and @return as well, the full namespace should start with a backslash.
It seems previously the form build ID was compared with $_POST. I don't know why this was dropped.
I realize this is identical to the previous code, but anyway: Couldn't this use \Drupal\Component\Utility\NestedArray::getValue()?
Again, I think this could also use NestedArray.
Again, it's fine if we leave optimizing this function to a follow-up.
As can be seen in the patch context the pattern should be '/system/ajax', i.e. start with a slash.
I think we usually use a dot (.) for namespacing of route names, i.e. I would suggest file.ajax.upload and file.upload.progress
Comment #11
glennpratt CreditAttribution: glennpratt commentedThanks for the review! I think I addressed each item in this patch.
Comment #12
glennpratt CreditAttribution: glennpratt commentedComment #14
glennpratt CreditAttribution: glennpratt commentedAhh, forgot I didn't review all the class names earlier, this should be better.
Comment #16
glennpratt CreditAttribution: glennpratt commented#14: form-ajax-callbacks-to-controller-1987602-13.patch queued for re-testing.
Comment #18
glennpratt CreditAttribution: glennpratt commented#14: form-ajax-callbacks-to-controller-1987602-13.patch queued for re-testing.
Comment #20
vijaycs85Re-rolling...
Comment #22
glennpratt CreditAttribution: glennpratt commentedThanks for the re-roll vijaycs85. Looks like the fix from #1792032: File validation error message not removed after subsequent upload of valid file was lost.
New patch.
Comment #23
glennpratt CreditAttribution: glennpratt commentedComment #25
glennpratt CreditAttribution: glennpratt commented#22: form-ajax-callbacks-to-controller-1987602-22.patch queued for re-testing.
Comment #26
tstoecklerLooks very good to me. I don't feel smart enough, however, to RTBC.
Comment #27
dawehnerOh #1987712: Convert file_download() to a new style controller there was a similar problem, as the new route system does not allow arbitrary parameters. The workaround was to write a inbound alter subscriber which changes the url before the route system comes into the game. Maybe this would be possible/should be done here as well?
Comment #28
glennpratt CreditAttribution: glennpratt commentedI didn't like that option since it's not really required here, the URL isn't a technical requirement like it might be with image styles.
Comment #29
dawehnerglennpratt told me that nice url are not a requirement here, so no need for this extra stuff.
Should be "Contains \."
Wasn't ahah a concept of just drupal 6. We could update the comment here.
Wrong indentation.
That's a classical examle what should be done in two separate objects ... What about open a follow up?
Contains
also still refer to old stuff.
Can't this one be protected? It's just used in the sub class.
Comment #30
glennpratt CreditAttribution: glennpratt commentedDoes seem like there should be a followup for file_progress_implementation and the if statements to make them OO and pluggable... not sure what it should be called or if that fall under and existing initiative.
Addressed the other comments.
Comment #31
dawehnerUploading a single file works fine, just to be sure.
All callbacks can be skipped.
@param string $key
@return array
Any reason why this MENU_CALLBACK is left? It's not really needed anymore.
Comment #32
glennpratt CreditAttribution: glennpratt commentedI left the hook_menu entries because of #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system.
Comment #33
dawehnerRight, the first two parts though don't have a 'theme callback'.
Comment #34
dawehnerI should concentrate more.
Comment #35
thedavidmeister CreditAttribution: thedavidmeister commentedMinor but
+ $form['#prefix'] .= theme('status_messages');
We're trying to remove theme() calls from core #2006152: [meta] Don't call theme() directly anywhere outside drupal_render() so please don't introduce a new one.
Comment #36
glennpratt CreditAttribution: glennpratt commentedI'm not introducing a new one really, just moving it. The relevant issue doesn't seem to have any progress #2007124: Replace theme() with drupal_render() in authorize.php.
Can you provide the correct code?
Comment #37
tstoecklerYeah, let's postpone that for now. I'm not sure it's such a trivial replacement in this case, as we're adding the themed output directly to $form['#prefix']. At least I'm not entirely sure off-hand how.
Comment #38
thedavidmeister CreditAttribution: thedavidmeister commentedShould do it, but fair enough that the other issue should pick it up.
Comment #39
alexpottNeeds a reroll
Comment #40
tstoecklerSure, we could certainly do #38.
Comment #41
SoumyaDas CreditAttribution: SoumyaDas commentedHi,
I have started working on "Needs reroll"
Thanks,
Soumya Sona Das
Comment #42
glennpratt CreditAttribution: glennpratt commentedGot impatient ssdas, sorry.
Comment #43
dawehner... We could specify even with more detail: JsonResponse.
Comment #44
dawehnerReadd the tags and remove the assignment.
Comment #45
alexpottNeeds a reroll
Comment #46
pwieck CreditAttribution: pwieck commentedworking on reroll of #42 disregarding #43 because I don't have enough info.
Comment #47
pwieck CreditAttribution: pwieck commentedrerolled
Comment #49
pwieck CreditAttribution: pwieck commented#47: form-ajax-callbacks-to-controller-1987602-47.patch queued for re-testing.
Why are many of my rerolls failing like this? Sometimes re-test passes and some times not. Seem like a bug.
Comment #50
thedavidmeister CreditAttribution: thedavidmeister commentedit does seem like a bug. I can't even see what the fatal error is in the testbot logs.
Comment #51
pwieck CreditAttribution: pwieck commentedGood for review!
Comment #52
tstoecklerLet's do this, finally. :-)
Comment #53
star-szrNeeds another reroll unfortunately.
Comment #54
dawehnerComment #56
pwieck CreditAttribution: pwieck commentedRe-rolled again to current head:-)
Comment #57
glennpratt CreditAttribution: glennpratt commentedJust about cross posted with the exact same patch...
Here's my Git branch for people who live in the future.
https://github.com/glennpratt/drupal/tree/1987602-system-ajax-controller
Comment #58
dawehnerActually it is always an ajax|json response. ... It is possible to find a more helpful comment here? (sorry)
Comment #59
juampynr CreditAttribution: juampynr commentedRe-rolling.
Comment #60
juampynr CreditAttribution: juampynr commentedImproved docblocks and fixed route patterns.
Comment #62
juampynr CreditAttribution: juampynr commentedAdded missing blank lines just before the closing parenthesis of the two new classes.
Comment #63
dawehnerThat is looking great now.
Comment #64
juampynr CreditAttribution: juampynr commentedAssigning to myself.
Comment #65
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #66
alexpottLooks like we've changed ajax_form_callback to throw a http exception if we don't have a callback. Whilst this change does make sense and should make Drupal less brittle I think it means we need a test so that we don't accidentally remove this behaviour in the future...
Comment #67
juampynr CreditAttribution: juampynr commentedSure, I added a test to verify that an AJAX form element without callback throws a 500.
Comment #68
dawehnerThe test is looking great.
Comment #69
effulgentsia CreditAttribution: effulgentsia commentedThis looks really good overall. Just some minor feedback. Sorry for downgrading from RTBC, but I think it makes sense to get these quick fixes in with the initial patch.
Let's remove the functions themselves as well then.
These controller methods could all use a more informative 1 line summary, a little more like the functions they replace. Also, the initial verb should end in an "s" (e.g., "Handles").
This test is good, but it only catches 1 of 3 situations. The other 2 are
'#ajax' => array('path' => 'system/ajax')
and'#ajax' => array('callback' => 'SOME_FUNCTION_THAT_DOES_NOT_EXIST')
, and I would argue that these 2 are more likely, so let's include them as well.Comment #70
juampynr CreditAttribution: juampynr commentedGreat feedback, thanks!
This patch addresses all suggestions except the ones for the test, which I will work on in the next patch. In the meantime I want all tests to be run over these changes (see interdiff) and @effulgentsia to confirm the updated method descriptions based on his suggestion.
Now I will review those missing test scenarios and update the test accordingly.
Comment #71
juampynr CreditAttribution: juampynr commentedHere I am adding an extra assertion to test
'#ajax' => array('callback' => 'SOME_FUNCTION_THAT_DOES_NOT_EXIST')
.@effulgentsia, could you give me more background on scenarios of
'#ajax' => array('callback' => 'SOME_FUNCTION_THAT_DOES_NOT_EXIST')
? I do not know how to test it nor why is it related with FormAjaxController->content().Comment #73
juampynr CreditAttribution: juampynr commented#71: drupal-form-ajax-callbacks-to-controller-1987602-71.patch queued for re-testing.
Comment #74
tim.plunkettRerolled for #1999370: Use Symfony Request for file module and #2009014: Replace theme() with drupal_render() in file module.
This blocks #1959574: Remove the deprecated Drupal 7 Ajax API
Comment #75
juampynr CreditAttribution: juampynr commentedThanks for re-rolling @tim.plunkett.
I still need feedback from @effulgentsia at #71.
Comment #76
effulgentsia CreditAttribution: effulgentsia commentedSorry for the delay in responding.
It's related, because that's where we throw the error within this if statement:
if (empty($callback) || !is_callable($callback)) {
, so if we're adding test coverage here for that, then let's add all the relevant conditions that can trigger it.The nonexistent function added in #71 looks good. This just also adds the NULL case, and now that there's 3, puts them into a loop.
This also cleans up a couple docs.
I have no other nits to pick, so if you're happy with these changes, please RTBC.
Comment #77
juampynr CreditAttribution: juampynr commentedThis is ready to go!
Comment #78
tim.plunkett+1
Comment #79
xjmDon't we not translate exception messages?
Comment #80
webchickLooks good to me; xjm is right that we shouldn't be translating those messages, but Alex found a few other places where that's happening, so let's get a follow-up filed to fix it everywhere in core.
Committed and pushed to 8.x. Thanks!
Comment #81
vijaycs85here is the follow up #2055851: Remove translation of exception messages
Comment #83
quietone CreditAttribution: quietone at PreviousNext commentedClosed #1954882: Convert system/ajax to a route as a duplicate and adding credit.