Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RoSk0’s picture

Assigned: Unassigned » RoSk0
Status: Active » Needs review
FileSize
7.2 KB

Initial patch.

dawehner’s picture

This looks really good so far!

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,182 @@
+

Nitpick alarm :) There should be just one line.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,182 @@
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::validateForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::submitForm().

All of them should use {@inheritdoc}

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,182 @@
+    module_load_include('inc', 'simpletest' ,'simpletest.pages');

This line could use a module handler, which get's injected into the class. There is ModuleHandlerInterface->loadInclude().

RoSk0’s picture

New patch version.
Fixed comments and moved simpletest_result_get and simpletest_result_status_image to form controller from pages.inc.

dawehner’s picture

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,279 @@
+class SimpletestResultsForm implements FormInterface,  ControllerInterface {

The class needs a short description.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,279 @@
+   * @var \Drupal\Core\Extension\ModuleHandler
...
+  public function __construct(ModuleHandler $module_handler) {

I'm sorry but I probably linked you a bad issue. You should better use the ModuleHandlerInterface + $this->moduleHandler instead.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,279 @@
+  private function simpletest_result_get($test_id) {
...
+  private function simpletest_result_status_image($status) {

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

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,279 @@
+    // $map does not use drupal_static() as its value never changes.
+    static $map;

We could store the map directly on the object instead.

RoSk0’s picture

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,267 @@
+  // $map does not use drupal_static() as its value never changes.
+  private static $map;

No longer needs to be static... just protected $map; also the variable needs proper documentation.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,267 @@
+    if (!isset(SimpletestResultsForm::$map)) {
+      SimpletestResultsForm::$map = array(
+        'pass' => theme('image', array('uri' => 'core/misc/watchdog-ok.png', 'width' => 18, 'height' => 18, 'alt' => t('Pass'))),
+        'fail' => theme('image', array('uri' => 'core/misc/watchdog-error.png', 'width' => 18, 'height' => 18, 'alt' => t('Fail'))),
+        'exception' => theme('image', array('uri' => 'core/misc/watchdog-warning.png', 'width' => 18, 'height' => 18, 'alt' => t('Exception'))),
+        'debug' => theme('image', array('uri' => 'core/misc/watchdog-warning.png', 'width' => 18, 'height' => 18, 'alt' => t('Debug'))),
+      );
+    }
+    if (isset(SimpletestResultsForm::$map[$status])) {
+      return SimpletestResultsForm::$map[$status];
+    }
+
+    return FALSE;

now this should just use $this->map

ParisLiakos’s picture

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,267 @@
+    $results = db_select('simpletest')

also 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

RoSk0’s picture

Status: Needs work » Needs review
FileSize
17.72 KB

Fixed review comments.

dawehner’s picture

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,284 @@
+    if (!isset($this->map)) {
+      $this->map = array(
+        'pass' => theme('image', array('uri' => 'core/misc/watchdog-ok.png', 'width' => 18, 'height' => 18, 'alt' => t('Pass'))),
+        'fail' => theme('image', array('uri' => 'core/misc/watchdog-error.png', 'width' => 18, 'height' => 18, 'alt' => t('Fail'))),
+        'exception' => theme('image', array('uri' => 'core/misc/watchdog-warning.png', 'width' => 18, 'height' => 18, 'alt' => t('Exception'))),
+        'debug' => theme('image', array('uri' => 'core/misc/watchdog-warning.png', 'width' => 18, 'height' => 18, 'alt' => t('Debug'))),
+      );
+    }

It 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.

ParisLiakos’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,284 @@
+   * Assoc array of themed status images keyed by status.

Associative

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,284 @@
+  public function __construct(Connection $database) {

needs docblock

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,284 @@
+  public function buildForm(array $form, array &$form_state, $test_id = NULL) {
+
+    // Make sure there are test results to display and a re-run is not being performed.
...
+  public function validateForm(array &$form, array &$form_state) {
+
...
+  public function submitForm(array &$form, array &$form_state) {
+
...
+  protected function simpletestResultStatusImage($status) {
+

unwanted newlines

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,284 @@
+    // Load all classes and include CSS.
+    drupal_add_css(drupal_get_path('module', 'simpletest') . '/simpletest.css');

lets move this to $form['#attached']['css'] since we are here

RoSk0’s picture

Status: Needs work » Needs review
FileSize
18.14 KB

Another fix.

ParisLiakos’s picture

@RoSk0: thanks a lot for your patience! here is patch, with some more issues i found

  • On test re-run simpletest_test_form_submit() is not loaded which leads to a fatal error
  • Renamed map property to something more describing: statusImageMap
  • Removed simpletestResultStatusImage(), as i no longer see a reason to have it
  • Renamed simpletestResultGet to getResults, no need for prefixing anymore
  • variosu other small fixes
ParisLiakos’s picture

i 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

andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,297 @@
+    // Initialize image mapping property.
+    $this->statusImageMap = array(
...
+        $row[] = $this->statusImageMap[$assertion->status];

render 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

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -0,0 +1,297 @@
+      drupal_goto('admin/config/development/testing');
+
+      return $form;

1) return is not needed here
2) is there other way to get rid of drupal_goto()?

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

@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

andypost’s picture

Yes, +1 to RTBC, return $form could be fixed in follow-up #1668866: Replace drupal_goto() with RedirectResponse
I just like to see a @todo with link to it

ParisLiakos’s picture

what about this?:)

dawehner’s picture

I'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?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I 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!

ParisLiakos’s picture

yeah..what i was thinking, form_state redirects only work on submit:P
thanks!

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