Comments

dishabhadra created an issue. See original summary.

dishabhadra’s picture

Status: Active » Needs review
StatusFileSize
new19.55 KB

Review my patch.

ashishdalvi’s picture

Hi @Disha,

Deletion of variables are proper. +1 for RTBC.

But patch have few more changes in age_calculator.helper_functions.inc files. Are these changes are scope of issues?

If no, then it should not be a part of patch. If yes, then we can update the issue summary.

Suggestion on age_calculator.helper_function.inc

diff --git a/age_calculator.helper_functions.inc b/age_calculator.helper_functions.inc
...
+function age_calculator_get_results($date1, $date2) {
+  $output = "";
+  $config = \Drupal::config('age_calculator.settings');
+  // Fetch the configuration from variables table.
+  $age_calculator_output = $config->get('age_calculator_output');
+  // If the configrations are not saved by admin, we shall use the default one.
+  if (empty($age_calculator_output)) {
+    // Fetching the defult configuration.
+    $age_calculator_output = age_calculator_default_output_options();
+  }
+  // Fetching defined variables, based on which output will be displayed.
+  $age_calculator_years_months_days = $age_calculator_output['age_calculator_years_months_days'];
+  $age_calculator_months_days = $age_calculator_output['age_calculator_months_days'];

We can create a services for helper function. So it will be more usable.

dishabhadra’s picture

Hi @Ashish,

Only variable deletion was the part of this issue.

Maybe in next release, we can consider the suggestion and try to create the Services for helper function.

dishabhadra’s picture

StatusFileSize
new468 bytes

As I have done the research and found that in D8 no need to write the hook_uninstall for configuration delete.
It will automatically delete configuration when the module is uninstalled.

So I am removing that code and providing a patch.

Review it.

kunalkursija’s picture

Version: 8.x-1.0 » 8.x-1.x-dev
Assigned: dishabhadra » Unassigned
Status: Needs review » Fixed

Thanks for the #5 @dishabhadra.
Have Committed to new 8.x-2.x branch, Will be creating the release soon.

kunalkursija’s picture

Thank you for the contribution guys.
I have added a new release that fixes this issue.
Here is 8.x-2.0 release: https://www.drupal.org/project/age_calculator/releases/8.x-2.0.

Status: Fixed » Closed (fixed)

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