Islandora 2136 #95

robyj · 2018-10-19T18:46:04Z

JIRA Ticket: https://jira.duraspace.org/browse/ISLANDORA-2136

Other Relevant Links (Google Groups discussion, related pull requests, Release pull requests, etc.)

What does this Pull Request do?

This pull request adds default values to the book and newspaper variable_get() alls in the install script

What's new?

I pulled the default values from the book and newspaper modules, modified them slightly as the lines after the variable_get calls modify the values anyway

How should this be tested?

A description of what steps someone could take to:
Test by having a system with OCR enabled and book/newspaper modules disabled/missing?

Additional Notes:

Its shouldn't change how the software operates and not sure documentation changes are needed

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/7-x-1-x-committers

- Added default values at the two locations, using values from the book & newspaper modules Resolves: https://jira.duraspace.org/browse/ISLANDORA-2136

- Added default values at the two locations, using default values from the book & newspaper modules Resolves: https://jira.duraspace.org/browse/ISLANDORA-2136

bondjimbond · 2018-10-19T19:02:09Z

Thanks for these new contributions, @robyj. Can you please fill out the PR template (which is now the opening comment on this pull)? It's an important piece for reviewers so that we know what we're looking at and how to test.

robyj · 2018-10-19T19:13:30Z

Comment block at the start of this ticket has been modified. Sorry, wasn't sure how things are done for the project.

whikloj · 2018-10-19T19:32:48Z

islandora_ocr.install

    $book['ocr'] = FALSE;
    variable_set('islandora_book_ingest_derivatives', $book);
  }

  if (module_exists('islandora_newspaper')) {
-    $newspaper = variable_get('islandora_newspaper_ingest_derivatives');
+    $newspaper = variable_get('islandora_newspaper_ingest_derivatives', array('ocr' => FALSE));


@robyj Based on the above change and the default from the newspaper solution pack's admin page I think you should just do that same as above and set

array ( 'pdf' => 'pdf', 'image' => 'image', 'ocr' => FALSE )

@whikloj

- modified the default values for the newspaper array to be the same as the book array, as per @whikloj's comment

whikloj · 2018-10-19T19:41:52Z

FYI @robyj and I both work at the U of Manitoba, so I won't be merging this to avoid any conflicts of interest.

DiegoPino

I have some concerns, not a blocker but something to think about...

DiegoPino · 2018-10-19T20:23:10Z

islandora_ocr.install

+    $book = variable_get('islandora_book_ingest_derivatives', array(
+      'pdf' => 'pdf',
+      'image' => 'image',
+      'ocr' => FALSE));
    $book['ocr'] = FALSE;


Why is ocr being defaulted to FALSE and then set to FALSE again?

Ok, checked code and I know why! but, as my
#95 (comment) says this should be defaulted to array('ocr') to make sure code is more portable.

DiegoPino · 2018-10-19T20:32:37Z

islandora_ocr.install

+    $newspaper = variable_get('islandora_newspaper_ingest_derivatives', array(
+      'pdf' => 'pdf',
+      'image' => 'image',
+      'ocr' => FALSE));
    $newspaper['ocr'] = FALSE;


This also happens here. Now I wonder if the issue is different. Do we have something in islandora_newspaper or 'islandora_book' that helps us set those defaults, quick look tells me this is hardcoded in some form options? What if someone changes those defaults or allows a new one?. Will we ever remember we need to change them here too? So if this piece of code only cares about ocr, would it be not better to default to variable_get('islandora_newspaper_ingest_derivatives', array('ocr')) ? Seems safer... thoughts @Islandora/7-x-1-x-committers

@robyj sorry this seem to conflict with @whikloj review 😭 . Weird enough we do default to array(ocr) in at least some other place... Islandora lacks of consistency sometimes... anyway. Mine are just comments.

Hi, I pulled that default value of just array('ocr') from what seemed to be another place it was being set by default. I'd just like it to be correct.

@robyj totally. I get it!, I also saw it as only array('ocr') somewhere else (zip importer for newspapers?). The issue is, defaulting here makes almost little sense. Defaulting makes sense when load of variable needs to be consistent with the whole expectation and the logic that comes after. In this case you only want to set ocr to false if the variable is set at all right? So i dare to question, why to even save in case the variable was not set? @whikloj thoughts? In any case nothing here is life or dead, but still this needs to A) respect what was set, B) reset the OCR to false C) if nothing was there, better leave it like that.

Those settings of array('pdf' => 'pdf', 'image' => 'image', 'ocr' => FALSE) are just defaults, if the user has ever set their own options that array would be returned instead.

However in this case because we are unsure if we are getting the default or the users setting we have to do the $newspaper['ocr'] = FALSE; a second time.

If that makes this too confusing then perhaps just doing a drupal_map_assoc(array('ocr', 'image', 'pdf')) would be better.

That would match how the default is set in the newspaper solution pack. https://github.com/Islandora/islandora_solution_pack_newspaper/blob/7.x/includes/admin.form.inc#L36-L37

I should have said that we would then still explicitly set the $newspaper['ocr'] = FALSE;

Hi, I pulled that default value of just array('ocr') from what seemed to be another place it was being set by default. I'd just like it to be correct.

@DiegoPino you're thinking something like this?

$newspaper = variable_get('islandora_newspaper_ingest_derivatives', array()); if (isset($newspaper['ocr'])) { $newspaper['ocr'] = FALSE; variable_set('islandora_newspaper_ingest_derivatives, $newspaper); }

Was there any more work for me to do for this?

Sorry. This gone slipped under my radar (radar is off-line, forgot to pay the bill)

DiegoPino · 2019-05-06T13:44:50Z

islandora_ocr.install

+    $newspaper = variable_get('islandora_newspaper_ingest_derivatives', array(
+      'pdf' => 'pdf',
+      'image' => 'image',
+      'ocr' => FALSE));
    $newspaper['ocr'] = FALSE;


This just needs the second 'false' to be removed, right? for newspaper and ocr? @whikloj that was your comment right?

whikloj · 2020-02-21T19:48:32Z

@DiegoPino this is an old one, but wondering if you've had a chance to review it again?

DiegoPino · 2020-02-21T20:15:22Z

@whikloj gosh. time travel. Yes. Ok, trying to catch up with the review.

I feel what you said here is right

$newspaper = variable_get('islandora_newspaper_ingest_derivatives', array());
if (isset($newspaper['ocr'])) {
$newspaper['ocr'] = FALSE;
variable_set('islandora_newspaper_ingest_derivatives, $newspaper);
} That seems to be exactly the best approach.

But that said, i'm also OK with the code as it is (me older == me more tolerant) The fact is it just sets it to false if no ocr module enable. Good enough You got my 👍 for a merge

Sorry @robyj, its me Diego, saying all is good, from the past.

robyj added 2 commits October 19, 2018 13:36

Default variables for all variable_get calls in islandora_ocr

491938b

- Added default values at the two locations, using values from the book & newspaper modules Resolves: https://jira.duraspace.org/browse/ISLANDORA-2136

Default variables for all variable_get() calls

3716d45

- Added default values at the two locations, using default values from the book & newspaper modules Resolves: https://jira.duraspace.org/browse/ISLANDORA-2136

https://jira.duraspace.org/browse/ISLANDORA-2136

2548b6c

whikloj reviewed Oct 19, 2018

View reviewed changes

https://jira.duraspace.org/browse/ISLANDORA-2136

555f8d3

- modified the default values for the newspaper array to be the same as the book array, as per @whikloj's comment

whikloj approved these changes Oct 19, 2018

View reviewed changes

DiegoPino reviewed Oct 19, 2018

View reviewed changes

DiegoPino reviewed May 6, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Islandora 2136 #95

Islandora 2136 #95

robyj commented Oct 19, 2018 •

edited

Loading

bondjimbond commented Oct 19, 2018

robyj commented Oct 19, 2018

whikloj Oct 19, 2018

robyj Oct 19, 2018

whikloj commented Oct 19, 2018

DiegoPino left a comment

DiegoPino Oct 19, 2018

DiegoPino Oct 19, 2018

DiegoPino Oct 19, 2018

DiegoPino Oct 19, 2018

robyj Oct 19, 2018

DiegoPino Oct 19, 2018

whikloj Oct 19, 2018

whikloj Oct 19, 2018

robyj Oct 22, 2018

whikloj Oct 22, 2018

robyj Nov 9, 2018

DiegoPino May 6, 2019

DiegoPino May 6, 2019

whikloj commented Feb 21, 2020

DiegoPino commented Feb 21, 2020

Islandora 2136 #95

Are you sure you want to change the base?

Islandora 2136 #95

Conversation

robyj commented Oct 19, 2018 • edited Loading

What does this Pull Request do?

What's new?

How should this be tested?

Additional Notes:

Interested parties

bondjimbond commented Oct 19, 2018

robyj commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whikloj commented Oct 19, 2018

DiegoPino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whikloj commented Feb 21, 2020

DiegoPino commented Feb 21, 2020

robyj commented Oct 19, 2018 •

edited

Loading