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 |
---|---|---|---|
#17 | drupal-simpletest_result_form_controller-1978982-17.patch | 17.94 KB | ParisLiakos |
#17 | interdiff.txt | 764 bytes | ParisLiakos |
#12 | drupal-simpletest_result_form_controller-1978982-12.patch | 17.93 KB | ParisLiakos |
#12 | interdiff.txt | 6.05 KB | ParisLiakos |
#11 | Convert_simpletest_result_form_to_a_Controller-1978982-11.patch | 18.14 KB | RoSk0 |
Comments
Comment #1
RoSk0Initial patch.
Comment #2
dawehnerThis looks really good so far!
Nitpick alarm :) There should be just one line.
All of them should use {@inheritdoc}
This line could use a module handler, which get's injected into the class. There is ModuleHandlerInterface->loadInclude().
Comment #3
RoSk0New patch version.
Fixed comments and moved simpletest_result_get and simpletest_result_status_image to form controller from pages.inc.
Comment #4
dawehnerThe class needs a short description.
I'm sorry but I probably linked you a bad issue. You should better use the ModuleHandlerInterface + $this->moduleHandler instead.
Should be protected as drupal does never use private. There might be some discussions needed for simpletest_result_get, as this feels like a generic api function, which can be used on different parts. In general the new methods should be camelCased
We could store the map directly on the object instead.
Comment #5
RoSk0Fixed comments.
Comment #6
alexpottNo longer needs to be static... just
protected $map;
also the variable needs proper documentation.now this should just use $this->map
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedalso this should be injected and stored in $this->database (protected).The service is called database.
THen instead of db_select you could do $this->database->select
Comment #8
RoSk0Fixed review comments.
Comment #9
dawehnerIt seems to be that this code is much easier to read, if the setup of these static values would be just moved to the constructor.
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedAssociative
needs docblock
unwanted newlines
lets move this to $form['#attached']['css'] since we are here
Comment #11
RoSk0Another fix.
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commented@RoSk0: thanks a lot for your patience! here is patch, with some more issues i found
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commentedi just rtbc'ed #1978978: Convert simpletest_test_form to a Controller so we could might as well wait for it, so we dont have to do the loadInclude() hack
Comment #14
andypostrender should be called when needed so please provide a helper method and remove this from constructor.
For initial form no reason to render something while there's no results
1) return is not needed here
2) is there other way to get rid of drupal_goto()?
Comment #15
dawehner@andypost
I hope I understood you right in IRC that this is not a real problem. Lazy-do that rendering is really a unneeded optimization.
Return is used by a lot of form controllers, so that is not a problem.
There is also an issue for drupal_goto: #1668866: Replace drupal_goto() with RedirectResponse
Comment #16
andypostYes, +1 to RTBC,
return $form
could be fixed in follow-up #1668866: Replace drupal_goto() with RedirectResponseI just like to see a @todo with link to it
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedwhat about this?:)
Comment #18
dawehnerI'm not sure how your change is intented to work, but if I to go to just /admin/config/development/testing/results/2222222 I'm not redirected
but just the error message is shown. Don't you need an actual POST request in order to make drupal_redirect_form to trigger?
Comment #19
alexpottI agree with #18... just setting #redirect won't do anything until the form is submitted... we should revert to #12.
Re #16 on one hand we can add a @todo but on the other #1668866: Replace drupal_goto() with RedirectResponse should actually convert this so... I'm ok without it.
So committed b307895 and pushed to 8.x. Thanks!
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedyeah..what i was thinking, form_state redirects only work on submit:P
thanks!