Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amarphule created an issue. See original summary.

rahul.shinde’s picture

rahul.shinde’s picture

Issue tags: +#d8port
amarphule’s picture

Assigned: amarphule » Unassigned
Status: Active » Needs review
FileSize
10.36 KB

Port system_settings_form to ConfigFormBase as per CR https://www.drupal.org/node/1910694

amarphule’s picture

Port system_settings_form to ConfigFormBase as per CR https://www.drupal.org/node/1910694

BramDriesen’s picture

Status: Needs review » Needs work

Review of #8.
Please include an interdiff between patches. Very difficult to see what changed between patches this way... I guess most of the remarks of #8 still apply.

  1. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    + * Class Settings
    

    Not according to coding standards.

  2. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +  public function buildForm(array $form, FormStateInterface $form_state)
    

    Missing PHPDoc.

  3. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +    // Default settings
    

    Comments should be closed with a full stop. (.)

  4. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +      '#title' => $this->t('Defaults Settings'),
    

    "Defaults Settings"? Should this be Default without the extra 's'?.

  5. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +      '#title' => $this->t('Highlights on by default'),
    

    Not really descriptive on it's own I think. Could be improved and also maybe add a description if you cant make it descriptive enough with the title alone.

  6. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +    // Visibility settings
    

    Missing full stop.

  7. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +    $hypothesis_visibility = $config->get('visibility');
    

    Should this be hypotesis_visibility ?

  8. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +      '#title' => $this->t('Show hypothesis on specific pages'),
    

    Be consistent with writing the module name. Should be with capital.

  9. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +      '#title' => '<span class="element-invisible">' . $title . '</span>',
    

    Is this the correct way to hide a title?

  10. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +      '#title' => $this->t('Show hypothesis for specific content types'),
    ...
    +      '#title' => $this->t('Show hypothesis for specific roles'),
    

    Module name with capital.

  11. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +      '#description' => $this->t('Show hypothesis only for the selected role(s). If you select no roles, hypothesis will be visible to all users.'),
    

    Module with capital.

  12. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +  public function validateForm(array &$form, FormStateInterface $form_state)
    

    No need to include this function if you only call the parent. Normally this is done automatically by doing the inherit.

  13. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +//
    

    Delete.

  14. +++ b/src/Form/hypothesisForm.php
    @@ -0,0 +1,173 @@
    +    $config->set('pages', $form_state->getValue('hypothesis_pages'));
    

    would be better if you would prefix all the config values with the module I think. e.g. hypothesis_pages instead of pages.

amarphule’s picture

Status: Needs work » Needs review
Parent issue: » #3012400: [hypothesis] Hypothesis
FileSize
9.81 KB

I have created a new patch file as per https://www.drupal.org/docs/8/api/configuration-api/working-with-configu... updated suggested points mentioned in comment #10 and removed coding standard errors and warning with the help of phpcs drupal coding standard.

BramDriesen’s picture

Status: Needs review » Reviewed & tested by the community

Code wise it looks ok. Functionality wise I didn't test this.

rahul.shinde’s picture

Status: Reviewed & tested by the community » Fixed

Merged to the dev branch.

Status: Fixed » Closed (fixed)

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