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.
CreditAttribution: BramDriesen as a volunteer and at Capgemini commented
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.
+++ b/src/Form/hypothesisForm.php
@@ -0,0 +1,173 @@
+ * Class Settings
Not according to coding standards.
+++ b/src/Form/hypothesisForm.php
@@ -0,0 +1,173 @@
+ public function buildForm(array $form, FormStateInterface $form_state)
"Defaults Settings"? Should this be Default without the extra 's'?.
+++ 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.
+++ 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.
+++ 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.
+++ 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.
Comments
Comment #5
rahul.shindeComment #7
rahul.shindeComment #8
amarphule CreditAttribution: amarphule at DevsAdda for DevsAdda commentedPort system_settings_form to ConfigFormBase as per CR https://www.drupal.org/node/1910694
Comment #9
amarphule CreditAttribution: amarphule at DevsAdda for DevsAdda commentedPort system_settings_form to ConfigFormBase as per CR https://www.drupal.org/node/1910694
Comment #10
BramDriesenReview 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.
Not according to coding standards.
Missing PHPDoc.
Comments should be closed with a full stop. (.)
"Defaults Settings"? Should this be Default without the extra 's'?.
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.
Missing full stop.
Should this be hypotesis_visibility ?
Be consistent with writing the module name. Should be with capital.
Is this the correct way to hide a title?
Module name with capital.
Module with capital.
No need to include this function if you only call the parent. Normally this is done automatically by doing the inherit.
Delete.
would be better if you would prefix all the config values with the module I think. e.g. hypothesis_pages instead of pages.
Comment #11
amarphule CreditAttribution: amarphule at DevsAdda for DevsAdda commentedI 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.
Comment #12
BramDriesenCode wise it looks ok. Functionality wise I didn't test this.
Comment #14
rahul.shindeMerged to the dev branch.