SUP-7358 : Adding OaiMetadataMap & DssiNodeAlter #5
Conversation
WalkthroughAdds a Migration Alter plugin that mutates the dssi_node migration process pipeline, introduces an OAI metadata map plugin (PcStandard) with paragraph-to-Dublin-Core mappings, and updates module and Composer manifests to declare/require additional dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Plugin/OaiMetadataMap/DublinCore.php(1 hunks)src/Plugin/dgi_migrate_alter/spreadsheet/DssiNodeAlter.php(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Code Linting
src/Plugin/dgi_migrate_alter/spreadsheet/DssiNodeAlter.php
[error] 19-19: Opening brace should be on the same line as the declaration (Drupal.Classes.ClassDeclaration.BraceOnNewLine). PHPCS can fix automatically with PHPCBF.
[error] 25-25: Opening brace should be on the same line as the declaration (Drupal.Functions.MultiLineFunctionDeclaration.BraceOnNewLine). PHPCS can fix automatically with PHPCBF.
🔇 Additional comments (3)
src/Plugin/dgi_migrate_alter/spreadsheet/DssiNodeAlter.php (1)
26-38: LGTM!The migration process configuration correctly chains
skip_on_emptyandexplodeplugins for conditional field processing.src/Plugin/OaiMetadataMap/DublinCore.php (2)
1-22: LGTM!The plugin annotation and class structure are correctly configured for an OAI Dublin Core metadata map.
30-64: Verify the field name on line 32 matches actual Drupal field definitions.The field name
'field_topic_general_subdivision_'on line 32 has a trailing underscore, which is inconsistent with the truncation pattern of other field names on lines 33-34 (field_temporal_chronological_sub,field_geographic_geographic_subd).Since field definitions are not present in this repository, verify that this field name matches the actual machine name of the Drupal field in your installation. The trailing underscore may be a typo, or it could be the correct field name if your field was intentionally named with a trailing underscore.
| 'field_origin_information' => [ | ||
| 'field_date_created' => 'dcterms:created', | ||
| 'field_date_issued' => 'dcterms:issued', | ||
| 'field_date_note' => 'dcterms:date', |
There was a problem hiding this comment.
Mapping overriden to include field_date_note
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Plugin/OaiMetadataMap/PcStandard.php(1 hunks)
🔇 Additional comments (3)
src/Plugin/OaiMetadataMap/PcStandard.php (3)
1-6: LGTM!The file header, namespace, and import statement follow Drupal coding standards.
30-64: Verify field names against your Drupal field configuration.The field names in this mapping, particularly
'field_topic_general_subdivision_'on line 32, could not be verified in the codebase search. The trailing underscore is unusual and warrants confirmation. Please verify that all field names in thePARAGRAPH_MAPPINGconstant match the actual field names defined in your Drupal configuration, especially the one with the trailing underscore and the truncated field names like'field_temporal_chronological_sub'and'field_geographic_geographic_subd'.
22-23: Verify the dgi_standard_oai module documentation to confirm parent class usage.The parent class DgiStandard is from an external Drupal module (
dgi_standard_oai) which is not available in this repository. The web search found no public documentation for this module, suggesting it's a private or custom implementation. You'll need to:
- Check the dgi_standard_oai module's DgiStandard class to confirm it actually references the PARAGRAPH_MAPPING constant
- Review whether the constant override pattern is supported in that module's design
The pattern of defining a constant in a child class for parent usage is valid OOP design, but only the module maintainers can confirm this specific implementation follows that contract.
| /** | ||
| * OAI implementation for the PC metadata profile. | ||
| * | ||
| * @OaiMetadataMap( | ||
| * id = "pc_standard_oai", | ||
| * label = @Translation("DPLAVA (PC)"), | ||
| * metadata_format = "mdRecord", | ||
| * template = { | ||
| * "type" = "module", | ||
| * "name" = "rest_oai_pmh", | ||
| * "directory" = "templates", | ||
| * "file" = "oai-default" | ||
| * } | ||
| * ) | ||
| */ |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the metadata format and template configuration.
The metadata_format is set to "mdRecord", which is non-standard for OAI-PMH (typical formats include "oai_dc", "oai_marc", etc.). Additionally, verify that the template file "oai-default" exists in the rest_oai_pmh module at the specified location.
Run the following script to verify the template reference:
🏁 Script executed:
#!/bin/bash
# Description: Verify the template file exists in rest_oai_pmh module
# Search for the template file
fd -t f "oai-default" --full-path --exec echo "Found: {}"
# Search for rest_oai_pmh module usage
rg -n "rest_oai_pmh" --type=php -C2Length of output: 354
🏁 Script executed:
#!/bin/bash
# Search for template files with common extensions
echo "=== Searching for template files with common extensions ==="
fd -t f "oai" --exec file {} \; | grep -i "template\|text"
# Search for any files named "oai-default" with any extension
echo -e "\n=== Searching for oai-default with any extension ==="
fd "oai-default" -t f
# Search for rest_oai_pmh module/directory
echo -e "\n=== Searching for rest_oai_pmh directory or module ==="
fd "rest_oai_pmh" -t d
# Look at similar OaiMetadataMap plugins for comparison
echo -e "\n=== Checking other OaiMetadataMap plugins ==="
fd -t f ".*Standard\.php$" -path "*/OaiMetadataMap/*"Length of output: 385
🏁 Script executed:
#!/bin/bash
# Find all OaiMetadataMap plugin files
echo "=== All OaiMetadataMap plugin files ==="
fd -t f -path "*/OaiMetadataMap/*.php"
# Check if there's a modules or contrib directory
echo -e "\n=== Checking for external modules directories ==="
fd -t d "modules\|contrib" -d 2
# Look for any configuration files that might reference rest_oai_pmh
echo -e "\n=== References to rest_oai_pmh in entire codebase ==="
rg "rest_oai_pmh" -n
# Check for template directories
echo -e "\n=== Template directories in repo ==="
fd -t d "templates"Length of output: 427
🏁 Script executed:
#!/bin/bash
# Find all OaiMetadataMap plugin files (corrected syntax)
echo "=== All OaiMetadataMap plugin files ==="
find src -type f -name "*.php" -path "*/OaiMetadataMap/*" 2>/dev/null
# Show content of OaiMetadataMap directory
echo -e "\n=== Contents of OaiMetadataMap directory ==="
find src -type f -path "*/OaiMetadataMap/*" 2>/dev/null | head -20
# Check directory structure around PcStandard.php
echo -e "\n=== Directory structure of OaiMetadataMap ==="
find src/Plugin/OaiMetadataMap -type f 2>/dev/null || echo "Directory not found"
# Look at the actual class definition in PcStandard.php
echo -e "\n=== Full PcStandard.php file ==="
cat src/Plugin/OaiMetadataMap/PcStandard.phpLength of output: 2296
🏁 Script executed:
#!/bin/bash
# Find DgiStandard class (parent class)
echo "=== Searching for DgiStandard class ==="
find . -type f -name "*.php" -exec grep -l "class DgiStandard" {} \;
# Check for dgi_standard_oai module
echo -e "\n=== Searching for dgi_standard_oai module ==="
find . -type d -name "dgi_standard_oai"
# Look for composer.json to see dependencies
echo -e "\n=== Checking for composer.json ==="
find . -maxdepth 2 -name "composer.json" -exec cat {} \;
# Search for .info.yml files (Drupal module definitions)
echo -e "\n=== Drupal module files (.info.yml) ==="
find . -name "*.info.yml" | head -10Length of output: 345
🏁 Script executed:
#!/bin/bash
# Check the module info file for dependencies
echo "=== pcdora.info.yml content ==="
cat ./pcdora.info.ymlLength of output: 177
</web_search>
🌐 Web query:
OAI-PMH metadata formats standard mdRecord
💡 Result:
Short answer: mdRecord is not part of the OAI‑PMH protocol standard — it’s an implementation‑specific metadataPrefix used by some repositories (e.g., Islandora/related projects) to expose a repository’s native/richer metadata. OAI‑PMH itself requires only oai_dc (Dublin Core) and allows repositories to define additional metadata formats identified by a metadataPrefix and a metadata schema. See the OAI‑PMH spec for how metadataPrefix/metadataSchema are defined and returned via ListMetadataFormats, and an example of mdRecord usage in Islandora/Islandora‑derived docs. [1][2][3]
If you need details for a particular repository:
- Call ?verb=ListMetadataFormats on that repository’s baseURL to see whether it supports metadataPrefix=mdRecord and the schemaLocation it uses. The response will include the metadataPrefix, metadataNamespace/metadataSchema (or schemaLocation) you must use. [2]
Sources
- OAI‑PMH v2.0 specification (metadataPrefix / ListMetadataFormats). [1]
- Example: Islandora / Arca documentation showing metadataPrefix "mdRecord" (repository‑specific). [2]
- Omeka / general OAI‑PMH docs showing repositories publish custom metadataPrefix values. [3]
References
- Open Archives Initiative Protocol for Metadata Harvesting (OAI‑PMH) v2.0 — metadataPrefix & ListMetadataFormats. [openarchives.org].
- OAI‑PMH harvesting — Islandora/Arca docs (example using metadataPrefix "mdRecord").
- Omeka S / OAI‑PMH documentation (examples of custom metadataPrefix values).
Add required module dependencies to pcdora.info.yml.
The code references external modules (dgi_standard_oai for the DgiStandard parent class and rest_oai_pmh for the template configuration) but these are not declared as dependencies in pcdora.info.yml. This will cause the module to fail at runtime when attempting to instantiate the class or load the template. Both modules must be explicitly declared in the module's info file.
Additionally, while metadata_format = "mdRecord" is not a standard OAI-PMH format (which typically use formats like "oai_dc" or "oai_marc"), it is a valid implementation-specific metadataPrefix used in projects like Islandora. Verify this is intentional for your use case.
🤖 Prompt for AI Agents
In src/Plugin/OaiMetadataMap/PcStandard.php around lines 7 to 21, the plugin
references external modules (dgi_standard_oai for the DgiStandard parent class
and rest_oai_pmh for the template) that are not declared as dependencies; update
pcdora.info.yml to add both dgi_standard_oai and rest_oai_pmh to the
dependencies list so Drupal installs/loads them before this module, and confirm
that metadata_format = "mdRecord" is intentional for your deployment (change to
a standard OAI-PMH metadataPrefix like oai_dc if that was a mistake).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Plugin/dgi_migrate_alter/spreadsheet/DssiNodeAlter.php(1 hunks)
🔇 Additional comments (1)
src/Plugin/dgi_migrate_alter/spreadsheet/DssiNodeAlter.php (1)
23-53: Alter logic forfield_origin_informationis clean and consistentThe use of a referenced
$processplus the explicitkeyslist and two-stepfield_date_notepipeline (skip_on_empty then explode on^) is clear and matches typical Drupal Migrate process patterns for sub-process/paragraph mappings, assuming the existing DSSI node migration structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pcdora.info.yml (1)
6-7: Ensure all plugin provider modules are declared as dependenciesAdding
drupal:migratemakes sense for the new migration alter plugin, but this module also depends on classes fromdgi_migrate_alter(and, for the OAI metadata map, whatever module provides theOaiMetadataMapplugin type /DgiStandardbase). To avoid enablingpcdorawithout its plugin providers, consider declaring those module dependencies here as well (in addition todrupal:migrate), or confirm they are already covered elsewhere in your stack.src/Plugin/dgi_migrate_alter/spreadsheet/DssiNodeAlter.php (1)
24-50: Optional: guard against unexpected migration process shapes
alter()assumes that$migration['process']['field_origin_information'][1]and[2]exist and that overwriting them is always safe for thedssi_nodemigration. That’s fine for the current config, but if the migration definition changes or this alter is reused, those hard‑coded indexes could end up clobbering unrelated process steps.If you want this to be more defensive, consider bailing out when
field_origin_informationis missing/not an array, or when the expected indexes aren’t present, before assigning into them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pcdora.info.yml(1 hunks)src/Plugin/dgi_migrate_alter/spreadsheet/DssiNodeAlter.php(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Code Linting
src/Plugin/dgi_migrate_alter/spreadsheet/DssiNodeAlter.php
[warning] 7-7: Unused use statement. Drupal.Classes.UnusedUseStatement.UnusedUse. PHPCBF can fix automatically.
[error] 1-1: PHPCS: Command '/home/runner/work/_actions/discoverygarden/CodeSniffer/v1/vendor/bin/phpcs -s --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,md,yml ./ --ignore=*.md' exited with code 2.
🔇 Additional comments (1)
src/Plugin/dgi_migrate_alter/spreadsheet/DssiNodeAlter.php (1)
5-17: Verify the external dgi_migrate_alter module to confirm annotation class path and import requirementThe review recommends adding
use Drupal\dgi_migrate_alter\Annotation\MigrationAlter;, but this cannot be verified within the current codebase. Codebase analysis shows:
@MigrationAlterannotation is used in the docblock without an explicit import- The
Translationimport is present and actively used in@Translation(...)annotations—the PHPCS "unused use" warning appears to be a false positivedgi_migrate_alteris an external module providingMigrationAlterBaseandMigrationAlterInterface, but the codebase does not contain its source to verify whether it provides aMigrationAlterannotation classTo resolve the review comment, you need to:
- Check the external
dgi_migrate_altermodule's source to confirm the exact namespace and existence of theMigrationAlterannotation class- Verify whether Drupal's plugin discovery in your version requires explicit annotation class imports
- If the annotation class exists at a different namespace or the import is required, add it; otherwise, dismiss the review as speculative
- Keep the
Translationimport—do not remove it despite PHPCS warnings, as it is required for@Translation()in annotations
Co-authored-by: Adam <[email protected]>
|
Tag generated by PR: v2.2.0 |
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.