Bug: identify is not running on Draft ADOs

[DiegoPino]

Bug

.. and because of that, since the as:structure does get filled up, it never runs, even after publishing. This feels like a regression or a bad mad If() condition. Needs fix before 1.0.0-RC1 is made public
@dmer @alliomeria will fix today

[alliomeria]

@DiegoPino, I could not duplicate this issue in any Beta 3 instances I tested/checked.

[DiegoPino]

@alliomeria thanks so much for testing. Clearly its a thing I 🤕 introduced in the most recent code. This helps a lot.

[DiegoPino]

@alliomeria this may be the most stupid bug/not bug ever.

I tested and tested and could not replicated except on our 1.0.0-RC1 demo of today.
Look. Right is the stored/deployed config
Left. The config after pressing the config form for basic metadata extraction

So where is the issue?

WE "tried" to give the form "defaults" which does not mean that when you are seeing the form the values are actually saved!
The user has no way of knowing that the defaults there are not persisted and are only "illustrative".

DOUBLE MEGA FACEPALM

Of course previously processed files won't get (as the code is) any new tech metadata because we assumed them processed so you can save and save and nothing.

So. Solutions are:

• DO not put any defaults? Or put them as suggested (like in the background of the textfields but not filled up as values)
• IF we put defaults as they are Mark the ones that are not saved yet "as not stored"? Can be complex to explain

Plus:

actually ship with the settings saved... I did not....

strawberryfield.filepersister_service_settings

[patdunlavey]

Wouldn't the normal approach here be:

1. In the functions that depend on this value being set, where it has not been set, try to use a hard-coded default.
2. On the configuration form, pre-fill the default (same value as 1) if it has not been previously saved.

[DiegoPino]

@patdunlavey thanks. Yes, in Drupal 7 that used to be the most common approach, but because of how config entities work on Drupal 8 (and config sync) that makes consistent checking on defaults a nightmare across multiple modules to manage and also obscures certain operation to the user. In this case I feel we may want to if something is not saved make sure people see it as not saved and also skip the operation of course.

Also, E.g Configs can be overridden by other modules but also via configurations YML and the actual $config->get() method does not allow for a default so making sure defaults would be applied when calling would mean a bit of extra code?

Its actually something we may want to check all around archipelago, where we are using defaults/ v/s where we need to act/exclude based on presence not presence.

Wonder if it would help to have some type of permission by User role that exposes all operations that where executed to the UI.
Things like "hey we did not run Identify because its empty". It could be a general method.

If you have some ideas/preferences about this I'm open to apply changes to your config system forms. Or if this is something you feel you want to tackle even better!

Thanks again!!

PS: https://github.com/esmero/strawberryfield/blob/1.0.0-RC1/src/Form/FilePersisterServiceSettingsForm.php#L105

[patdunlavey]

I'm afraid my lack of D8 knowledge is showing. Is there no way to define a default value in configuration, e.g. in the web/modules/contrib/strawberryfield/config/schema/strawberryfield.schema.yml file?

    identify_exec_path:
      type: string
      label: 'Identify binary full executable path (graphics magic or Imagemagick)'
      default:'/usr/bin/identify'

[patdunlavey]

Ok, one baby step up the learning curve, thanks for your patience... So these configuration values are set in config/sync/strawberryfield.filepersister_service_settings.yml. How did the values that it started out with get there if the File Persister Service Settings form was never saved?:

extractmetadata: true
exif_exec_path: /usr/bin/exiftool
fido_exec_path: /usr/bin/fido

Can we simply add the missing defaults to this during install?

extractmetadata: true
exif_exec_path: /usr/bin/exiftool
fido_exec_path: /usr/bin/fido
identify_exec_path: /usr/bin/identify
pdfinfo_exec_path: /usr/bin/pdfinfo

[patdunlavey]

Can we just modify this? https://github.com/esmero/archipelago-deployment/blob/8.x-1.0-beta3/config/sync/strawberryfield.filepersister_service_settings.yml Or is that a bad idea somehow?

[DiegoPino]

Yes. Exactly! That is the simplest approach for the default deployment, it just needs to be in the RC1 branches. Want to make a pull to 1.0.0-RC1 and 1.0.0-RC1D9?
Would be your first contribution and a reason to celebrate!

[DiegoPino]

That way specs of that particular Docker environment don't get fixed into the code. =)

[patdunlavey]

@DiegoPino Can/should this issue be closed now that esmero/archipelago-deployment#82 has been closed? Or is there more that you think should be followed up on to handle cases where strawberryfield is not deployed using archipelago-deployment? If so, perhaps that should get an issue that is more broadly defined?

[DiegoPino]

Thanks! I will keep this open until I figure out how to tell the user (generically) if some value is missing but still show the config. Thinking of use cases like @giancarlobi where all is setup manually basically.

[DiegoPino]

Closing as fixed.