DGI9-628: Attempt to avoid making assertions on unpublished content.

[adam-vessey]

Summary by CodeRabbit

• Bug Fixes
  • Access checks now explicitly treat unpublished content as neutral, preventing incorrect allow/deny decisions when viewing unpublished items.
• Chores
  • Internal codebase cleanups and signature clarifications with no change to visible behavior.

[coderabbitai]

Walkthrough

Added a published-status pre-check in EntityAccessHandler::check to return a neutral AccessResult for unpublished entities; several constructors and a hook signature received trailing-comma formatting edits; LUTGenerator and its interface now accept a nullable EntityInterface parameter.

Changes

Cohort / File(s)	Change Summary
Entity Access Handler src/EntityAccessHandler.php	Added use Drupal\Core\Entity\EntityPublishedInterface; in check() added a pre-check: if the entity implements EntityPublishedInterface and isPublished() is false, return a neutral AccessResult with a message, short-circuiting existing access logic.
Lookup Generator (type change) src/LUTGenerator.php, src/LUTGeneratorInterface.php	Changed generate parameter type from EntityInterface $entity = NULL to ?EntityInterface $entity = NULL in both implementation and interface; behavior otherwise unchanged (still handles null).
Constructor / Signature formatting src/Commands/IslandoraHierarchicalAccessCommands.php, src/EntityCUDHandler.php	Added trailing commas to constructor parameter lists (syntax/formatting only; no logic changes).
Hook signature formatting islandora_hierarchical_access.module	Added a trailing comma after the $account parameter in islandora_hierarchical_access_entity_access function signature (formatting only; no behavioral change).

Sequence Diagram

sequenceDiagram
    participant Caller as Caller
    participant Handler as EntityAccessHandler
    participant Entity as Entity

    Caller->>Handler: check(entity, ...)
    Handler->>Entity: implements EntityPublishedInterface?
    alt implements
        Entity-->>Handler: yes
        Handler->>Entity: isPublished()?
        alt not published
            Entity-->>Handler: no
            Handler-->>Caller: neutral AccessResult (published check)
        else published
            Entity-->>Handler: yes
            Handler->>Handler: continue existing access logic
            Handler-->>Caller: AccessResult (allow/deny/neutral)
        end
    else does not implement
        Entity-->>Handler: no
        Handler->>Handler: run existing access logic
        Handler-->>Caller: AccessResult (allow/deny/neutral)
    end

Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hop the code paths, nose a-twitch,
Found unpublished pages and gave them a switch,
A neutral nod, then onward I roam,
Little changes tidy the access home. ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title "Attempt to avoid making assertions on unpublished content" directly addresses the primary functional change in the changeset. The most substantive modification is in EntityAccessHandler.php, where a new pre-check was added that returns a neutral AccessResult when an entity implements EntityPublishedInterface and is not published, effectively bypassing existing access check logic for unpublished content. This aligns precisely with the title's intent. The other changes in the PR are minor formatting updates (trailing commas in parameter lists) and a signature modification to make a parameter explicitly nullable, which are supporting changes. The title is specific, concise, and clearly conveys the main purpose to a reviewer scanning the pull request history.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/status

---

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between aaff6f4 and 51bcb20.

📒 Files selected for processing (6)

• islandora_hierarchical_access.module (1 hunks)
• src/Commands/IslandoraHierarchicalAccessCommands.php (1 hunks)
• src/EntityAccessHandler.php (3 hunks)
• src/EntityCUDHandler.php (1 hunks)
• src/LUTGenerator.php (1 hunks)
• src/LUTGeneratorInterface.php (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/LUTGenerator.php (1)

src/LUTGeneratorInterface.php (1)

• generate (27-27)

src/LUTGeneratorInterface.php (1)

src/LUTGenerator.php (1)

• generate (53-91)

⏰ 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). (5)

• GitHub Check: PHPUnit / Drupal 10.4 | PHP 8.3
• GitHub Check: PHPUnit / Drupal 10.4 | PHP 8.4
• GitHub Check: PHPUnit / Drupal 10.3 | PHP 8.2
• GitHub Check: PHPUnit / Drupal 10.5 | PHP 8.4
• GitHub Check: PHPUnit / Drupal 10.5 | PHP 8.3

🔇 Additional comments (8)

src/EntityCUDHandler.php (1)

81-86: LGTM: Trailing comma added for consistency.

The trailing comma in the constructor parameter list is a stylistic improvement supported in PHP 8.0+. No functional impact.

src/Commands/IslandoraHierarchicalAccessCommands.php (1)

36-39: LGTM: Trailing comma added for consistency.

The trailing comma in the constructor parameter list aligns with the broader formatting update across the codebase. No functional impact.

src/EntityAccessHandler.php (3)

10-10: LGTM: Import added for new functionality.

The import of EntityPublishedInterface is required for the new unpublished entity check below.

---

104-111: LGTM: Trailing comma added for consistency.

The trailing comma in the constructor parameter list is consistent with formatting updates throughout the PR.

---

144-146: Verify caching behavior for unpublished entity bypass.

The pre-check correctly returns neutral access for unpublished entities, avoiding potential issues with LUT lookups on unpublished content. However, ensure that cache invalidation works properly when an entity transitions between published/unpublished states.

Consider verifying the following:

1. When an entity is published/unpublished, does the access cache get properly invalidated?
2. The neutral AccessResult here doesn't include the entity as a cacheable dependency - is this intentional? In line 177, you do add the entity as a cacheable dependency, but here you're returning early without that.

Apply this diff to ensure proper cache invalidation:

 if ($entity instanceof EntityPublishedInterface && !$entity->isPublished()) {
-  return AccessResult::neutral("Hierarchical access is only concerned with published content.");
+  return AccessResult::neutral("Hierarchical access is only concerned with published content.")
+    ->addCacheableDependency($entity);
 }

src/LUTGeneratorInterface.php (1)

27-27: LGTM: Explicit nullable type improves clarity.

Changing from implicit nullable (EntityInterface $entity = NULL) to explicit nullable (?EntityInterface $entity = NULL) makes the intent clearer and is a best practice in modern PHP.

src/LUTGenerator.php (1)

53-53: LGTM: Implements interface with explicit nullable type.

This change aligns with the interface update in LUTGeneratorInterface.php, making the nullable parameter explicit. The implementation logic remains unchanged.

islandora_hierarchical_access.module (1)

77-81: LGTM: Trailing comma added for consistency.

The trailing comma in the hook function signature maintains consistency with the formatting updates across the module.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)

• JIRA integration encountered authorization issues. Please disconnect and reconnect the integration in the CodeRabbit UI.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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/EntityAccessHandler.php (1)

104-111: Fix the linting error to unblock the pipeline.

The pipeline is failing due to a missing trailing comma in the multi-line function declaration. Although this line wasn't changed in this PR, the linting error must be resolved to merge.

Apply this diff to fix the linting error:

   public function __construct(
     Connection $database,
     $ops,
     EntityStorageInterface $storage,
     $column,
     $target_column,
-    array $op_map
+    array $op_map,
   ) {

🧹 Nitpick comments (1)

src/EntityAccessHandler.php (1)

144-146: Approve the early return for unpublished entities; consider adding cache dependency.

The logic correctly short-circuits hierarchical access checks for unpublished content, allowing Drupal's standard access control to handle unpublished entities. The instanceof check is appropriate since not all entities implement EntityPublishedInterface.

However, consider adding the entity as a cacheable dependency since the access result depends on the entity's published status:

 if ($entity instanceof EntityPublishedInterface && !$entity->isPublished()) {
-  return AccessResult::neutral("Hierarchical access is only concerned with published content.");
+  return AccessResult::neutral("Hierarchical access is only concerned with published content.")
+    ->addCacheableDependency($entity);
 }

This ensures that if the entity's published status changes, the cached access result will be invalidated appropriately.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 4d321c9 and aaff6f4.

📒 Files selected for processing (1)

• src/EntityAccessHandler.php (2 hunks)

🧰 Additional context used

🪛 GitHub Actions: Code Linting

src/EntityAccessHandler.php

[error] 110-110: Multi-line function declarations must have a trailing comma after the last parameter (Drupal.Functions.MultiLineFunctionDeclaration.MissingTrailingComma).