Problem/Motivation

Follow up: https://www.drupal.org/node/2850804

Remaining tasks

  1. Remove calls to \Drupal.
  2. Configurable names should be array to avoid error of in_array() paramater 2 expects array, string given.
  3. Use dependency injection for services.
  4. Use proper getter functions for getting entity bundle ids.
  5. Optimise multiple calls to functions.
  6. Improve readability of code.

Data model changes

  1. Show only configurable fields of entity bundles as options.
CommentFileSizeAuthor
#8 clean_up_QuoraConfigForm.php-2850961-8.patch5.85 KBnavneet0693
#8 interdiff.txt1.17 KBnavneet0693
#6 clean_up_QuoraConfigForm.php-2850961-5.patch5.85 KBnavneet0693
#6 interdiff.txt1.99 KBnavneet0693
#4 clean_up_QuoraConfigForm.php-2850961-4.patch5.84 KBnavneet0693
#2 clean_up_QuoraConfigForm.php-2850961-2.patch5.34 KBnavneet0693

Comments

navneet0693 created an issue. See original summary.

navneet0693’s picture

navneet0693 commented
Status: Active » Needs review
StatusFileSize
new clean_up_QuoraConfigForm.php-2850961-2.patch5.34 KB
navneet0693’s picture

navneet0693 commented
Assigned: navneet0693 » Unassigned

Please review.

navneet0693’s picture

navneet0693 commented
StatusFileSize
new clean_up_QuoraConfigForm.php-2850961-4.patch5.84 KB

Please ignore above patch.

ajits’s picture

ajits
he/him
Primary language Marathi
Location India
commented
Status: Needs review » Needs work

Good job on the clean up! Just little nitpicks before this is ready:

  1. +++ b/src/Form/QuoraConfigForm.php
@@ -12,6 +15,31 @@ use Drupal\node\Entity\NodeType;
+  public function __construct(ConfigFactoryInterface $config_factory, EntityFieldManager $entityFieldManager) {

    The parameters variables passed to the function should not follow camelCase.
    The variable $entityFieldManager can be renamed to $entity_field_manager 

  2. +++ b/src/Form/QuoraConfigForm.php
@@ -22,46 +50,50 @@ class QuoraConfigForm extends ConfigFormBase {
+      '#title' => $this->t('Google Custom Search Api'),

    Capitalize API. 

  3. +++ b/src/Form/QuoraConfigForm.php
@@ -22,46 +50,50 @@ class QuoraConfigForm extends ConfigFormBase {
+      '#description' => $this->t('Provide google cse api to be used my module'),

    Can use proper capitalization. Something like: Provide Google CSE API to be used by module. 

  4. +++ b/src/Form/QuoraConfigForm.php
@@ -22,46 +50,50 @@ class QuoraConfigForm extends ConfigFormBase {
+          // Removes basefields.

    s/basefields/base fields

navneet0693’s picture

navneet0693 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.99 KB
new clean_up_QuoraConfigForm.php-2850961-5.patch5.85 KB
ajits’s picture

ajits
he/him
Primary language Marathi
Location India
commented
Status: Needs review » Needs work

Just one change:

+++ b/src/Form/QuoraConfigForm.php
@@ -12,6 +15,31 @@ use Drupal\node\Entity\NodeType;
+  private $entity_field_manager;

This needs to be in camelCase as it is a class variable.

navneet0693’s picture

navneet0693 commented
StatusFileSize
new interdiff.txt1.17 KB
new clean_up_QuoraConfigForm.php-2850961-8.patch5.85 KB
navneet0693’s picture

navneet0693 commented
Status: Needs work » Needs review
ajits’s picture

ajits
he/him
Primary language Marathi
Location India
commented
Status: Needs review » Reviewed & tested by the community

Thank you!

  • gg24 committed 0c825dd on 8.x-2.x authored by navneet0693 
    Issue #2850961 by navneet0693, AjitS: Clean up QuoraConfigForm.php
gg24’s picture

gg24 commented
Status: Reviewed & tested by the community » Fixed

Thanks everyone for the efforts.

Status: Fixed » Closed (fixed)

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