Skip to content

Conversation

@guizzettic
Copy link

@guizzettic guizzettic commented Jan 12, 2026

  • Sync is_vertical from video asset to video in VideoEnricher when mezzanine changes.

@guizzettic guizzettic marked this pull request as ready for review January 26, 2026 18:48
@guizzettic guizzettic requested review from efjacobson and removed request for efjacobson January 26, 2026 18:48
@guizzettic guizzettic changed the title add support for vertical videos v3.0.1 Jan 26, 2026
@guizzettic guizzettic requested a review from efjacobson January 26, 2026 20:22
Comment on lines 61 to 78
// Sync is_vertical when mezzanine_ref changes
$oldMezzanineRef = null;
if ($pbjxEvent->hasParentEvent()) {
$parentEvent = $pbjxEvent->getParentEvent()->getMessage();
if ($parentEvent->has('old_node')) {
$oldMezzanineRef = $parentEvent->get('old_node')->fget('mezzanine_ref');
}
}
$newMezzanineRef = $node->fget('mezzanine_ref');

if ($newMezzanineRef !== $oldMezzanineRef) {
$mezzanineRef = $node->get('mezzanine_ref');
$videoAsset = $this->ncr->getNode($mezzanineRef, false, ['causator' => $pbjxEvent]);

if ($videoAsset::schema()->hasField('is_vertical')) {
$node->set('is_vertical', $videoAsset->get('is_vertical'));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@guizzettic this avoids the awkwardness of fgetting the mezz ref and then later regular getting it. also the second schema check is probably unnecessary, anyone who adopts one will adopt the other

diff --git a/src/Ovp/VideoEnricher.php b/src/Ovp/VideoEnricher.php
index b04dcd6..da38ffb 100644
--- a/src/Ovp/VideoEnricher.php
+++ b/src/Ovp/VideoEnricher.php
@@ -59,23 +59,17 @@ class VideoEnricher implements EventSubscriber, PbjxEnricher
         }

         // Sync is_vertical when mezzanine_ref changes
-        $oldMezzanineRef = null;
         if ($pbjxEvent->hasParentEvent()) {
             $parentEvent = $pbjxEvent->getParentEvent()->getMessage();
-            if ($parentEvent->has('old_node')) {
-                $oldMezzanineRef = $parentEvent->get('old_node')->fget('mezzanine_ref');
-            }
-        }
-        $newMezzanineRef = $node->fget('mezzanine_ref');
-
-        if ($newMezzanineRef !== $oldMezzanineRef) {
-            $mezzanineRef = $node->get('mezzanine_ref');
-            $videoAsset = $this->ncr->getNode($mezzanineRef, false, ['causator' => $pbjxEvent]);
-
-            if ($videoAsset::schema()->hasField('is_vertical')) {
-                $node->set('is_vertical', $videoAsset->get('is_vertical'));
+            if (
+                $parentEvent->has('old_node')
+                && $node->fget('mezzanine_ref') === $parentEvent->get('old_node')->fget('mezzanine_ref')
+            ) {
+                return;
             }
         }
+        $videoAsset = $this->ncr->getNode($node->get('mezzanine_ref'), false, ['causator' => $pbjxEvent]);
+        $node->set('is_vertical', $videoAsset->get('is_vertical'));
     }

     protected function enrichWithCaptionUrls(Message $node): void

Copy link
Author

@guizzettic guizzettic Jan 28, 2026

Choose a reason for hiding this comment

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

@efjacobson Yeah this is much cleaner 👍

return;
}
}
$videoAsset = $this->ncr->getNode($node->get('mezzanine_ref'), false, ['causator' => $pbjxEvent]);
Copy link
Contributor

@efjacobson efjacobson Jan 29, 2026

Choose a reason for hiding this comment

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

@guizzettic the create commands for both the video and the asset are sent at the same time so it's almost guaranteed that the asset won't be available during that enrichment. that's not a problem because the is_vertical will have been set to the same value, but this getNode call will throw. so you should catch NodeNotFound here - if it throws and the parentEvent exists and is a create command, that's fine, but if it is anything else then rethrow

edit: actually, you can probably just check hasNode first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants