[#677] Added shared entity registry with automatic scenario cleanup.#686
Open
AlexSkrypnyk wants to merge 3 commits into
Open
[#677] Added shared entity registry with automatic scenario cleanup.#686AlexSkrypnyk wants to merge 3 commits into
AlexSkrypnyk wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughAdds shared scenario-scoped entity cleanup in ChangesUnified entity cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Drupal/ContentBlockTrait.php (1)
168-169: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate stale docblock referencing removed
$blockContentEntitiesarray.This comment still describes the old per-trait static tracking array, which was removed in favor of
entityRegister(). Update it to reflect the shared registry cleanup mechanism.📝 Proposed doc fix
- * This internal helper method creates and saves a single content block - * entity. - * Created entities are stored in the static $blockContentEntities array for - * automatic cleanup after the scenario. + * This internal helper method creates and saves a single content block + * entity. + * Created entities are registered via entityRegister() for automatic + * cleanup after the scenario.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Drupal/ContentBlockTrait.php` around lines 168 - 169, Update the stale docblock in ContentBlockTrait so it no longer references the removed $blockContentEntities array; revise the comment to describe the current shared cleanup flow via entityRegister() and related entity registry tracking. Keep the wording aligned with the current behavior in the trait so the documentation matches the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 171-177: Update the cleanup documentation paragraph so it matches
HelperTrait::ENTITY_CLEANUP_EXCLUDED_TYPES by explicitly adding the skipped
language entity types, language and configurable_language, alongside the
existing exclusion list. Keep the wording in README consistent with the actual
cleanup behavior described by HelperTrait and the shared registry logic.
In `@src/Drupal/HelperTrait.php`:
- Around line 25-27: The class docblock for HelperTrait has an excluded-types
list that no longer matches ENTITY_CLEANUP_EXCLUDED_TYPES. Update the
documentation near the trait/class comment to include every currently excluded
type, including language and configurable_language, so it accurately reflects
the double-deletion safeguard and stays in sync with the constant.
In `@tests/phpunit/src/Drupal/HelperTraitTest.php`:
- Around line 54-59: Add test coverage for
HelperTrait::entityRegister(EntityInterface) instead of only calling
entityRegisterId() directly. Create a stubbed EntityInterface that returns a
valid id and another that returns NULL, then verify entityRegister() records the
non-null entity via entityRegisterId() and skips the null-id case by leaving the
registry unchanged. Use the existing test helper object in HelperTraitTest to
assert the behavior around the null-id guard.
---
Outside diff comments:
In `@src/Drupal/ContentBlockTrait.php`:
- Around line 168-169: Update the stale docblock in ContentBlockTrait so it no
longer references the removed $blockContentEntities array; revise the comment to
describe the current shared cleanup flow via entityRegister() and related entity
registry tracking. Keep the wording aligned with the current behavior in the
trait so the documentation matches the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e4449e4f-4cab-42bb-ab57-2f87b18e4dca
📒 Files selected for processing (19)
MIGRATION.mdREADME.mdSTEPS.mddocs.phpsrc/Drupal/BlockTrait.phpsrc/Drupal/ContentBlockTrait.phpsrc/Drupal/EckTrait.phpsrc/Drupal/FileTrait.phpsrc/Drupal/HelperTrait.phpsrc/Drupal/MediaTrait.phpsrc/Drupal/MenuTrait.phpsrc/Drupal/ParagraphsTrait.phpsrc/Drupal/RedirectTrait.phpsrc/Drupal/WebformTrait.phptests/behat/features/drupal_block.featuretests/behat/features/drupal_content_block.featuretests/behat/features/drupal_eck.featuretests/behat/features/drupal_entity_cleanup.featuretests/phpunit/src/Drupal/HelperTraitTest.php
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #686 +/- ##
==========================================
- Coverage 97.15% 97.13% -0.02%
==========================================
Files 51 51
Lines 4215 4192 -23
==========================================
- Hits 4095 4072 -23
Misses 120 120 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Drupal/HelperTrait.php (1)
104-111: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIsolate each entity cleanup failure
getStorage()can throwPluginNotFoundExceptionorInvalidPluginDefinitionException, anddelete()can throwEntityStorageException; one failed entity currently aborts the rest of the teardown and leavesentityRegistryuncleared. Wrap each iteration so cleanup keeps going.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Drupal/HelperTrait.php` around lines 104 - 111, Wrap each iteration in the entity cleanup loop in HelperTrait::cleanup so a failure in one entity does not stop the remaining teardown work. Specifically, catch exceptions from both \Drupal::entityTypeManager()->getStorage(...)->load(...) and the subsequent delete call around the existing $entity_type_id / $entity_id processing, log or otherwise handle the failure per entity, and continue to the next entry. Make sure entityRegistry is still cleared after the loop even when some entities fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Drupal/HelperTrait.php`:
- Around line 104-111: Wrap each iteration in the entity cleanup loop in
HelperTrait::cleanup so a failure in one entity does not stop the remaining
teardown work. Specifically, catch exceptions from both
\Drupal::entityTypeManager()->getStorage(...)->load(...) and the subsequent
delete call around the existing $entity_type_id / $entity_id processing, log or
otherwise handle the failure per entity, and continue to the next entry. Make
sure entityRegistry is still cleared after the loop even when some entities
fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6e79c0fb-6bcf-4ad5-a970-fee69aa41252
📒 Files selected for processing (2)
README.mdsrc/Drupal/HelperTrait.php
AlexSkrypnyk
added
the
Needs review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #677
Summary
Adds a shared per-scenario entity registry to
HelperTrait(Drupal) that centralises cleanup of Drupal entities created during Behat scenarios. Instead of each direct-API creation trait maintaining its own registry property, its own#[AfterScenario]hook, and its own skip tag, entities are registered into one array and deleted in reverse creation order by a single hook.Changes
src/Drupal/HelperTrait.php: addsentityRegister(EntityInterface)andentityRegisterId(string $type, int|string $id)to record[type, id]pairs in a new$entityRegistryarray, and a single#[AfterScenario('@api')] entityCleanupAfterScenario()hook that reloads each registered entity by id and deletes it in reverse creation order, tolerating entities already deleted elsewhere in the scenario.ENTITY_CLEANUP_EXCLUDED_TYPESlist (node,user,taxonomy_term,user_role,language,configurable_language) stops the hook from double-deleting entities the base Drupal Extension already cleans up.@behat-steps-skip:entityCleanupAfterScenarioskips cleanup entirely for a scenario;@behat-steps-entity-cleanup-skip:ENTITY_TYPE_IDskips one entity type and can be repeated for several types.entityRegister()/entityRegisterId()and removes their individual registry property and#[AfterScenario]hook:MediaTrait,FileTrait(managed files only - itsfileAfterScenariohook still handles unmanaged files),ParagraphsTrait,ContentBlockTrait,EckTrait,MenuTrait(menus and menu links),RedirectTrait,BlockTrait,WebformTrait.RedirectTraitandBlockTraitalso drop manual registry-pruning from their delete steps, since the shared hook's reload-and-tolerate approach already handles entities deleted mid-scenario.docs.php: registers the newbehat-steps-entity-cleanup-skipparametrized tag.tests/behat/features/drupal_entity_cleanup.feature(new): paired scenarios that prove auto-cleanup, the global skip tag, and the per-type skip tag, relying on Behat's file-order execution - one scenario creates an entity, the next asserts whether it survived teardown.tests/phpunit/src/Drupal/HelperTraitTest.php: unit tests forentityRegisterId()andentityCleanupSkippedTypes().drupal_block.feature,drupal_content_block.feature,drupal_eck.feature: migrated from the removed per-trait skip tags to the new per-type tag.MIGRATION.md(new): maps each removed skip tag to its replacement.README.md: documents the cleanup mechanism and its two bypass tags.STEPS.md: regenerated.Breaking change
The per-trait cleanup skip tags are removed:
@behat-steps-skip:mediaAfterScenario,:contentBlockAfterScenario,:eckAfterScenario,:menuAfterScenario,:redirectAfterScenario,:blockAfterScenario,:webformAfterScenario,:paragraphsAfterScenario. SeeMIGRATION.mdfor the replacement tag for each.Before / After
Summary
This PR introduces a shared, per-scenario Drupal entity registry used by Behat step traits to ensure scenario-created entities are automatically cleaned up at teardown in reverse creation order. Cleanup is implemented once in
src/Drupal/HelperTrait.phpvia a#[AfterScenario('@api')]hook that:Key behavior changes
HelperTrait::entityRegister()instead of maintaining their own static registries and trait-level*AfterScenariohooks.ENTITY_CLEANUP_EXCLUDED_TYPES.behat-steps-skip:entityCleanupAfterScenario@behat-steps-entity-cleanup-skip:<entity_type_id>Traits updated to use the shared registry
Removed per-trait teardown logic and switched to registering entities for the shared cleanup:
MediaTraitFileTrait(managed files only via registry; unmanaged files still cleaned up via unlinking tracked URIs)ParagraphsTraitContentBlockTraitEckTraitMenuTraitRedirectTraitBlockTraitWebformTraitDocumentation and tag validation updates
MIGRATION.mddocumenting the new shared cleanup mechanism, removal/replacement of old per-trait cleanup skip tags, and theFileTraitmanaged vs unmanaged behavior.README.mdwith an “Automatic entity cleanup” section and the new tag-based controls.STEPS.mdto reflect the new lifecycle wording and remove references to old per-trait skip tags.docs.phptag validation to recognize the parametrized tag@behat-steps-entity-cleanup-skip:<value>.Tests
tests/behat/features/drupal_entity_cleanup.featureto verify:tests/behat/features/drupal_block.featuretests/behat/features/drupal_content_block.featuretests/behat/features/drupal_eck.featuretests/phpunit/src/Drupal/HelperTraitTest.phpwith coverage for entity registration and scenario skip-tag parsing.Critical: Step format guideline violations (per
CONTRIBUTING.md)The following feature steps use
Given I ...and/orThen I ..., which conflicts with the documented steps format guidance (“Avoid usingGiven I” / “Avoid usingThen I”):tests/behat/features/drupal_content_block.featureGiven I am logged in ...tests/behat/features/drupal_block.featureThen I should see ...Then I should not see ...tests/behat/features/drupal_eck.featureGiven I am logged in ...