DGI9-618: Allow for both file_access and media_access being present.
#23
Conversation
Can happen with views, say, creating a view of media with relationships to their files, for whatever reason. As is here, it would might end up tagging multiple times.
WalkthroughRefactors query-tagging to pass an explicit type string into Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as Hook implementation
participant Service as QueryTagger
participant Query as SelectInterface
Hook->>Service: tagQuery(Query, "file" or "media")
Note right of Service #D0E8D0: Validate type == "file" | "media"
Service->>Query: Apply hierarchical-access tags/conditions
Query-->>Hook: Query prepared for execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
|
Reviewing as part of https://github.com/discoverygarden/ctda-drupal/pull/432 |
Was all small automatically-fixable bits.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/LUTGenerator.php (1)
66-69: Replace assert with a runtime guard to avoid silent misuse in production.
assert($entity instanceof MediaInterface)is commonly disabled in prod. If a non-media entity slips through, the query will filtermidby a non-media ID, leading to confusing no-op behavior. Throw a clear exception (or early return) instead.Apply within this hunk:
- if ($entity) { - assert($entity instanceof MediaInterface); - $query->condition("{$media_alias}.mid", $entity->id()); - } + if ($entity !== NULL) { + if (!($entity instanceof MediaInterface)) { + throw new \InvalidArgumentException('LUTGenerator::generate() expects a Media entity when $entity is provided.'); + } + $query->condition("{$media_alias}.mid", $entity->id()); + }
🧹 Nitpick comments (4)
src/EntityCUDHandler.php (2)
81-86: Trailing comma is fine on PHP 8+, but consider adding scalar types to constructor args.
- The trailing comma after
$operationsis valid only on PHP 8.0+. Please confirm the module’s PHP constraint (composer.json) is >= 8.0.- Add scalar types for
$columnand$operationsto align with the typed properties (string $column,int $operations) and fail fast.Apply within this hunk:
public function __construct( Connection $database, LUTGeneratorInterface $generator, - $column, - $operations, + string $column, + int $operations, ) {
96-103: Defaulting to DELETE when not configured—make intent explicit.
createInstance()falls back toOPERATIONS_DELETEif the entity type didn’t set operations. That’s likely intended for file/node, but it’s implicit. Consider explicitly setting operations inattach()for File and Node to avoid surprises if new entity types are attached without ops.Outside this hunk (for clarity), suggested changes in
attach():// File entities. $entity_type->setHandlerClass(static::NAME, static::class) ->set(static::PROPERTY_NAME__COLUMN, 'fid') ->set(static::PROPERTY_NAME__OPERATIONS, static::OPERATIONS_DELETE); // Media entities. $entity_type->setHandlerClass(static::NAME, static::class) ->set(static::PROPERTY_NAME__COLUMN, 'mid') ->set(static::PROPERTY_NAME__OPERATIONS, static::OPERATIONS_CREATE | static::OPERATIONS_UPDATE | static::OPERATIONS_DELETE); // Node entities. $entity_type->setHandlerClass(static::NAME, static::class) ->set(static::PROPERTY_NAME__COLUMN, 'nid') ->set(static::PROPERTY_NAME__OPERATIONS, static::OPERATIONS_DELETE);src/LUTGeneratorInterface.php (1)
27-27: Nullable parameter: OK, but type should reflect actual usage (Media only).
generate(?EntityInterface $entity = NULL)widens acceptance, but the implementation only supports Media (filters onmid). Either:
- Keep as-is and document that, if provided,
$entitymust be aMediaInterface; or- Preferably narrow the type to
?MediaInterfaceto make the contract explicit.Option A (narrow type) within this hunk:
- public function generate(?EntityInterface $entity = NULL): void; + public function generate(?\Drupal\media\MediaInterface $entity = NULL): void;Additionally (outside this hunk), add the import and update the docblock:
use Drupal\media\MediaInterface; /** * @param \Drupal\media\MediaInterface|null $entity * If provided, only rows for the given media entity will be added. */src/LUTGenerator.php (1)
103-114: Minor: use strict in_array to avoid accidental matches.When de-duplicating field names, add strict comparison to be safe.
Suggested change (outside this hunk):
- if (!in_array($name, $this->uniqueFileFields)) { + if (!in_array($name, $this->uniqueFileFields, true)) { $this->uniqueFileFields[] = $name; }
📜 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 (6)
islandora_hierarchical_access.module(3 hunks)src/Commands/IslandoraHierarchicalAccessCommands.php(1 hunks)src/EntityAccessHandler.php(1 hunks)src/EntityCUDHandler.php(1 hunks)src/LUTGenerator.php(1 hunks)src/LUTGeneratorInterface.php(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/EntityAccessHandler.php
- src/Commands/IslandoraHierarchicalAccessCommands.php
🚧 Files skipped from review as they are similar to previous changes (1)
- islandora_hierarchical_access.module
🧰 Additional context used
🧬 Code graph analysis (2)
src/LUTGeneratorInterface.php (1)
src/LUTGenerator.php (1)
generate(53-91)
src/LUTGenerator.php (1)
src/LUTGeneratorInterface.php (1)
generate(27-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: PHPUnit / Drupal 10.2 | PHP 8.3
- GitHub Check: PHPUnit / Drupal 10.3 | PHP 8.2
- GitHub Check: PHPUnit / Drupal 10.3 | PHP 8.3
- GitHub Check: PHPUnit / Drupal 10.2 | PHP 8.2
🔇 Additional comments (1)
src/LUTGenerator.php (1)
53-53: Signature stays consistent with the interface—LGTM.Matches
LUTGeneratorInterface::generate(?EntityInterface $entity = NULL). No issues.
| jobs: | ||
| PHPUnit: | ||
| uses: discoverygarden/phpunit-action/.github/workflows/phpunit.yml@v1 | ||
| uses: discoverygarden/phpunit-action/.github/workflows/phpunit.yml@fix/update-matrix |
There was a problem hiding this comment.
Is pointing at discoverygarden/phpunit-action#26 to fix tests. Should probably be merged and pointed at before this PR is merged.
Can happen with views, say, creating a view of media with relationships to their files, for whatever reason.
As it was, it could end up exploding with assertions.
As is here, it would might end up tagging multiple times.
Summary by CodeRabbit