Content modeling overhaul #23

dannylamb · 2018-05-02T16:37:21Z

GitHub Issue: Part of this monstrosity: Islandora/documentation#86

What does this Pull Request do?

Adds an action to generate image derivatives. Packages up default config as a feature.

What's new?

Said action and feature.

How should this be tested?

Tested as part of Islandora/documentation#86

Interested parties

@Islandora-CLAW/committers

whikloj · 2018-05-03T14:33:53Z

src/Plugin/Action/GenerateImageDerivative.php

+    $source_media = $this->utils->getMediaWithTerm($entity, $source_term);
+    $source_field = $this->mediaSource->getSourceFieldName($source_media->bundle());
+    $files = $source_media->get($source_field)->referencedEntities();
+    $file = reset($files);


Field references always confuse me a bit but does this mean each field can only reference a single entity? If so, is this a choice or no other alternatives worked and lastly if so, we should document it so when people create their own objects they don't try to link multiple objects to the same field.

This is a complex pull to review and I'm most likely getting 10% here : so who triggers the derivative here? Comment

// Find media belonging to node that has the source term, and get its file url.

makes understand that the node via a taxonomy term, or it is the media entity via a taxonomy term or both? Since Drupal is Drupal (deep thinking of mine), and media entities can be referenced by any many other ways outside the islandora scope, N nodes to 1 Media entity is possible right?

So a using a taxonomy term on a Node to define if the thumbnail gets generated would make things complicated, assuming node is the trigger. Thinking about how complicated derivative generation on 7.x even on a shallow single tree (one datastream can belong to a single object only) is, since we are already using the inline_entity_form which at least would make user experience good, would it make things easier that derivative is triggered by the media entity itself and bound to it?

and the any node that refers to that media entity gets (if the term for using the derivative is there) the existing derivative? Do i need more coffee?

Yeah, it's a gross PR because of features and shuffling around / deleting tons of config. But you've both zeroed in on the right parts.

This bit in particular pulls out the file that the media is representing. That's why I'm assuming there's only one. This is pretty much the only place where that assumption is made, though. Generally speaking, any entity reference can have multiple values at pretty much any point in time.

@DiegoPino If you wanna see where all that jazz happens, it's in entity hooks in islandora.module: https://github.com/dannylamb/islandora/blob/content-modeling-overhaul/islandora.module#L98

There's a few layers, but basically, in the hook you check to see if you should trigger derivitves at all (e.g. has the file actually changed?). If so it uses Context to evaluate conditions the user provides (e.g. Was it a Media that's a Preservation Master who belongs to an Image node that got updated?) Then it fires a ContextReaction that's a small wrapper around core Drupal actions like our indexing and un-indexing.

The actual generating of the message and publishing it to the queue is what this Action does.

Oh ok, so once you have a media entity we are assuming that each media entity references only one file? I'm okay with that assumption.

whikloj · 2018-05-03T14:36:13Z

src/Plugin/Action/GenerateImageDerivative.php

+  /**
+   * {@inheritdoc}
+   */
+  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {


Should there be a validation that the $form['mimetype'] doesn't have a non-image mimetype in it?

good catch, could probably we a select box to limit the options and the errors

that's a good idea

DiegoPino

I really don't fully understand the logic yet, but added some comments

DiegoPino · 2018-05-03T14:55:43Z

src/Plugin/Action/GenerateImageDerivative.php

+    $source_media = $this->utils->getMediaWithTerm($entity, $source_term);
+    $source_field = $this->mediaSource->getSourceFieldName($source_media->bundle());
+    $files = $source_media->get($source_field)->referencedEntities();
+    $file = reset($files);


This is a complex pull to review and I'm most likely getting 10% here : so who triggers the derivative here? Comment

// Find media belonging to node that has the source term, and get its file url.

makes understand that the node via a taxonomy term, or it is the media entity via a taxonomy term or both? Since Drupal is Drupal (deep thinking of mine), and media entities can be referenced by any many other ways outside the islandora scope, N nodes to 1 Media entity is possible right?

So a using a taxonomy term on a Node to define if the thumbnail gets generated would make things complicated, assuming node is the trigger. Thinking about how complicated derivative generation on 7.x even on a shallow single tree (one datastream can belong to a single object only) is, since we are already using the inline_entity_form which at least would make user experience good, would it make things easier that derivative is triggered by the media entity itself and bound to it?

and the any node that refers to that media entity gets (if the term for using the derivative is there) the existing derivative? Do i need more coffee?

DiegoPino · 2018-05-03T15:02:07Z

src/Plugin/Action/GenerateImageDerivative.php

+
+    $parts = explode('#', $this->configuration['derivative_term_uri']);
+    if (count($parts) > 1) {
+      $name = $entity->id() . ' - ' . $parts[1];


$entity->uuid() to avoid collisions? Should it be the uuid of the media entity?(again, maybe not understanding this)

Good call. We might run into that on multisites.

DiegoPino · 2018-05-03T15:03:23Z

src/Plugin/Action/GenerateImageDerivative.php

+  /**
+   * {@inheritdoc}
+   */
+  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {


good catch, could probably we a select box to limit the options and the errors

whikloj · 2018-05-03T16:14:41Z

Also you are apparently already in need of a rebase. 😆

dannylamb · 2018-05-10T17:31:03Z

@whikloj @DiegoPino Ok, rebasing now. Just putting your review into this comment because they'll get dismissed when I force push.

I will use the uuid instead of the id for the default filename as per @DiegoPino's request.

I will do a form validation on mimetype to make sure its in the image/* family as per @whikloj 's request.

dannylamb · 2018-05-10T19:50:21Z

Ok, so these tests are passing locally, but will fail until Islandora/documentation#86 is merged due to dependencies.

whikloj

I'm good here now. I'll wait till the Islandora PR is merged and re-start Travis to make sure we didn't miss anything.

whikloj · 2018-06-05T13:00:25Z

Restarting travis

whikloj · 2018-06-05T13:10:49Z

@dannylamb you have some code review stuff in here.
https://travis-ci.org/Islandora-CLAW/islandora_image/jobs/382270749#L820

dannylamb · 2018-06-05T14:42:35Z

@whikloj @seth-shaw-unlv Travis is appeased.

whikloj reviewed May 3, 2018

View reviewed changes

DiegoPino reviewed May 3, 2018

View reviewed changes

dannylamb mentioned this pull request May 3, 2018

Derviatives #22

Closed

dannylamb and others added 6 commits May 10, 2018 17:27

Derivatives

cdf6d79

tests

443d60c

updating event for image derivatives

72f1800

updating outgoing message format

4e96a51

Updating composer dependencies

e3a2308

Feature

64100e8

dannylamb force-pushed the content-modeling-overhaul branch from 272f031 to 64100e8 Compare May 10, 2018 17:31

dannylamb added 3 commits May 10, 2018 17:55

responding to feedback

1b6abdb

Updating tests

7ce7dc2

Might want to uncomment out that line...

7bd9931

Shuffling features into islandora_demo

421b754

whikloj approved these changes May 23, 2018

View reviewed changes

Coder

9d93895

whikloj merged commit 4a19d3f into islandora-deprecated:8.x-1.x Jun 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Content modeling overhaul #23

Content modeling overhaul #23

dannylamb commented May 2, 2018

whikloj May 3, 2018

DiegoPino May 3, 2018

dannylamb May 3, 2018

dannylamb May 3, 2018

whikloj May 3, 2018

whikloj May 3, 2018

DiegoPino May 3, 2018

dannylamb May 3, 2018

DiegoPino left a comment

DiegoPino May 3, 2018

DiegoPino May 3, 2018

dannylamb May 3, 2018

DiegoPino May 3, 2018

whikloj commented May 3, 2018

dannylamb commented May 10, 2018

dannylamb commented May 10, 2018

whikloj left a comment

whikloj commented Jun 5, 2018

whikloj commented Jun 5, 2018

dannylamb commented Jun 5, 2018

Content modeling overhaul #23

Content modeling overhaul #23

Conversation

dannylamb commented May 2, 2018

What does this Pull Request do?

What's new?

How should this be tested?

Interested parties

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

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

whikloj commented May 3, 2018

dannylamb commented May 10, 2018

dannylamb commented May 10, 2018

whikloj left a comment

Choose a reason for hiding this comment

whikloj commented Jun 5, 2018

whikloj commented Jun 5, 2018

dannylamb commented Jun 5, 2018