FDSF-287: module to reindex embargoes. - #43
chrismacdonaldw wants to merge 21 commits into
Conversation
chrismacdonaldw
added
the
minor
WalkthroughA new Drupal module, Changes
Sequence Diagram(s)sequenceDiagram
participant EmbargoEvent as Embargo Insert/Update
participant Queue
participant QueueWorker
participant EmbargoEntityStorage as Embargo Entity Storage
participant Config
participant SearchAPIIndex as Search API Index
EmbargoEvent->>Queue: If embargo expiration in future, enqueue embargo ID
QueueWorker->>EmbargoEntityStorage: Load embargo by ID
alt Expiration in future
QueueWorker-->>Queue: Requeue with delay until expiration
else Expired
QueueWorker->>EmbargoEntityStorage: Load embargoed node
QueueWorker->>Config: Load selected indexes
loop For each selected index
QueueWorker->>SearchAPIIndex: Mark node translations as updated
end
end
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
chrismacdonaldw
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
modules/reindex_embargoes/reindex_embargoes.module (2)
15-16: Consider using a configurable time window.The 24-hour window (86400 seconds) is hardcoded. Consider making this configurable to allow site administrators to adjust the timeframe according to their needs.
- $next_day = $request_time + 86400; + $config = \Drupal::config('reindex_embargoes.settings'); + $time_window = $config->get('time_window') ?? 86400; + $next_time = $request_time + $time_window;This would require adding a 'time_window' field to your configuration schema.
22-33: Consider using batch processing for better performance.For sites with many embargoes, processing them all in a single cron run might consume excessive resources. Consider implementing a batch approach with a limit.
+ // Limit the number of embargoes processed per cron run. + $config = \Drupal::config('reindex_embargoes.settings'); + $batch_size = $config->get('batch_size') ?? 50; + $embargoes = $entity_type_manager->getStorage('embargo')->loadMultiple($embargo_ids); + $count = 0; foreach ($embargoes as $embargo) { $expiration_date = $embargo->getExpirationDate()->getTimestamp(); if ($expiration_date >= $request_time && $expiration_date < $next_day) { $queue->createItem([ 'embargo_id' => $embargo->id(), ]); + $count++; + if ($count >= $batch_size) { + break; + } } }This would require adding a 'batch_size' field to your configuration schema.
modules/reindex_embargoes/src/Form/SettingsForm.php (2)
32-37: Consider dependency injection for testability
Index::loadMultiple()hard‑codes a static call which makes unit testing harder. Injecting thesearch_api.indexstorage via the service container is the Drupal‑preferred approach.-use Drupal\search_api\Entity\Index; +use Drupal\Core\Entity\EntityTypeManagerInterface; +... +public function __construct(EntityTypeManagerInterface $entity_type_manager) { + $this->entityTypeManager = $entity_type_manager; +} +public static function create(ContainerInterface $container) { + return new static( + $container->get('entity_type.manager'), + ); +} ... -$indexes = Index::loadMultiple(); +$indexes = $this->entityTypeManager + ->getStorage('search_api_index') + ->loadMultiple();This also removes a hard dependency on Search API’s static API.
52-58: Minor: avoid numeric keys when nothing is selectedWhen no checkbox is ticked,
$form_state->getValue('selected_indexes')returns an empty array; afterarray_filter()it stays empty. Callingarray_keys()then yields[], which is fine.
However, casting explicitly to an array eliminates a “null to array” edge case if the element is removed or disabled.-$selected_indexes = $form_state->getValue('selected_indexes'); +$selected_indexes = (array) $form_state->getValue('selected_indexes');modules/reindex_embargoes/src/Plugin/QueueWorker/EmbargoExpirationReindex.php (3)
50-58: Missing method docblocks and spacing
create()andprocessItem()need docblocks describing parameters/return types; they also lack a blank line after the method, violatingSquiz.WhiteSpace.FunctionSpacing.AfterLast.Add something like:
/** * {@inheritdoc} */ public static function create( ... ) { … }and ensure one empty line follows each method body.
69-76: Potential queue flooding: guard against re‑queuing duplicatesEvery run inside the 24‑hour window deletes the current item and pushes a
new one, generating one duplicate per cron execution. For long embargo
windows this can explode the queue size.Mitigation:
-if ($current_expiration > $current_time && $current_expiration < $current_time + 86400) { - $this->queueFactory->get('embargo_expiration_reindex')->createItem($data); - return; -} +if ($current_expiration > $current_time) { + // Re‑queue only once every 6 hours. + if (($current_expiration - $current_time) > 21600) { + $this->queueFactory->get('embargo_expiration_reindex')->createItem($data); + } + return; +}Alternatively, store a “re‑queued” flag in
$datato avoid unbounded growth.
83-91: Fail‑safe when index or datasource is missing
$index->getDatasource('entity:node')can returnNULL; attempting to
iterate translations afterwards will still succeed, buttrackItemsUpdated
may silently fail if the datasource is disabled. Add an explicit
null‑check and log a watchdog warning for observability.-$index = $this->entityTypeManager->getStorage('search_api_index')->load($index_id); -if ($index && $index->isServerEnabled() && $index->getDatasource('entity:node')) { +/** @var \Drupal\search_api\Entity\Index|null $index */ +$index = $this->entityTypeManager->getStorage('search_api_index')->load($index_id); +$datasource = $index ? $index->getDatasource('entity:node') : NULL; +if ($index && $datasource && $index->isServerEnabled()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
modules/reindex_embargoes/README.md(1 hunks)modules/reindex_embargoes/config/schema/reindex_embargoes.schema.yml(1 hunks)modules/reindex_embargoes/reindex_embargoes.info.yml(1 hunks)modules/reindex_embargoes/reindex_embargoes.module(1 hunks)modules/reindex_embargoes/reindex_embargoes.routing.yml(1 hunks)modules/reindex_embargoes/src/Form/SettingsForm.php(1 hunks)modules/reindex_embargoes/src/Plugin/QueueWorker/EmbargoExpirationReindex.php(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Code Linting
modules/reindex_embargoes/src/Form/SettingsForm.php
[error] 9-60: Multiple coding standard errors including missing class doc comment, incorrect line indentation, missing blank lines before/after functions, and missing empty line before closing brace. (Drupal.Commenting.ClassComment.Missing, Drupal.WhiteSpace.ScopeIndent.IncorrectExact, Squiz.WhiteSpace.FunctionSpacing.BeforeFirst, Squiz.WhiteSpace.FunctionSpacing.AfterLast, Drupal.Classes.ClassDeclaration.CloseBraceAfterBody)
modules/reindex_embargoes/src/Plugin/QueueWorker/EmbargoExpirationReindex.php
[error] 13-94: Multiple errors including missing short description in doc comment, incorrect @var tag type, missing function doc comments, missing blank line after function, and missing empty line before closing brace. (Drupal.Commenting.DocComment.MissingShort, Drupal.Commenting.VariableComment.IncorrectVarType, Drupal.Commenting.FunctionComment.Missing, Squiz.WhiteSpace.FunctionSpacing.AfterLast, Drupal.Classes.ClassDeclaration.CloseBraceAfterBody)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: PHPUnit / Drupal 10.3 | PHP 8.3
- GitHub Check: PHPUnit / Drupal 10.3 | PHP 8.2
- GitHub Check: PHPUnit / Drupal 10.2 | PHP 8.3
- GitHub Check: PHPUnit / Drupal 10.2 | PHP 8.2
🔇 Additional comments (5)
modules/reindex_embargoes/reindex_embargoes.info.yml (1)
1-9: Module definition looks complete and follows Drupal standards.The info file correctly defines the new module with appropriate dependencies on embargo and search_api. The core version requirement is properly specified to support both Drupal 9 and 10.
modules/reindex_embargoes/config/schema/reindex_embargoes.schema.yml (1)
1-10: Schema definition is appropriate for the module's needs.The configuration schema properly defines a sequence of string values to store selected Search API indexes. This follows Drupal's configuration schema standards and will correctly support the module's functionality.
modules/reindex_embargoes/reindex_embargoes.routing.yml (1)
1-7: Routing configuration appears correct.The route is correctly defined with an appropriate path under the embargo administration section, references the settings form class, and requires the 'administer site configuration' permission. This follows Drupal's routing standards.
modules/reindex_embargoes/README.md (1)
1-26: Documentation is clear and comprehensive.The README covers the module's purpose, features, maintainers, and licensing information. It also provides guidance for contributors, which is good practice.
modules/reindex_embargoes/reindex_embargoes.module (1)
18-21:Details
❌ Incorrect review comment
Check if expiration_date field exists before querying.
There appears to be a mismatch between the query and later code. The query doesn't check for an expiration_date field, but line 26 assumes its existence.
Please verify that the embargo entity type has an expiration_date field and that it's being queried correctly.
🏁 Script executed:
#!/bin/bash # Verify the embargo entity type has an expiration_date field drush eval "print_r(\Drupal::entityTypeManager()->getStorage('embargo')->getEntityType()->getKeys());"Length of output: 206
🏁 Script executed:
#!/bin/bash # Search for expiration_date usage across the codebase echo ">> Looking for any occurrences of 'expiration_date'" rg -Hn "expiration_date" || echo "No expiration_date occurrences found." # Locate the definition of the embargo entity type echo -e "\n>> Locating @ContentEntityType annotation for 'embargo'" rg -Hn "@ContentEntityType" -A5 | rg "embargo" || echo "No embargo ContentEntityType annotation found." # Check for hook_entity_base_field_info implementations echo -e "\n>> Searching for hook_entity_base_field_info to see where base fields are defined" rg -Hn "hook_entity_base_field_info" -A5 || echo "No hook_entity_base_field_info implementations found."Length of output: 3262
The
expiration_datefield is defined as a base field on the Embargo entity—no runtime existence check is needed.
- src/Entity/Embargo.php (around line 122) declares
$fields['expiration_date'] = BaseFieldDefinition::create('datetime');- modules/reindex_embargoes/reindex_embargoes.module (lines 18–21) loads all embargos with a non‑null embargoed_node and then filters by expiration_date in PHP.
If your goal is to avoid loading already‑expired embargos, consider adding expiration_date conditions directly to the entity query. For example:
- $embargo_ids = $entity_type_manager->getStorage('embargo')->getQuery() - ->accessCheck(FALSE) - ->condition('embargoed_node', NULL, 'IS NOT NULL') - ->execute(); + $request_time = \Drupal::time()->getRequestTime(); + $next_day = $request_time + 86400; + $embargo_ids = $entity_type_manager->getStorage('embargo')->getQuery() + ->accessCheck(FALSE) + ->condition('embargoed_node', NULL, 'IS NOT NULL') + ->condition('expiration_date', $request_time, '>=') + ->condition('expiration_date', $next_day, '<') + ->execute();No additional check for field existence is required.
Likely an incorrect or invalid review comment.
| foreach ($embargoed_node->getTranslationLanguages() as $language) { | ||
| $doc_ids[] = $embargoed_node->id() . ':' . $language->getId(); | ||
| } | ||
| $index->trackItemsUpdated('entity:node', $doc_ids); |
There was a problem hiding this comment.
Paranoia perhaps, but: Does this work for inherited embargoes?
There was a problem hiding this comment.
Question was never answered? Is still relevant?
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
modules/reindex_embargoes/src/Plugin/QueueWorker/EmbargoExpirationReindex.php (1)
14-22: Improve class documentationThe class docblock doesn't provide enough information about what this queue worker does, when it runs, and how it works. This was flagged in previous reviews.
/** - * QueueWorker for reindexing embargoed nodes. + * Processes embargo records and re-indexes affected nodes when they expire. + * + * This queue worker handles embargoed content that needs reindexing after an + * embargo has expired. Items are enqueued either via cron, on embargo + * creation/update, or via Drush command. Each queue item contains: + * - embargo_id: The ID of the embargo entity to process * * @QueueWorker( * id = "embargo_expiration_reindex", * title = @Translation("Embargo Expiration Reindex"), * cron = {"time" = 60} * ) */
🧹 Nitpick comments (5)
modules/reindex_embargoes/src/Drush/Commands/ReindexEmbargoesCommands.php (3)
34-38: Fix docblock length issueThe docblock line exceeds the 80 character limit. Consider rewording or breaking it into multiple lines.
/** - * Populates the embargo expiration queue with existing future-dated embargoes. + * Populates the embargo expiration queue with future-dated embargoes. + * + * Finds all embargoes with future expiration dates and adds them to the queue. */🧰 Tools
🪛 GitHub Actions: Code Linting
[warning] 35-35: Line exceeds 80 characters; contains 81 characters (Drupal.Files.LineLength.TooLong)
41-53: Query implementation looks good, but could be more efficientThe query correctly filters for embargoes that have non-null embargoed nodes, an expiration type of 1, non-null expiration dates, and expiration dates in the future.
However, you're using a string date comparison (
$current_iso), then later filtering by timestamp. Consider using a timestamp directly in the query for more accurate filtering.- $current_iso = gmdate('Y-m-d', $current_timestamp); - $query = $embargo_storage->getQuery() ->accessCheck(FALSE) ->condition('embargoed_node', NULL, 'IS NOT NULL') ->condition('expiration_type', 1, '=') ->condition('expiration_date', NULL, 'IS NOT NULL') - ->condition('expiration_date', $current_iso, '>'); + ->condition('expiration_date.value', $current_timestamp, '>');
38-81: Consider adding error handlingThe queue operations could potentially fail. Consider adding try/catch blocks to handle queue-related exceptions.
public function populateQueue(): void { $this->output()->writeln("Populating Embargo expiration queue..."); + try { $embargo_storage = $this->entityTypeManager->getStorage('embargo'); $queue = $this->queueFactory->get('embargo_expiration_reindex', TRUE); // ... rest of the method ... $this->output()->writeln(dt("Successfully queued @count Embargoes for future re-indexing.", ['@count' => $queued_count])); + } + catch (\Exception $e) { + $this->logger()->error('Error populating embargo queue: @message', ['@message' => $e->getMessage()]); + $this->output()->writeln('An error occurred while populating the embargo queue. See the error log for details.'); + } }modules/reindex_embargoes/src/Plugin/QueueWorker/EmbargoExpirationReindex.php (2)
106-108: Handle empty array case for selected indexesYou're using
??to default to an empty array if the configuration is null, but consider adding a log message or early return if there are no selected indexes.$config = $this->configFactory->get('reindex_embargoes.settings'); $selected_indexes = array_values($config->get('selected_indexes') ?? []); + if (empty($selected_indexes)) { + \Drupal::logger('reindex_embargoes')->notice('No search indexes selected for reindexing in module configuration.'); + return; + }
109-124: Consider adding logging for reindexing operationsThe reindexing logic works well, but adding logging would help with troubleshooting and monitoring.
foreach ($selected_indexes as $index_id) { $index = $this->entityTypeManager->getStorage('search_api_index')->load($index_id); if ($index instanceof IndexInterface && $index->isServerEnabled() && $index->status()) { if ($index->isValidDatasource('entity:node')) { $node_ids_to_reindex = []; // Item ID format is typically 'entity_id:langcode'. foreach ($embargoed_node->getTranslationLanguages() as $language) { $node_ids_to_reindex[] = $embargoed_node->id() . ':' . $language->getId(); } if (!empty($node_ids_to_reindex)) { $index->trackItemsUpdated('entity:node', $node_ids_to_reindex); + \Drupal::logger('reindex_embargoes')->info( + 'Reindexed @count translation(s) of node @nid in index @index after embargo expiration.', + [ + '@count' => count($node_ids_to_reindex), + '@nid' => $embargoed_node->id(), + '@index' => $index_id, + ] + ); } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
embargo.info.yml(1 hunks)modules/migrate_embargoes_to_embargo/migrate_embargoes_to_embargo.info.yml(1 hunks)modules/migrate_embargoes_to_embargo/src/Plugin/migrate/source/Entity.php(1 hunks)modules/reindex_embargoes/drush.services.yml(1 hunks)modules/reindex_embargoes/reindex_embargoes.info.yml(1 hunks)modules/reindex_embargoes/reindex_embargoes.module(1 hunks)modules/reindex_embargoes/src/Drush/Commands/ReindexEmbargoesCommands.php(1 hunks)modules/reindex_embargoes/src/Form/SettingsForm.php(1 hunks)modules/reindex_embargoes/src/Plugin/QueueWorker/EmbargoExpirationReindex.php(1 hunks)src/Controller/IpRangeAccessExemptionController.php(1 hunks)src/Plugin/search_api/processor/EmbargoJoinProcessor.php(1 hunks)src/Plugin/search_api/processor/EmbargoProcessor.php(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- embargo.info.yml
- modules/migrate_embargoes_to_embargo/migrate_embargoes_to_embargo.info.yml
- modules/migrate_embargoes_to_embargo/src/Plugin/migrate/source/Entity.php
- src/Controller/IpRangeAccessExemptionController.php
- modules/reindex_embargoes/drush.services.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/reindex_embargoes/reindex_embargoes.info.yml
- modules/reindex_embargoes/reindex_embargoes.module
- modules/reindex_embargoes/src/Form/SettingsForm.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Plugin/search_api/processor/EmbargoProcessor.php (1)
src/Plugin/search_api/processor/EmbargoJoinProcessor.php (1)
getPropertyDefinitions(107-132)
src/Plugin/search_api/processor/EmbargoJoinProcessor.php (1)
src/Plugin/search_api/processor/EmbargoProcessor.php (1)
getPropertyDefinitions(91-103)
🪛 GitHub Actions: Code Linting
modules/reindex_embargoes/src/Drush/Commands/ReindexEmbargoesCommands.php
[error] 29-29: Multi-line function declarations must have a trailing comma after the last parameter (Drupal.Functions.MultiLineFunctionDeclaration.MissingTrailingComma)
[warning] 35-35: Line exceeds 80 characters; contains 81 characters (Drupal.Files.LineLength.TooLong)
modules/reindex_embargoes/src/Plugin/QueueWorker/EmbargoExpirationReindex.php
[error] 51-51: Multi-line function declaration not indented correctly; expected 4 spaces but found 10 (Drupal.Functions.MultiLineFunctionDeclaration.Indent)
[error] 52-52: Multi-line function declaration not indented correctly; expected 4 spaces but found 10 (Drupal.Functions.MultiLineFunctionDeclaration.Indent)
[error] 55-55: Multi-line function declarations must have a trailing comma after the last parameter (Drupal.Functions.MultiLineFunctionDeclaration.MissingTrailingComma)
🔇 Additional comments (6)
src/Plugin/search_api/processor/EmbargoJoinProcessor.php (1)
107-107: Improved type safety with explicit nullable hint.The updated method signature now uses the explicit nullable type hint
?DatasourceInterfacewhile maintaining backward compatibility. This change aligns with modern PHP type hinting practices and improves code readability by making it immediately clear that the parameter can be null.src/Plugin/search_api/processor/EmbargoProcessor.php (1)
91-91: Improved type safety with explicit nullable hint.The updated method signature now uses the explicit nullable type hint
?DatasourceInterfacewhile maintaining backward compatibility. This change is consistent with the update in EmbargoJoinProcessor and follows modern PHP type hinting best practices, making it clear that the parameter can be null.modules/reindex_embargoes/src/Drush/Commands/ReindexEmbargoesCommands.php (2)
1-12: Imports and namespaces look correctThe class properly imports all necessary dependencies for entity management, queue handling, and time functions.
56-59: Early return looks goodGood practice to check for empty results early and return with an appropriate message rather than continuing processing.
modules/reindex_embargoes/src/Plugin/QueueWorker/EmbargoExpirationReindex.php (2)
98-102: Good use of the delay mechanismUsing
DelayedRequeueExceptionto defer processing until the expiration time is a good pattern that prevents unnecessary processing and maintains proper timing.
104-125:Details
❌ Incorrect review comment
Consider handling inherited embargoes
The code doesn't appear to handle inherited embargoes, as noted in a previous review comment.
Can you confirm whether this implementation should handle inherited embargoes? If so, the current code needs to be updated to traverse the inheritance chain.
🏁 Script executed:
#!/bin/bash # Check if the Embargo entity has methods related to inheritance echo "Checking for inheritance methods in Embargo entity:" rg -A 2 "function get(Parent|Child|Inherit)" --type php modules/custom/embargo/src/Entity/Embargo.php || echo "No inheritance methods found" # Check if there are any usages of inheritance in Embargo entities echo -e "\nChecking for usage of embargo inheritance in the codebase:" rg -A 2 "(parent|child) embargo" --type php || echo "No explicit references to embargo inheritance found"Length of output: 661
🏁 Script executed:
#!/bin/bash # Locate Embargo entity file(s) in the repository echo "Embargo entity file locations:" rg -l "class\s+Embargo" -t php # Search for inheritance-related terms in those files echo -e "\nChecking for inheritance support in Embargo entity files:" for file in $(rg -l "class\s+Embargo" -t php); do echo "---- $file ----" rg -n "inherit" -t php "$file" || echo " No inheritance keywords found in $file" doneLength of output: 7937
Inherited embargoes aren’t supported today
I searched the Embargo entity (src/Entity/Embargo.php) and the rest of the codebase and found no methods or properties for parent/child or inherited embargo relationships—only
@inheritdocdocblocks. Since there’s no inheritance model in the current API, there’s nothing to traverse. You can safely ignore this suggestion.Likely an incorrect or invalid review comment.
| #uses: discoverygarden/phpunit-action/.github/workflows/phpunit.yml@v1 | ||
| uses: discoverygarden/phpunit-action/.github/workflows/phpunit.yml@adam-vessey-patch-1 |
There was a problem hiding this comment.
To be moved back prior to merge, after merge of discoverygarden/phpunit-action#24
| #uses: discoverygarden/phpunit-action/.github/workflows/phpunit.yml@v1 | |
| uses: discoverygarden/phpunit-action/.github/workflows/phpunit.yml@adam-vessey-patch-1 | |
| uses: discoverygarden/phpunit-action/.github/workflows/phpunit.yml@v1 |
| composer_prereqs: >- | ||
| "discoverygarden/islandora_test_support:dev-adam-vessey-patch-1 as 1.x-dev" |
There was a problem hiding this comment.
Can go away with merge of discoverygarden/islandora_test_support#9
| composer_prereqs: >- | |
| "discoverygarden/islandora_test_support:dev-adam-vessey-patch-1 as 1.x-dev" |
| composer_patches: |- | ||
| { | ||
| "discoverygarden/islandora_hierarchical_access": { | ||
| "dependent work from dependency": "https://github.com/discoverygarden/islandora_hierarchical_access/pull/19.patch" | ||
| } | ||
| } |
There was a problem hiding this comment.
No longer necessary, has been merged for over a year, now: discoverygarden/islandora_hierarchical_access#19
| $config = $this->configFactory->get('reindex_embargoes.settings'); | ||
| $selected_indexes = array_values($config->get('selected_indexes') ?? []); | ||
|
|
||
| foreach ($selected_indexes as $index_id) { | ||
| $index = $this->entityTypeManager->getStorage('search_api_index')->load($index_id); | ||
| if ($index instanceof IndexInterface && $index->isServerEnabled() && $index->status()) { | ||
| if ($index->isValidDatasource('entity:node')) { | ||
| $node_ids_to_reindex = []; | ||
| // Item ID format is typically 'entity_id:langcode'. | ||
| foreach ($embargoed_node->getTranslationLanguages() as $language) { | ||
| $node_ids_to_reindex[] = $embargoed_node->id() . ':' . $language->getId(); | ||
| } | ||
|
|
||
| if (!empty($node_ids_to_reindex)) { | ||
| $index->trackItemsUpdated('entity:node', $node_ids_to_reindex); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Are we prematurely optimizing here, targeting indexes? Could see leaving it open as an option maybe, but it would be much simpler just to call search_api_entity_update($embargoed_node);, and let it hit everything, yeah?
On the other hand... not sure this fully addresses the need, especially WRT inherited embargoes. Might make sense to move the queue worker inside of the embargo module proper, and to dispatch an event instead of doing concrete work. Have an event subscriber that performs the reindexing for the single thing, and another for the event over on the embargo_inheritance module to deal with the inherited embargo stuff?
| foreach ($embargoed_node->getTranslationLanguages() as $language) { | ||
| $doc_ids[] = $embargoed_node->id() . ':' . $language->getId(); | ||
| } | ||
| $index->trackItemsUpdated('entity:node', $doc_ids); |
There was a problem hiding this comment.
Question was never answered? Is still relevant?
…ommands.php Co-authored-by: Adam <607975+adam-vessey@users.noreply.github.com>
…ommands.php Co-authored-by: Adam <607975+adam-vessey@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/phpunit.yml (1)
11-12: 🛠️ Refactor suggestionTemporary branch pin for external PHPUnit workflow; pin to a commit SHA and revert after upstream merge
Theusesline is pointing at theadam-vessey-patch-1branch. For reproducible CI runs, pin this to the specific commit SHA of the merged patch. Once discoverygarden/phpunit-action#24 is merged and a new tag is released, revert back to@v1and remove the commented-out line.Proposed diff:
-#uses: discoverygarden/phpunit-action/.github/workflows/phpunit.yml@v1 -uses: discoverygarden/phpunit-action/.github/workflows/phpunit.yml@adam-vessey-patch-1 +uses: discoverygarden/phpunit-action/.github/workflows/phpunit.yml@<commit-sha>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/phpunit.yml(1 hunks)modules/reindex_embargoes/src/Drush/Commands/ReindexEmbargoesCommands.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/reindex_embargoes/src/Drush/Commands/ReindexEmbargoesCommands.php
| composer_prereqs: >- | ||
| "discoverygarden/islandora_test_support:dev-adam-vessey-patch-1 as 1.x-dev" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate composer_prereqs input and version alias syntax
You’ve switched from composer_patches to composer_prereqs with an alias. Please confirm that the external action in the adam-vessey-patch-1 branch supports this input and that the alias "discoverygarden/islandora_test_support:dev-adam-vessey-patch-1 as 1.x-dev" meets your dependency constraints.
You can verify the action’s inputs by inspecting its metadata:
🏁 Script executed:
#!/bin/bash
# Fetch action metadata to list supported inputs
curl -sL \
https://raw.githubusercontent.com/discoverygarden/phpunit-action/adam-vessey-patch-1/action.yml \
| grep -A5 'inputs:'Length of output: 448
Unsupported composer_prereqs input in phpunit.yml
The external action at discoverygarden/phpunit-action@adam-vessey-patch-1 only defines these inputs in its action.yml:
composer-authcomposer_package
There is no composer_prereqs input, so the workflow will fail to pass that value. You must either:
- Rename
composer_prereqstocomposer_packageif you intend to use the built-in input, or - Add a
composer_prereqsinput to the action’saction.yml(and handle it in the action code).
Also double-check that your alias syntax (dev-adam-vessey-patch-1 as 1.x-dev) aligns with your composer version constraints.
Affected file:
.github/workflows/phpunit.yml(lines 15–16)
🤖 Prompt for AI Agents
In .github/workflows/phpunit.yml at lines 15 to 16, the input name
composer_prereqs is not supported by the external action
discoverygarden/phpunit-action@adam-vessey-patch-1, which only accepts
composer_package and composer-auth inputs. To fix this, rename the
composer_prereqs input to composer_package if you want to use the existing
input, or alternatively, add a composer_prereqs input to the action's action.yml
and update the action code to handle it. Also verify that the alias syntax
"dev-adam-vessey-patch-1 as 1.x-dev" matches your composer version constraints.
3 participants
Summary by CodeRabbit