Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plopesc’s picture

Status: Active » Needs review
FileSize
8.25 KB

Hello

Attaching patch that converts simpletest_test_form() to a Controller.

Regards

plopesc’s picture

Assigned: Unassigned » plopesc
FileSize
1.86 KB
7.79 KB

Re-rolling improved patch.

Regards.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

hi! Great job, thanks!
i think its good to go:)

ParisLiakos’s picture

Status: Reviewed & tested by the community » Needs work

#1978982: Convert simpletest_result_form to a Controller was committed first, so now we need to fix the @todo there

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion
FileSize
10.64 KB

here it is

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -9,13 +9,12 @@
-class SimpletestResultsForm implements FormInterface, ControllerInterface {
+class SimpletestResultsForm extends SimpletestTestForm implements ControllerInterface {

I'm not sure that this is the right thing to do... and might turn out to be quite brittle... this kind of implies that the SimpletestResultsForm is a special case of the SimpletestTestForm and can share validator's etc... I don't think this is the case. For instance, if for whatever reason we add validation to SimpletestTestForm... then we'll have to add an empty validateForm() method to SimpletestResultsForm

I think in SimpletestResultsForm::submitForm() we should create a new instance of SimpletestTestForm and submit it.

Crell’s picture

I'm inclined to agree with Alex. This doesn't strike me as a "is a special case of" relationship.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
9.77 KB

That's indeed a good idea. Do you think this comment describes what we are doing?

alexpott’s picture

Yep comment looks good to me... will leave for someone else to rtbc so I can commit :)

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Get with the committing, dude!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b8ddc39 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.