Just nit refactoring.

CommentFileSizeAuthor
#6 2966488-6-diff-w.txt20.16 KBAnonymous (not verified)
#6 interdiff-2-6.txt9.61 KBAnonymous (not verified)
#6 2966488-6.patch24.12 KBAnonymous (not verified)
#2 interdiff-refactoring.txt20.84 KBAnonymous (not verified)
#2 2966488-2.patch24.87 KBAnonymous (not verified)

Comments

Anonymous’s picture

Anonymous (not verified) commented

vaplas created an issue. See original summary.

Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Needs review
StatusFileSize
new 2966488-2.patch24.87 KB
new interdiff-refactoring.txt20.84 KB

There are a couple of minor ideas on how to change image_field_repair.inc. Changes do not bring significant improvements, so you can ignore them if laziness. Or use only partially.

The interdiff-refactoring.txt is patch with diff -w for more readability.

Review:

  1. +++ b/image_field_repair.inc
@@ -25,37 +24,51 @@ use Drupal\image_field_repair\ImageSizer;
-    $sandbox['#finished'] = $sandbox['total'] !== 0 ? $sandbox['processed'] / $sandbox['total'] : 1;
+    $sandbox['#finished'] = $total === 0;
...
+    return $sandbox['#finished'] ? t('Not found any image records') : NULL;

    A slight simplification + message for site without images. 

  2. +++ b/image_field_repair.inc
@@ -25,37 +24,51 @@ use Drupal\image_field_repair\ImageSizer;
+  return image_field_repair_update_result($sandbox);

    Decouple report in a separate function. 

  3. +++ b/image_field_repair.inc
@@ -69,7 +82,7 @@ function image_field_repair_process_batch(&$sandbox) {
-    $sandbox['#finished'] = $sandbox['processed'] / $sandbox['total'];
+  $sandbox['current_range_start'] += $processed;

@@ -89,26 +102,18 @@ function image_field_repair_process_batch(&$sandbox) {
+  $sandbox['#finished'] = $sandbox['processed'] / $sandbox['total'];
...
-    else {
-      // Not yet at the end of this table: proceed to next batch size range.
-      $sandbox['current_range_start'] += $processed;

    Just a little simplified logic. 

  4. +++ b/image_field_repair.inc
@@ -166,221 +167,175 @@ function image_field_repair_process_table_range(array $table_info, $rangeStart,
+function image_field_repair_get_all_table_infos() {
...
+      $tables[] = image_field_repair_get_field_table($field, $table_mapping);
+      $tables[] = image_field_repair_get_field_table_revision($field, $table_mapping, $target_entity_type_id);
...
+function image_field_repair_get_table_info(FieldStorageConfigInterface $field, DefaultTableMapping $table_mapping, $table_name, $primary_keys) {

    5. The biggest change :)

    The idea is that instead of a one function with $dataOrRevision flag, use two special functions + one general function.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented
Status: Needs review » Needs work 
  1. +++ b/image_field_repair.inc
@@ -25,99 +24,104 @@ use Drupal\image_field_repair\ImageSizer;
-    $sandbox['field_ids'] = image_field_repair_get_image_field_ids($entityTypeManager);
-    $sandbox['table_infos'] = image_field_repair_get_all_table_infos($sandbox['field_ids'], $entityTypeManager, $database);
+    list($field_ids, $table_info) = image_field_repair_get_all_table_infos();
+    $total = array_sum(array_column($table_info, 'record_count'));

    I don't see much advantage in combining this into 1 function, I find the list() even less readable. 

  2. +++ b/image_field_repair.inc
@@ -25,99 +24,104 @@ use Drupal\image_field_repair\ImageSizer;
+    $sandbox['#finished'] = $total === 0;

    #finished is a number, a fraction between 0 and 1 indicating the percentage completed, it is not a bool. See e.g. #2141221: hook_update_N() should document how to use $sandbox['#finished']. Let's revert all logic around #finished. 

  3. +++ b/image_field_repair.inc
@@ -25,99 +24,104 @@ use Drupal\image_field_repair\ImageSizer;
+  return image_field_repair_update_result($sandbox);

    Extracting the reporting is a good idea! 

  4. +++ b/image_field_repair.inc
@@ -126,21 +130,19 @@ function image_field_repair_process_batch(&$sandbox) {
+  $database = \Drupal::database();

    I am not sure about this. I think, I prefer to have 1 place to get service classes and from there on pass them around using parameters.

    Putting the other parameters and results in the $batch parameter is a good idea though!

Anonymous’s picture

Anonymous (not verified) commented

#3.1 Full agree.
list($field_ids, $table_info) is not an improvement here, it is rather a side effect :(
I just wanted to hide preparation detalis:

$sandbox['field_ids'] = image_field_repair_get_image_field_ids(...
$sandbox['table_infos'] = image_field_repair_get_all_table_infos(...

But I did not succeed, and now this part has turned into spaghetti. Will be reverted.

#3.2:
If this should be a number, how about a casting?

- $sandbox['#finished'] = $sandbox['total'] !== 0 ? $sandbox['processed'] / $sandbox['total'] : 1;
+ $sandbox['#finished'] = (int) $sandbox['total'] === 0;

It just seems to me that here is quite a complicated logic for the initialization phase. Because $sandbox['processed'] is always 0. So, we need check only $sandbox['total'].

#3.3: Thanks!

#3.4: One place is really makes sense. Will be reverted.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented

RE #4:3.2: Originally it may have been outside the if and the else, but as it is now, in the if part it indeed doesn't make sense, so let's change it to:
$sandbox['#finished'] = $sandbox['total'] === 0 ? 1 : 0;
(I don't want to know (or depend on) how booleans are cast to int)

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review
StatusFileSize
new 2966488-6.patch24.12 KB
new interdiff-2-6.txt9.61 KB
new 2966488-6-diff-w.txt20.16 KB

#5: Even better! Done.

  • fietserwin committed cb9b0eb on 8.x-1.x authored by vaplas 
    [#2966488] by vaplas, fietserwin: Refactoring image_field_repair.inc.
fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented

Committed with some minor changes.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Fixed

I compared the some minor changes - they all absolutely make sense! Thanks again, @fietserwin!

Also I have been testing the module with this refactoring for a long time. Everything works like a charm!

return $sandbox['#finished'] === 1 ? t('Not found any image records') : NULL;
I had a funny case, because of which this is really useful message. When I tried the module on one internal site (for employees), I found that pressing the "Start" button does not lead to any effects - only page relaod. I was very confused, because site contains images.

But debug explained to me that all of them are just part of ckeditor fields in nodes))

So the module worked really well. Just did not bring out the reasons for which so quickly coped :)

@fietserwin, do you have a plan to create a new release? Now the usage part of the module page is oriented to the dev version. But usually everyone downloads the release version.

Status: Fixed » Closed (fixed)

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