-
Notifications
You must be signed in to change notification settings - Fork 0
use drupal trackchange instead of hw mark property to resolve migrati… #9
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,8 +39,6 @@ class MediaXmlData extends SourcePluginBase implements ContainerFactoryPluginInt | |
| * @var array | ||
| */ | ||
| protected $parsedData = []; | ||
| protected string $eadXmlDir; | ||
| const SAVE_BASE_URI = 'private://findingaid'; | ||
|
|
||
| /** | ||
| * {@inheritdoc} | ||
|
|
@@ -49,15 +47,6 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition | |
| parent::__construct($configuration, $plugin_id, $plugin_definition, $migration); | ||
| $this->entityTypeManager = $entity_type_manager; | ||
| $this->fileSystem = $file_system; | ||
|
|
||
| //handle field uri scheme | ||
| $fieldConfig = \Drupal::entityTypeManager() | ||
| ->getStorage('field_config') | ||
| ->load("media.findingaid.field_media_file"); | ||
|
|
||
| $f_uri_scheme = ($fieldConfig === null) ? 'private' : ($fieldConfig->getSetting('uri_scheme') ?? 'private'); | ||
| $f_sub_dir = ($fieldConfig === null) ? 'findingaid' : ($fieldConfig->getSetting('file_directory') ?? 'findingaid'); | ||
| $this->eadXmlDir = $f_uri_scheme . '://' . trim($f_sub_dir, '/'); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -152,13 +141,6 @@ protected function parseMediaXmlFiles() { | |
| //$file_path = $this->fileSystem->realpath($file_uri); | ||
| $file_changed = $file->getChangedTime(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. file_changed is still referred in parseXmlFile() to capture the value in the source data row (...// Process each item |
||
|
|
||
| // Determine if we should process this media | ||
| $should_process = $this->shouldProcessMedia($media, $file_changed); | ||
|
|
||
| if (!$should_process) { | ||
| continue; | ||
| } | ||
|
|
||
| // Check if it's an XML file | ||
| $mime_type = $file->getMimeType(); | ||
| if (in_array($mime_type, ['application/xml', 'text/xml']) || | ||
|
|
@@ -175,43 +157,6 @@ protected function parseMediaXmlFiles() { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Determine if a media entity should be processed. | ||
| * | ||
| * @param \Drupal\media\Entity\Media $media: media entity | ||
| * @param int $file_changed: file changed timestamp. | ||
| * | ||
| * @return bool: TRUE if should process, FALSE otherwise. | ||
| */ | ||
| protected function shouldProcessMedia($media, $file_changed) { | ||
| // Check if media has an associated node in field_media_of | ||
| if ($media->hasField('field_media_of') && !$media->get('field_media_of')->isEmpty()) { | ||
| $node_id = $media->get('field_media_of')->target_id; | ||
| $node_storage = $this->entityTypeManager->getStorage('node'); | ||
| $node = $node_storage->load($node_id); | ||
|
|
||
| if ($node) { | ||
| $node_changed = $node->getChangedTime(); | ||
|
|
||
| // Process if file timestamp is greater than node timestamp | ||
| // This will trigger a reimport and override the existing node | ||
| if ($file_changed > $node_changed) { | ||
| return TRUE; | ||
| } | ||
|
|
||
| // File hasn't changed since node was last updated, skip | ||
| return FALSE; | ||
| } | ||
|
|
||
| // Referenced node doesn't exist, should process | ||
| return TRUE; | ||
| } | ||
|
|
||
| // No associated node (field_media_of is empty) | ||
| // Always process to create new node | ||
| return TRUE; | ||
| } | ||
|
|
||
| /** | ||
| * Parse an XML file and extract data based on configuration. | ||
| * | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will this addition impact existing migrated items?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked with Claude to consult how drupal track_changes works:
track_changes is a property read by Drupal\migrate\Plugin\migrate\source\SourcePluginBase. When set to true:
For each source row, Migrate computes a hash of the row's data. This hash is stored in the map table's hash column.
On subsequent runs, the newly computed hash for each row is compared against the stored hash.
If they differ, the row is flagged as needing update (MigrateIdMapInterface::STATUS_NEEDS_UPDATE) even if it was already STATUS_IMPORTED, so it gets reprocessed.
If they match, the row is skipped as unchanged.
while the tradeoff or impact I think will be for a very large dataset, it can slow down migration run due to hash.
I also asked Claude in case if we don't use any mechanism to detect change:
Without any mechanism, drupal migration will simply check the map table for the row's source MediaID. If no matching row exists in the map table, it's treated as new and gets imported; while if a matching row exists and its status is STATUS_IMPORTED, the row is skipped, no matter file change in the source (in our case file_changed value).
I think it will be better we add the track_changes property for a clean incremental migration. but there's a performance tradeoff as mentioned above. I am open to your thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran this through Claude as well, as:
The response was essentially:
prepareRow()with areturn FALSE;when there is nothing to doSo: