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.

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

Add an "input args" parameter when defining derivative actions. #904

Open
wants to merge 6 commits into
base: 2.x
Choose a base branch
from

Conversation

jasonhildebrand
Copy link

  • Other Relevant Links:

Related PRs:

What does this Pull Request do?

This PR adds an "input args" parameter in derivative actions, to be used to pass additional arguments when generating image derivatives. This fields is only exposed in the UI for image derivatives (not for other derivative types), as images are the only use case which needs this additional field.

Background/Motivation

I wanted to use the "density" parameter in imagemagick, but this parameter only works if applied BEFORE the input stream. According to the man page (https://manpages.ubuntu.com/manpages/trusty/man1/convert.im6.1.html) there are several options which are only valid as so-called "input options". The addition of this field makes it possible to use these parameters to adjust the conversion. In particular, the density parameter makes it possible to generate higher-resolution JPGs from a PDF source file.

What's new?

One field is added to the "Edit action" page for image derivatives.

The field should not show for other derivative types (I think I got them all - I followed existing code to remove the field from their configuration forms).

Until the related Alpaca PR is merged, the addition of this field will cause alpaca to spew errors (about the new unknown field).

I think this means some kind of version constraint will need to be applied, but I don't know exactly what/how.

How should this be tested?

It is easier to test these PRs together. See in-depth test plan in Islandora/Alpaca#91

Documentation Status

  • I have added some inline documentation into the Action configuration page (links to imagemagick documentation).

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/committers

…e UI, this field is only exposed for image derivatives, where it allows passing "input options" to imagemagick convert (houdini).
@adam-vessey
Copy link

Instead of introducing this at the AbstractGenerateDerivative level, seemingly only to remove it from all but one extending class, would it make more sense to only implement in the one class? The AbstractGenerateDerivative class could be extended by other modules in the wild, which would then start showing this new option in contexts where it is not desirable.

Also, the addition of new plugin config should be accompanied by an update to the config schema; If this was just hitting the generate_image_derivative bit, then: https://github.com/Islandora/islandora/blob/2.x/modules/islandora_image/config/schema/islandora_image.schema.yml

@jasonhildebrand
Copy link
Author

jasonhildebrand commented Oct 21, 2022 via email

…ativeFile, instead of in AbstractGenerateDerivative. Add utility function to add a form element (or other array entry) at a specific position.
@jasonhildebrand
Copy link
Author

jasonhildebrand commented Oct 24, 2022

I have made the suggested changes, including updating the config schema.

@rosiel
Copy link
Member

rosiel commented Jul 27, 2023

This will be a breaking change as it changes Islandora's "API" (not really defined, but here used to mean "the way it communicates with other systems"). I'd like to include this in the first islandora 3.x release.

@rosiel rosiel added the 3.x label Jul 3, 2024
@rosiel rosiel self-requested a review July 10, 2024 17:37
@jasonhildebrand
Copy link
Author

I resolved the merge conflicts.

@jasonhildebrand
Copy link
Author

I installed phpcs locally, but when I ran it, it produced 600+ errors on IslandoraUtils.php (an order of magnitude more than the CI job showed). Not sure what to do.

Copy link

@adam-vessey adam-vessey left a comment

Choose a reason for hiding this comment

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

RE: coding standards, our CI runs: https://github.com/Islandora/islandora_ci/blob/859a92805dd8cb51646ce30cfc12f30452dbff5c/travis_scripts.sh#L16

Specifically, with the Drupal coding standards.

Additional details about installing the Drupal coding standards might be gleaned from that repo, or it might be more useful to reference Drupal's docs directly: https://www.drupal.org/docs/extending-drupal/contributed-modules/contributed-module-documentation/coder/installing-coder

src/IslandoraUtils.php Outdated Show resolved Hide resolved
@jasonhildebrand
Copy link
Author

RE: coding standards, our CI runs: https://github.com/Islandora/islandora_ci/blob/859a92805dd8cb51646ce30cfc12f30452dbff5c/travis_scripts.sh#L16

Thanks, this is useful.

@jasonhildebrand
Copy link
Author

jasonhildebrand commented Sep 10, 2024

I've pushed a partial de-lint to run these and see if two of the files are now clean - more coming.

I've completed the de-linting, all clean now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants