Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Islandora 2136 #95

Open
wants to merge 4 commits into
base: 7.x
Choose a base branch
from
Open

Islandora 2136 #95

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
491938b
Default variables for all variable_get calls in islandora_ocr
robyj Oct 19, 2018
3716d45
Default variables for all variable_get() calls
robyj Oct 19, 2018
2548b6c
https://jira.duraspace.org/browse/ISLANDORA-2136
robyj Oct 19, 2018
555f8d3
https://jira.duraspace.org/browse/ISLANDORA-2136
robyj Oct 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions islandora_ocr.install
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,16 @@ function islandora_ocr_enable() {
*/
function islandora_ocr_cleanup_derivatives() {
if (module_exists('islandora_book')) {
$book = variable_get('islandora_book_ingest_derivatives');
$book = variable_get('islandora_book_ingest_derivatives', array(
'pdf' => 'pdf',
'image' => 'image',
'ocr' => FALSE));
$book['ocr'] = FALSE;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

$newspaper['ocr'] = FALSE;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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);
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

variable_set('islandora_newspaper_ingest_derivatives', $newspaper);
}
Expand Down