[#367] Updated BlockTrait.
#376
Conversation
|
""" WalkthroughThe changes enhance the block-related Behat testing framework by refining method names, parameter orders, and step definitions for clarity and consistency. The Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Behat
participant BlockTrait
Tester->>Behat: Run feature with block steps
Behat->>BlockTrait: Call block step method (with updated parameter names/order)
BlockTrait-->>Behat: Perform block operation (create/configure/assert/visibility)
Behat-->>Tester: Report result
Poem
🪧 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.x #376 +/- ##
==========================================
- Coverage 81.29% 81.28% -0.02%
==========================================
Files 32 32
Lines 1893 1881 -12
==========================================
- Hits 1539 1529 -10
+ Misses 354 352 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6046b33 to
350bde4
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
src/BlockTrait.php (1)
133-136:⚠️ Potential issueBoolean cast treats
'0'asTRUE– visibility & status flags will be wrongCasting a non-empty string to
boolin PHP always yieldstrue, therefore
'0','false', and any other non-empty string set in the TableNode will
be interpreted astrue.
Blocks configured withlabel_display = 0orstatus = 0will still be saved
as visible and enabled.- case 'label_display': - $settings['label_display'] = (bool) $value; + case 'label_display': + // Accept 0/1, true/false, yes/no, etc. + $settings['label_display'] = filter_var($value, FILTER_VALIDATE_BOOLEAN); ... - case 'status': - $block->setStatus((bool) $value); + case 'status': + $block->setStatus(filter_var($value, FILTER_VALIDATE_BOOLEAN));Also applies to: 142-144
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (5)
MIGRATION.md(1 hunks)src/BlockTrait.php(3 hunks)steps.md(2 hunks)tests/behat/features/block.feature(1 hunks)tests/behat/features/content_block.feature(1 hunks)
🔇 Additional comments (15)
tests/behat/features/content_block.feature (1)
47-48: Improved step definition phrasing for better consistency and readabilityThe step definition updates follow a more direct "[subject] should [state]" pattern instead of "I should see..." which improves readability and maintains consistency with other assertion steps throughout the test suite.
MIGRATION.md (1)
7-19: Well-structured migration guide for BlockTrait step definitionsThe migration mappings for BlockTrait follow a consistent pattern that improves clarity and readability:
- Changing from "When" to "Given" for setup actions
- Removing redundant "with label" phrasing
- Adding definite articles and standardizing parameter order
- Clarifying region specifications
This addition helps users migrate their existing tests from v2 to v3 of the library.
tests/behat/features/block.feature (7)
6-7: Improved test isolation with Background sectionAdding a Background section ensures each scenario starts with a clean state where the test block doesn't exist, improving test isolation and preventing potential interference between scenarios.
11-17: Updated step definitions with clearer, more consistent phrasingBlock creation and verification steps now use more declarative and consistent phrasing, making the test more readable while maintaining the same functionality.
21-23: Added explicit cache clearing for better test reliabilityThe addition of explicit cache clearing steps after disabling the block ensures that the change is properly reflected when testing, improving test reliability.
26-28: Consistent pattern for enabling blocks and cache clearingThe same pattern of explicit cache clearing is applied after enabling the block, ensuring consistency in the test approach.
39-41: Clearer visibility condition configurationThe step for configuring block visibility conditions now uses more descriptive phrasing that clearly indicates what is being configured.
48-50: Explicit condition removal and cache clearingThe step for removing visibility conditions is clearer, and including cache clearing ensures the change is immediately reflected in testing.
62-65: Improved block configuration and assertion stepsThe steps for configuring blocks and verifying their placement in regions use more consistent phrasing that clearly indicates the block's expected state and location.
steps.md (6)
5-5: Updated trait description for clarityThe BlockTrait description has been updated to be more concise and clear about its purpose.
35-43: Updated block creation step definition and exampleThe documentation for block creation now uses "Given" instead of "When" and has clearer parameter naming with "the instance of :admin_label block exists" rather than "I create a block of type :label".
48-55: Improved block configuration step documentationThe block configuration step now uses clearer phrasing that focuses on the block's state rather than the action of configuring it.
87-93: Clearer documentation for visibility condition configurationThe visibility condition configuration step now has more explicit parameter naming and consistent phrasing.
107-111: Standardized block existence assertion stepsThe block existence assertion steps now follow a consistent "[subject] should [state]" pattern, making them more readable and aligned with other assertion steps.
125-130: Improved region-specific block assertion documentationThe documentation for region-specific block assertions now clearly specifies "in the :region region" for improved readability.
src/BlockTrait.php
Outdated
| $this->blockConfigureVisibilityCondition($condition, $label, new TableNode([])); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
blockRemoveVisibilityCondition() doesn’t actually remove the condition
Passing an empty configuration only resets the condition to its defaults.
Use the dedicated unsetVisibilityConfig() helper (available since Drupal 8.8)
or explicit unset() to remove it completely:
- $this->blockConfigureVisibilityCondition($label, $condition, new TableNode([]));
+ $this->blockAssertExists($label);
+ $block = $this->blockLoadByLabel($label);
+ $block->unsetVisibilityConfig($condition);
+ $block->save();Committable suggestion skipped: line range outside the PR's diff.
src/BlockTrait.php
Outdated
| * @Given the instance of :admin_label block exists with the following configuration: | ||
| */ | ||
| public function blockPlaceBlockInRegion(string $label, TableNode $fields): void { | ||
| public function blockCreateInstance(string $admin_label, TableNode $fields): void { | ||
| $block = NULL; | ||
| $block_id = NULL; | ||
|
|
||
| /** @var \Drupal\Core\Block\BlockManagerInterface $block_manager */ | ||
| $block_manager = \Drupal::service('plugin.manager.block'); | ||
| $definitions = $block_manager->getDefinitions(); | ||
| $default_theme = \Drupal::config('system.theme')->get('default'); | ||
| $block = NULL; | ||
| foreach ($definitions as $plugin_id => $definition) { | ||
| if ((string) $definition['admin_label'] === $label) { | ||
| if ((string) $definition['admin_label'] === $admin_label) { | ||
| $default_theme = \Drupal::config('system.theme')->get('default'); | ||
| $block = \Drupal::entityTypeManager()->getStorage('block')->create([ | ||
| 'plugin' => $plugin_id, | ||
| 'theme' => $default_theme, | ||
| ]); | ||
|
|
||
| $suggestion = $block->getPlugin()->getMachineNameSuggestion(); | ||
| $block_id = \Drupal::service('block.repository')->getUniqueMachineName($suggestion, $block->getTheme()); | ||
|
|
||
| $block->set('id', $block_id); | ||
|
|
||
| // Set temporary label to pass to the block configuration step. | ||
| $settings = $block->get('settings'); | ||
| $settings['label'] = $admin_label; | ||
| $block->set('settings', $settings); | ||
|
|
||
| $block->save(); | ||
|
|
||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!$block instanceof Block) { | ||
| throw new \Exception(sprintf('Could not create block of type "%s"', $label)); | ||
| throw new \Exception(sprintf('Could not create block with admin label "%s"', $admin_label)); | ||
| } | ||
| $this->blockConfigureBlockInstance($block, $fields); | ||
| static::$blockInstances[] = $block; | ||
| } | ||
|
|
||
| /** | ||
| * Clean up all blocks created during the scenario. | ||
| * | ||
| * This method automatically runs after each scenario to ensure clean | ||
| * test state. | ||
| * Add the tag @behat-steps-skip:AfterScenario to your scenario to | ||
| * prevent automatic cleanup of blocks. | ||
| * | ||
| * @AfterScenario | ||
| */ | ||
| public function blockAfterScenario(AfterScenarioScope $scope): void { | ||
| if ($scope->getScenario()->hasTag('behat-steps-skip:' . __FUNCTION__)) { | ||
| return; | ||
| } | ||
| $this->blockConfigure($admin_label, $fields); | ||
|
|
||
| foreach (static::$blockInstances as $block) { | ||
| $block->delete(); | ||
| } | ||
| static::$blockInstances = []; | ||
| static::$blockInstances[] = $block; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Potential duplicate “admin label” ambiguity & premature save
-
settings.labelis not unique;blockLoadByLabel()may return a
previously-created block with the same label, causing
$this->blockConfigure()to target the wrong entity.Consider storing the freshly-created block’s ID in a context variable
(or injecting the entity directly) instead of re-loading by label. -
The initial
save()happens before the real configuration is applied,
creating two revision writes where one would suffice.
- $block->save();
- ...
- $this->blockConfigure($admin_label, $fields);
+ // Apply full configuration before the first save to avoid a second write.
+ $this->blockConfigure($admin_label, $fields);
+ $block->save();Committable suggestion skipped: line range outside the PR's diff.
350bde4 to
eee26a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
src/BlockTrait.php (5)
232-235:⚠️ Potential issueParameter order still reversed – step will inject wrong values
The step uses the placeholder order
:label … :condition, yet the method expects
(string $condition, string $label, …).
Behat will pass label to$conditionand condition to$label, breaking
the call (most likely an exception fromblockLoadByLabel()).This was pointed out in a previous review but remains unfixed.
260-263:⚠️ Potential issueSame ordering bug for
blockRemoveVisibilityCondition()
@Given the block :label has the :condition condition removed
↔public function blockRemoveVisibilityCondition(string $condition, string $label)The injected arguments will be swapped, resulting in an attempt to remove a
condition named block-label from a block called condition-id.
160-163: 🧹 Nitpick (assertive)PSR-12 formatting & consistency
public function blockRemove(string $label):voidis still missing the required
space before the return type – "): void".
82-89: 🧹 Nitpick (assertive)Avoid double
save()– write once, not twiceThe block is saved immediately after creation and again after
blockConfigure(). This generates two config-entity revisions and extra I/O
for no benefit.- $block->save(); - ... - $this->blockConfigure($admin_label, $fields); + // Apply full configuration first to avoid an extra write. + $this->blockConfigure($admin_label, $fields); + $block->save();
381-389: 🛠️ Refactor suggestion
end($blocks)returns non-deterministic elementPicking the last array element relies on PHP’s internal hash order, giving
unpredictable results when multiple blocks share a label. Sort by ID (or throw
when duplicates exist) for deterministic behaviour.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (4)
MIGRATION.md(1 hunks)src/BlockTrait.php(3 hunks)steps.md(2 hunks)tests/behat/features/block.feature(1 hunks)
🔇 Additional comments (1)
MIGRATION.md (1)
7-19: Documentation looks great 👍The v2→v3 mapping table is clear and comprehensive.
src/BlockTrait.php
Outdated
| $block_id = NULL; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unused variable $block_id
$block_id is assigned but never referenced afterwards.
If you only need the generated ID on the entity itself, drop the separate
variable.
| Given the block "[TEST] User Account Menu" does not exist | ||
| And the block "[TEST] User Account Menu" does not exist | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Redundant pre-condition
Given the block "[TEST] User Account Menu" does not exist is executed twice in
a row. The second occurrence is redundant and can be removed for clearer
scenarios.
| Given the block "My block" has the following "request_path" condition configuration: | ||
| | pages | /node/1\r\n/about | | ||
| | negate | 0 | | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Literal \r\n sequence may confuse readers
In the example table the value /node/1\r\n/about shows the escaped CR/LF
sequence literally. Consider using a multi-line cell or an explicit bullet
list to illustrate multiple paths.
eee26a3 to
3cb4dce
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tests/behat/features/content_block.feature (1)
6-24: 🧹 Nitpick (assertive)Consider moving repeated login & pre-conditions into a Background for clarity
All four scenarios begin with identical steps:
Given I am logged in as a user with the "administrator" role And the content block type "basic" should exist And the following "basic" content blocks do not exist: …Extracting these into a single
Background:section removes duplication,
shortens the scenarios and makes the actual intent of each scenario pop out
immediately.Diff sketch (outside the shown lines):
Background: Given I am logged in as a user with the "administrator" role And the content block type "basic" should exist And the following "basic" content blocks do not exist: | … |Each scenario can then start with the creation step.
♻️ Duplicate comments (6)
tests/behat/features/block.feature (1)
33-35: Redundant pre-condition – duplicate “does not exist” step
Given the block "[TEST] User Account Menu" does not existis executed twice
in a row. The second invocation is redundant and can safely be removed.src/BlockTrait.php (4)
261-263:⚠️ Potential issueIncorrect argument order re-opens the earlier bug
The first argument of
blockConfigureVisibilityCondition()must be$label, but the call here passes$conditionfirst.
This will silently create/overwrite a different visibility config on the wrong block.- $this->blockConfigureVisibilityCondition($condition, $label, new TableNode([])); + $this->blockConfigureVisibilityCondition($label, $condition, new TableNode([]));(You may also want to replace the “empty table” trick with an explicit removal API; see next comment.)
388-391: 🧹 Nitpick (assertive)Block selection is still nondeterministic
krsort()followed byend()returns the oldest entity after an
implementation-defined hash sort.
If multiple blocks share the label you may test a different one each run.Consider:
- krsort($blocks); - return empty($blocks) ? NULL : end($blocks); + uasort($blocks, fn(Block $a, Block $b) => $b->id() <=> $a->id()); // newest first + return reset($blocks) ?: NULL;or throw when duplicates exist.
62-88: 🛠️ Refactor suggestionAvoid the double write: save only after full configuration
$block->save()is called before the real configuration is applied, causing
two entity revisions and extra database I/O.- $block->save(); - - break; + break; } @@ - $this->blockConfigure($admin_label, $fields); - - static::$blockInstances[] = $block; + $this->blockConfigure($admin_label, $fields); + $block->save(); + + static::$blockInstances[] = $block;
245-263: 🛠️ Refactor suggestionUse the dedicated removal API instead of “configure with an empty table”
Resetting the condition to its defaults does not remove the visibility
restriction. Drupal ≥8.8 exposesunsetVisibilityConfig()for this purpose.- public function blockRemoveVisibilityCondition(string $label, string $condition): void { - $this->blockConfigureVisibilityCondition($label, $condition, new TableNode([])); + public function blockRemoveVisibilityCondition(string $label, string $condition): void { + $this->blockAssertExists($label); + $block = $this->blockLoadByLabel($label); + + if ($block->getVisibilityConfig()[$condition] ?? FALSE) { + $block->unsetVisibilityConfig($condition); + $block->save(); + } }steps.md (1)
90-93: Literal “\r\n” may confuse readers (unchanged)The example cell shows the escaped CR/LF sequence verbatim:
/node/1\r\n/about.Readers may interpret this as two characters rather than a line break. Use a
multi-line cell instead:| pages | """ /node/1 /about """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (5)
MIGRATION.md(1 hunks)src/BlockTrait.php(3 hunks)steps.md(2 hunks)tests/behat/features/block.feature(1 hunks)tests/behat/features/content_block.feature(5 hunks)
🔇 Additional comments (2)
tests/behat/features/content_block.feature (1)
17-19: Verify the availability of the step “I go to …”The PathTrait examples (and the migration table) still refer to the step
When I visit :path. If the library does not register an alias for
I go to, these steps will be undefined at runtime.Please confirm that
I go tois wired up, or revert to the canonical
When I visit "/admin/content/block"/When I visit "/"wording.tests/behat/features/block.feature (1)
19-21: Same “I go to …” alias question applies hereThe scenario uses
When I go to "/". Please double-check that this alias
is registered; otherwise the step will be undefined. If not, replace with
When I visit "/"to match existing PathTrait definitions.
4012d49 to
6d925cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
tests/behat/features/content_block.feature (2)
8-10: 🧹 Nitpick (assertive)Consider extracting repeated login & pre-conditions into a
BackgroundblockEach scenario starts with
Given I am logged in as a user with the "administrator" role
and
And the content block type "basic" should exist.Moving these two ubiquitous pre-conditions into a single
Background:section would make the scenarios shorter, remove duplication, and surface only the behaviour that differs per scenario.Example:
Background: Given I am logged in as a user with the "administrator" role And the content block type "basic" should existThis keeps all scenarios readable while still exercising the same paths.
Also applies to: 26-28, 40-43
14-16: 🧹 Nitpick (assertive)Keep table headers in a consistent order
The column order for the “basic” block tables changes from
info | status | body(lines 14-16) toinfo | body | status(line 42).
Mixing header order makes scenarios harder to read and increases the chance of copying the wrong format when tests are extended.Align the later table with the earlier one:
- | info | body | status | + | info | status | body |(or vice-versa, provided you pick one convention and stick to it throughout the suite).
Also applies to: 42-43
♻️ Duplicate comments (7)
MIGRATION.md (1)
16-17: 🧹 Nitpick (assertive)“When” → “Then” still incorrect in v2 mapping
These two rows keep the v2 step keyword as
When, but the original v2 steps were actually assertions (Then).
The mismatch was raised in a previous review and is still unresolved.-| `When block with label :label should exist in the region :region` -| `When block with label :label should not exist in the region :region` +| `Then block with label :label should exist in the region :region` +| `Then block with label :label should not exist in the region :region`Keeping the correct keyword prevents users from blindly copy-pasting an inaccurate mapping and wondering why their scenarios read strangely.
steps.md (1)
90-93: 🧹 Nitpick (assertive)Literal “\r\n” in example table may confuse readers
In the example value the carriage-return / line-feed escape sequence is rendered literally:
| pages | /node/1\r\n/about |Most readers will not expect to paste an escaped CR-LF sequence.
Consider showing it as a real multi-line cell (or bullet list) instead:-| pages | /node/1\r\n/about | +| pages | /node/1 +/about |or add a short note that multiple lines are allowed. This tiny adjustment improves copy-pasting accuracy.
src/BlockTrait.php (4)
261-263:⚠️ Potential issueParameter order still swapped – step will fail
blockRemoveVisibilityCondition()callsblockConfigureVisibilityCondition()with the parameters in reverse order, reproducing the bug flagged in the previous review.- $this->blockConfigureVisibilityCondition($condition, $label, new TableNode([])); + $this->blockConfigureVisibilityCondition($label, $condition, new TableNode([]));Without this fix Behat will inject label as condition and vice-versa, yielding “Unknown visibility condition” errors.
245-263: 🛠️ Refactor suggestionCondition is reset, not removed – use
unsetVisibilityConfig()Passing an empty config merely resets the condition to its defaults; the visibility entry still exists.
Drupal ≥ 8.8 exposesunsetVisibilityConfig($id)for true removal.- $this->blockConfigureVisibilityCondition($label, $condition, new TableNode([])); + $this->blockAssertExists($label); + $block = $this->blockLoadByLabel($label); + $block->unsetVisibilityConfig($condition); + $block->save();This guarantees the condition is gone, not silently set to default values.
81-87: 🛠️ Refactor suggestionAvoid the double–write: configure the block before the first save
blockCreateInstance()persistently saves the entity (line 86) before handing it over toblockConfigure(), which then loads the same block again and saves once more.
This produces two revision writes where one would suffice, slowing down test runs and polluting the revision table.A leaner flow is:
- Apply the configuration contained in
$fieldsdirectly on the freshly-created$block.- Call
$block->save()only once after all properties are in place.- Drop the subsequent
blockConfigure()call (or refactor it to accept aBlockargument so it can act in-memory).- $block->set('settings', $settings); - $block->save(); - - break; + $block->set('settings', $settings); + + // Apply additional fields before the first (and only) save. + $this->applyBlockFieldConfiguration($block, $fields); // helper you’d introduce + $block->save(); + + break;Doing so eliminates an unnecessary service round-trip and keeps revision history clean.
380-391: 🧹 Nitpick (assertive)Non-deterministic block selection when duplicates exist
krsort($blocks); end($blocks);relies on PHP array keys (entity IDs) and still picks the last element after a descending sort.
If multiple blocks share the same label, the chosen entity changes with every new ID, leading to flaky tests.Recommended approaches:
- krsort($blocks); - return empty($blocks) ? NULL : end($blocks); + // Sort by numeric ID for repeatable order and always take the *first*. + uasort($blocks, static fn($a, $b) => $a->id() <=> $b->id()); + return reset($blocks) ?: NULL;Better yet, detect duplicates and throw to force unique labels in fixtures.
tests/behat/features/block.feature (1)
32-35: Duplicate “block does not exist” step – already flagged previouslyThe step is executed twice intentionally, but it was highlighted in a past review as redundant. If the goal is to prove idempotency you could add an inline comment or wrap it in a Scenario Outline to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (5)
MIGRATION.md(1 hunks)src/BlockTrait.php(3 hunks)steps.md(2 hunks)tests/behat/features/block.feature(1 hunks)tests/behat/features/content_block.feature(5 hunks)
🔇 Additional comments (1)
tests/behat/features/block.feature (1)
19-20: Mixing “I go to” and “I visit” steps may hide coverage gapsThe file switches between
When I go to "/" When I visit "/"If both phrases map to the same underlying step this is harmless, but if they differ (e.g. one clears cache or waits for AJAX) the test intent becomes ambiguous. Standardise on one phrasing or add a comment explaining why both are required.
Also applies to: 180-184
| | label | [TEST] Content Block | | ||
| | label_display | 1 | | ||
| | region | content | | ||
| | status | 1 | | ||
| Then I should see the block with label "[TEST] Content Block" | ||
| And I should see the block with label "[TEST] Content Block" in the region "content" | ||
| Then the block "[TEST] Content Block" should exist |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Redundant status setting between content block and block instance
status is set to 1 in both the content-entity creation table (lines 40-42) and again inside the block-instance configuration table (lines 44-47).
Unless the underlying step definitions require both values, dropping the duplicate in the instance configuration reduces noise.
- | region | content |
- | status | 1 |
+ | region | content || | label | [TEST] User Account Menu | | ||
| | label_display | 1 | | ||
| | region | content | | ||
| | status | 1 | | ||
| Then I should see the block with label "[TEST] User Account Menu" in the region "content" | ||
| When I configure the block with the label "[TEST] User Account Menu" with: | ||
| Then the block "[TEST] User Account Menu" should exist in the "content" region | ||
|
|
||
| Given the block "[TEST] User Account Menu" has the following configuration: | ||
| | region | header | | ||
| Then I should see the block with label "[TEST] User Account Menu" in the region "header" | ||
| And I should not see the block with label "[TEST] User Account Menu" in the region "content" | ||
| Then the block "[TEST] User Account Menu" should exist in the "header" region | ||
| And the block "[TEST] User Account Menu" should not exist in the "content" region |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Possible missing cache clear after block move
After updating the block’s region configuration (line 227), the scenario immediately asserts the new placement (line 225-231). Depending on Drupal caching, the move may not be visible until caches are cleared. Consider adding the existing And the cache has been cleared step used elsewhere to avoid intermittent failures on slow CI runners.
| @trait:BlockTrait @api | ||
| Scenario: Assert "block should exist" fails for non-existing block | ||
| Given some behat configuration | ||
| And scenario steps: | ||
| """ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tag consistency – use @trait:BlockTrait everywhere or drop it
Most negative-test scenarios are tagged with @trait:BlockTrait @api, yet the positive scenarios (e.g. lines 9-31, 170-190, 219-231) only have @api. Using a consistent tag makes it trivial to run all BlockTrait tests via behat --tags=trait:BlockTrait.
Recommendation: add @trait:BlockTrait to the positive scenarios (or remove it from the negatives) so the suite can be filtered predictably.
6d925cf to
700fe3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
tests/behat/features/content_block.feature (1)
43-47: 🧹 Nitpick (assertive)Consider removing redundant status setting.
The
statusfield is set to1in both the content block entity creation table (line 42) and again in the block instance configuration table (line 47). Unless both settings are required by the underlying implementation, removing the duplicate in the instance configuration would reduce noise.| label | [TEST] Content Block | | label_display | 1 | | region | content | - | status | 1 |tests/behat/features/block.feature (2)
227-230: Still no cache clear after a region changeWe previously pointed out that immediately asserting a new placement after changing the
regionconfiguration can be flaky on some CI runners due to cached render arrays. You have not added theAnd the cache has been clearedstep here, so the same intermittent-failure risk remains.
9-10: 🧹 Nitpick (assertive)Add
@trait:BlockTraittag for consistencyThe positive scenario is still tagged only with
@api, while every negative scenario carries@trait:BlockTrait @api. Keeping the tag set uniform makes it trivial to run all trait-specific tests viabehat --tags=trait:BlockTrait.src/BlockTrait.php (3)
261-263:⚠️ Potential issue
blockRemoveVisibilityCondition()still only resets, it doesn’t removePassing an empty
TableNodecallssetVisibilityConfig()with an empty array, which merely resets the condition to its defaults.
UseunsetVisibilityConfig()(Drupal 8.8+) orunset()to delete it outright, as previously suggested.
298-304: 🧹 Nitpick (assertive)Doc-comment still contradicts behaviour
blockAssertNotExists()throws when the block does exist, but the@throwsannotation claims it throws “When no block … is found”. Please update the comment to match reality.
80-88: 🛠️ Refactor suggestionAvoid the double-save – configure before persisting
The block entity is saved once here (line 86) with only a temporary label and then saved again inside
blockConfigure().
This produces an unnecessary write and two revisions.- $block->save(); - - ... - $this->blockConfigure($admin_label, $fields); + // Apply the full configuration before the first save to avoid a + // second write. + $this->blockConfigure($admin_label, $fields); + $block->save();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (6)
MIGRATION.md(1 hunks)src/BlockTrait.php(3 hunks)steps.md(2 hunks)tests/behat/features/block.feature(1 hunks)tests/behat/features/content_block.feature(5 hunks)tests/behat/features/wait.feature10000 (2 hunks)
🔇 Additional comments (10)
tests/behat/features/wait.feature (1)
6-6: Improved scenario tagging approach.Moving tags from the feature level to individual scenarios creates more precise test execution contexts. This change allows the first scenario to run with just the
@apitag while the second scenario retains both@apiand@javascripttags as it tests AJAX functionality.Also applies to: 17-17
MIGRATION.md (2)
7-19: Complete and well-structured BlockTrait migration mapping.The new migration mapping for
BlockTraitprovides a clear path from v2 to v3 step definitions. The changes appropriately:
- Rename steps from "When" to "Given" or "Then" to better reflect their purpose
- Standardize phrasing by adding articles ("the") and simplifying terminology
- Clearly indicate which steps have been removed (visibility condition checks)
This mapping will help users transition their existing tests to the new standardized format.
16-17: Thanks for fixing the step keyword in the migration table.The rows correctly show "Then" steps in both the original v2 and new v3 columns, addressing the previous review comment about using the correct assertion step keyword.
tests/behat/features/content_block.feature (3)
6-6: Improved tag granularity by moving @api to scenario level.Tags are now properly applied at the scenario level rather than the feature level, improving execution context specificity for each test scenario.
Also applies to: 24-24, 38-38, 54-54, 64-64, 84-84
40-49: Standardized block instance creation and verification syntax.Steps now use consistent phrasing with "Given the instance of..." and "Then the block... should exist" patterns, aligning with the updated BlockTrait interface. This improves readability and maintains consistency across the codebase.
75-79: Consistent block creation syntax in updated scenario.Step definition now uses the standardized "Given the instance of... block exists with the following configuration" syntax, matching the other scenarios and maintaining consistent phrasing throughout the test suite.
steps.md (4)
5-5: Clearer BlockTrait description.The updated description "Create, configure, and test block" is more concise and accurately reflects the trait's purpose.
35-43: Standardized step definitions with improved semantics.The BlockTrait step definitions have been thoughtfully standardized with:
- More appropriate step keywords (
@Givenfor setup and@Thenfor assertions)- Consistent singular phrasing ("the block" instead of "block with label")
- Clearer parameter naming
- Addition of articles for improved readability
- Simplified, more direct language
These changes make the documentation more consistent and the step definitions more intuitive to use.
Also applies to: 48-55, 60-64, 69-73, 78-82, 87-93, 98-102, 107-111, 116-120, 125-129, 134-138
90-92: Clear example for condition configuration.The example showing visibility condition configuration with paths is now clearer. The
/node/1\r\n/aboutformat might be better presented as a bullet list in future updates for even greater clarity, but it's sufficient as is.
101-101: Concrete condition value in example.The example now uses a concrete condition value "request_path" instead of a placeholder, making it copyable for users - addressing a previous review comment.
| krsort($blocks); | ||
|
|
||
| return reset($blocks); | ||
| return empty($blocks) ? NULL : end($blocks); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
blockLoadByLabel() selection is still non-deterministic
krsort() + end() relies on PHP array-key order.
If several blocks share the same label you might pick an arbitrary one.
Load by id (ascending) or throw on duplicates instead of relying on hash order.
700fe3b to
fdfd7be
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
MIGRATION.md (1)
16-17: Step keyword corrected from "When" to "Then" as per previous feedback.The block existence verification steps are now correctly categorized as assertions (
Then) in the migration mapping, which aligns with their actual purpose and fixes the previously identified inconsistency.tests/behat/features/content_block.feature (1)
89-92: 🧹 Nitpick (assertive)Block instance configuration contains redundant status setting.
The
statusfield appears to be set to1in both the content block entity (line 87) and again in the block instance configuration (line 92). Consider removing the duplicate setting in the instance configuration to reduce noise.| label | [TEST] Content Block | | label_display | 1 | | region | content | - | status | 1 |steps.md (1)
90-93: 🧹 Nitpick (assertive)Example uses literal
\r\nwhich may confuse users.The example shows
\r\nliterally in the paths value, which might confuse readers about how to format multiple paths. Consider using a clearer demonstration of multiple paths, such as a bulleted list or explicit note.tests/behat/features/block.feature (1)
9-10: Tag consistency – positive scenarios still miss@trait:BlockTraitEarlier feedback (see past comments) recommended tagging every BlockTrait-related scenario with
@trait:BlockTraitso that the whole trait can be executed viabehat --tags=trait:BlockTrait.
The positive scenario starting here is still missing the tag, whereas the negative scenarios further below have it.Adding the tag (or removing it from the negatives) will make the suite easier to filter.
src/BlockTrait.php (2)
374-390: Block selection is still non-deterministicUsing
krsort()+end()relies on PHP’s hash order; if multiple blocks share the same label you may pick an arbitrary one.
Earlier reviews already flagged this – consider ordering by entity ID or throwing on duplicates for predictable behaviour.
261-263:⚠️ Potential issueVisibility condition is reset, not removed
Passing an empty
TableNodemerely resets the condition to defaults; it does not remove it.
Drupal ≥ 8.8 providesunsetVisibilityConfig()for this purpose.- public function blockRemoveVisibilityCondition(string $label, string $condition): void { - $this->blockConfigureVisibilityCondition($label, $condition, new TableNode([])); + public function blockRemoveVisibilityCondition(string $label, string $condition): void { + $this->blockAssertExists($label); + $block = $this->blockLoadByLabel($label); + + $block->unsetVisibilityConfig($condition); + $block->save(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (6)
MIGRATION.md(1 hunks)src/BlockTrait.php(3 hunks)steps.md(2 hunks)tests/behat/features/block.feature(1 hunks)tests/behat/features/content_block.feature(6 hunks)tests/behat/features/wait.feature(2 hunks)
🔇 Additional comments (21)
tests/behat/features/wait.feature (2)
6-6: Tag scope narrowed appropriately.The
@apitag has been applied directly to this scenario instead of at the feature level, providing more precise control over test execution requirements.
17-17: Tags correctly specify required driver capabilities.The
@api @javascripttags ensure this AJAX-related scenario explicitly indicates its JavaScript dependency, making it clear that this test requires a JavaScript-capable driver.MIGRATION.md (1)
7-19: Complete BlockTrait migration map added.The migration table for BlockTrait provides a clear mapping between v2 and v3 step definitions, which is essential for users upgrading their test suites. The changes consistently apply the new naming conventions and step categorizations.
Key improvements:
- Steps properly categorized as
GivenorThenbased on their purpose- Use of definite articles ("the") for improved readability
- Parameter naming standardization (
:label→:admin_labelwhere appropriate)- Clear indication of removed steps
tests/behat/features/content_block.feature (9)
6-15: New scenario added to verify content block type existence.This scenario properly tests the content block type verification functionality with appropriate tagging. It creates a test block and verifies the block type exists.
16-28: Added negative test for content block type validation.This is a well-structured test that verifies the proper error message when attempting to check a non-existent content block type. Good practice to include negative test cases.
47-54: Added test to verify non-existent content blocks don't cause failures.This scenario confirms that the step for checking non-existence of content blocks works correctly when the blocks already don't exist, which is an important edge case to test.
69-82: Added negative test for editing non-existent content block.This scenario confirms that attempting to edit a non-existent content block produces the expected error message, providing thorough test coverage for error handling.
85-94: Updated block creation syntax.The step has been updated from "When I create a block..." to "Given the instance of... block exists..." which aligns with the new naming conventions in BlockTrait. The surrounding assertions use the new syntax as well.
99-115: Added test for handling non-existent blocks in instance creation.This scenario properly tests the error case when attempting to create a block instance with a non-existent block, verifying that the correct error message is displayed.
127-134: Updated block creation syntax in content update scenario.The step for creating a block instance has been updated to use the new "Given the instance of... block exists" syntax, maintaining consistency with the new naming conventions.
150-163: Added comprehensive test for block deletion workflow.This scenario provides a complete test of the content block lifecycle, creating, verifying, deleting, and confirming deletion of a block. This is good practice to ensure the entire workflow functions correctly.
164-171: Added test for deleting non-existent content block.This scenario confirms that attempting to delete a content block that doesn't exist doesn't cause failures, which is important for robustness in test suites.
steps.md (8)
5-5: BlockTrait description simplified.The description has been made more concise while still accurately describing the trait's functionality.
35-43: Updated block creation step definition.The step has been renamed from
@When I create a block of type :label with:to@Given the instance of :admin_label block exists with the following configuration:, which better reflects its purpose as a setup step rather than an action.
48-56: Updated block configuration step definition.The step has been renamed from
@When I configure the block with the label :label with:to@Given the block :label has the following configuration:, which is more consistent with the new naming pattern.
60-66: Added step to specify block non-existence.A new step
@Given the block :label does not existhas been added, which provides a direct way to ensure a block doesn't exist before running tests.
69-84: Renamed block enable/disable steps.The enable/disable steps have been renamed to use more direct phrasing:
@Given the block :label is enabledand@Given the block :label is disabled, which is clearer than the previous versions.
87-94: Updated block condition configuration step.The condition configuration step has been updated to use clearer phrasing and includes a proper example with request_path condition.
98-103: Updated block condition removal step.The step for removing conditions has been updated to use consistent phrasing with other steps, and the example now uses a concrete condition type ("request_path") instead of the placeholder.
107-140: Updated block existence verification steps.The verification steps have been updated to use assertion phrasing (
@Then the block :label should exist), which better reflects their purpose as assertions rather than actions.tests/behat/features/block.feature (1)
32-34: Idempotency check acknowledgedNice touch adding the explicit comment explaining why the step is repeated.
Running the “block does not exist” step twice is a neat way to verify idempotency and no further action is required.
tests/behat/features/block.feature
Outdated
| When I go to "/" | ||
| Then I should see "[TEST] User Account Menu" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mixed “go to” vs “visit” step keywords
Most scenarios were migrated to the new wording When I go to "/", yet lines 180-185 still use the old When I visit "/".
If the old step definition was removed (as suggested by the PR description), these scenarios will break.
Please harmonise the wording to avoid dangling step definitions or runtime errors.
Also applies to: 180-185
451823d to
c1a2264
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
MIGRATION.md (1)
16-17: Doc rows correctly show original step keywords.The migration table properly shows that the v2 assertion steps were originally
Whensteps despite being assertions. This matches the historical implementation and will help users accurately identify which steps to replace when migrating.tests/behat/features/content_block.feature (1)
83-98: 🧹 Nitpick (assertive)Redundant
statussetting between content block and block instance.The
statusis set to1in both the content-entity creation table (line 87) and again inside the block-instance configuration table (line 92). Unless both values are required by the underlying implementation, this creates unnecessary duplication.Consider removing the redundant status setting from the block instance configuration:
| label_display | 1 | | region | content | - | status | 1 |steps.md (1)
87-93: 🧹 Nitpick (assertive)Consider improving the readability of the multi-line example value.
The example uses
\r\nto represent newlines in thepagesfield, which may be confusing to readers who aren't familiar with this notation. This could be represented more clearly.Consider using a more readable format for representing multiple values:
-| pages | /node/1\r\n/about | +| pages | /node/1 | +| | /about |Or alternatively:
-| pages | /node/1\r\n/about | +| pages | [multiple paths]: | +| | - /node/1 | +| | - /about |src/BlockTrait.php (4)
261-263:blockRemoveVisibilityCondition()does not actually remove the conditionPassing an empty configuration only resets the condition; the condition object
still exists and will be evaluated with default settings.Use
unsetVisibilityConfig()(available since Drupal 8.8) or explicitunset():- public function blockRemoveVisibilityCondition(string $label, string $condition): void { - $this->blockConfigureVisibilityCondition($label, $condition, new TableNode([])); + public function blockRemoveVisibilityCondition(string $label, string $condition): void { + $this->blockAssertExists($label); + $block = $this->blockLoadByLabel($label); + $block->unsetVisibilityConfig($condition); + $block->save(); }
388-390: Selection remains non-deterministic when duplicate labels exist
krsort()+end()relies on PHP’s array-key order; if several blocks share
the same visible label you may randomly pick different ones between runs.
Consider ordering byid(ascending) or throwing on duplicates instead.
161-165: 🛠️ Refactor suggestionKeep the static registry in sync after explicit removals
blockRemove()deletes matching entities but never removes them fromstatic::$blockInstances.
If the same label was registered earlier,blockAfterScenario()will attempt a second delete and explode.while ($block = $this->blockLoadByLabel($label)) { $block->delete(); } + + // Purge the registry so teardown stays idempotent. + static::$blockInstances = array_filter( + static::$blockInstances, + fn(Block $b) => $b->get('settings')['label'] !== $label + );
42-47: 🛠️ Refactor suggestionMake teardown resilient to blocks deleted inside scenarios
blockAfterScenario()unconditionally calls$block->delete().
If a scenario already removed the entity (e.g. viaGiven the block … does not exist), the second delete triggers anEntityStorageException, failing the suite during@afterScenario.- foreach (static::$blockInstances as $block) { - $block->delete(); - } + foreach (static::$blockInstances as $key => $block) { + try { + $block->delete(); + } + // Ignore “already deleted” errors so teardown is idempotent. + catch (\Drupal\Core\Entity\EntityStorageException $e) { + // no-op + } + unset(static::$blockInstances[$key]); + }tests/behat/features/block.feature (3)
32-35: Redundant “block does not exist” step executed twiceLines 33–34 repeat the same pre-condition and add noise to the scenario without
providing extra safety. Deleting the second line keeps the intent clear.
19-20: Mixed “visit” vs “go to” wording may leave dangling step definitionsNew wording elsewhere uses
When I go to "/", but these lines still rely on
When I visit "/". If the old step definition was removed (per migration
guide) these steps will break at runtime. Harmonise the vocabulary.Also applies to: 183-184
227-231: Possible missing cache clear after moving the blockAfter changing the region configuration (line 227) the scenario immediately
asserts the new placement. Depending on Drupal caching, the move might not be
visible until caches are cleared, causing flakes on slow CI runners. Consider
addingAnd the cache has been clearedbefore the assertions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (6)
MIGRATION.md(1 hunks)src/BlockTrait.php(3 hunks)steps.md(2 hunks)tests/behat/features/block.feature(1 hunks)tests/behat/features/content_block.feature(6 hunks)tests/behat/features/wait.feature(2 hunks)
🔇 Additional comments (14)
tests/behat/features/wait.feature (1)
6-6: Improved test organization with scenario-level tagging.The changes add specific Behat tags (
@apiand@javascript) to individual scenarios rather than at the feature level. This provides more granular control over which scenarios require API access or JavaScript capabilities.Also applies to: 17-17
MIGRATION.md (1)
7-19: Well-structured BlockTrait migration documentation.The new BlockTrait migration mapping clearly documents how v2 step definitions map to v3 equivalents. The changes reflect a shift to more semantic and consistent step definitions:
- Verb changes from "When" to "Given" for setup steps
- Improved phrasing with "the block" instead of "block with label"
- Standardized organization of parameters
- Clearly marked removed steps
This addition provides essential guidance for users migrating from v2 to v3.
tests/behat/features/content_block.feature (7)
6-14: Added comprehensive test for content block type validation.This new scenario provides proper coverage for verifying that content block types exist, including testing for a basic content block type and checking the admin UI to confirm visibility.
16-28: Added negative test scenario for non-existent block types.This scenario verifies the error handling when attempting to validate non-existent content block types, ensuring the proper error message is displayed. This complements the positive test case and ensures robust error handling.
47-54: Added idempotency test for "blocks do not exist" operation.This scenario verifies that the "Given the following content blocks do not exist" step does not fail when the blocks already don't exist. This ensures the step is safe to use in setups without knowledge of current state.
69-82: Added test for failure handling with non-existent content blocks.This scenario ensures that attempting to edit a non-existent content block produces the expected error message, providing test coverage for error handling in this edge case.
99-115: Added failure test for invalid block instances.This scenario properly tests that attempting to create a block instance with a non-existent block admin label fails with the appropriate error message.
150-163: Added complete block lifecycle test.This scenario provides end-to-end testing of the creation and deletion of content blocks, verifying UI state along the way. This is an important test for ensuring the complete lifecycle of content blocks functions correctly.
164-171: Added safety check for deleting non-existent blocks.This scenario ensures that attempting to delete non-existent content blocks doesn't fail, which is important for idempotent operations in test setups.
steps.md (5)
5-5: Updated BlockTrait description.The description has been simplified and clarified to better reflect the functionality provided by the trait.
35-43: Improved block creation step definition syntax.The step definition has been changed from
@When I create a block of type :label with:to@Given the instance of :admin_label block exists with the following configuration:. This makes the intent clearer and shifts from an action-focused to a state-focused approach, which is better for test setup.
48-56: Improved block configuration step definition 3D11 .The step definition has been changed from
@When I configure the block with the label :label with:to@Given the block :label has the following configuration:. This makes the intent clearer and shifts to a state-focused approach.
98-103: Improved block condition removal step.The step definition has been changed from
@When I remove the visibility condition :condition from the block with label :labelto@Given the block :label has the :condition condition removed. The example now correctly demonstrates a concrete condition name ("request_path") rather than a placeholder.
107-140: Standardized block assertion step definitions.The assertion steps have been standardized to follow a consistent pattern:
@Then the block :label should exist@Then the block :label should not exist@Then the block :label should exist in the :region region@Then the block :label should not exist in the :region regionThis consistency makes the steps more intuitive and easier to remember.
c1a2264 to
e675bda
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
MIGRATION.md (1)
16-17: Correctly addressed previous review feedbackThe mapping now correctly shows that these block existence checks are "Then" steps in v3, properly reflecting their purpose as assertions rather than actions.
tests/behat/features/content_block.feature (1)
86-92: 🧹 Nitpick (assertive)Redundant
statussetting between content block and block instanceThe
statusfield is set to1in both the content block creation table and the block instance configuration table. Unless both settings are required by the underlying implementation, one could be removed to reduce redundancy.| info | body | status | | [TEST] Basic Block | [TEST] Body content | 1 | And the instance of "[TEST] Basic Block" block exists with the following configuration: | label | [TEST] Content Block | | label_display | 1 | | region | content | - | status | 1 | + | status | TRUE |Alternatively, if both status fields are serving different purposes, consider adding a code comment to clarify the distinction.
steps.md (1)
90-92: 🧹 Nitpick (assertive)Literal
\r\nsequence may confuse readersThe example still shows the escaped CR/LF sequence literally (
/node/1\r\n/about).
Consider using a multi-line cell or bullet list so the rendered Markdown is clearer to readers and avoids copy/paste surprises.tests/behat/features/block.feature (3)
19-21: Step wording diverges from the new “go to” conventionMost scenarios were migrated to
When I go to "/", yet these still useWhen I visit "/".
If the legacy step definition was removed during the refactor, these steps will break at runtime.
9-11: 🧹 Nitpick (assertive)Tag consistency – add
@trait:BlockTraitto positive scenarioNegative scenarios carry the tag, but this main happy-path scenario does not.
Keeping tags consistent makes it trivial to run all BlockTrait tests:
behat --tags=trait:BlockTrait.
227-231: 🧹 Nitpick (assertive)Possible missing cache clear after changing region
Immediately asserting the new placement after updating the
regionconfiguration can intermittently fail on some CI runners due to cached render arrays.
Consider insertingAnd the cache has been clearedbefore the assertions, mirroring the pattern used elsewhere in the file.src/BlockTrait.php (3)
271-273: 🛠️ Refactor suggestion
⚠️ Potential issue
blockRemoveVisibilityCondition()does not actually remove the conditionPassing an empty configuration only resets to defaults. Use
unsetVisibilityConfig()(Drupal ≥ 8.8) or explicitunset()so the condition is removed:- $this->blockConfigureVisibilityCondition($label, $condition, new TableNode([])); + $this->blockAssertExists($label); + $block = $this->blockLoadByLabel($label); + + if (method_exists($block, 'unsetVisibilityConfig')) { + $block->unsetVisibilityConfig($condition); + } + else { + $block->setVisibilityConfig($condition, []); + } + + $block->save();
86-92: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid double-save when creating a block
blockCreateInstance()saves the entity (line 91) and thenblockConfigure()saves it again (line 151).
This creates an extra revision and doubles the I/O cost.- // Temporary label… - $block->set('settings', $settings); - - $block->save(); + // Temporary label… + $block->set('settings', $settings);
blockConfigure()will persist the entity once the full configuration is applied.
398-400: 🧹 Nitpick (assertive)Non-deterministic selection when multiple blocks share a label
krsort()+end()depends on PHP’s internal hash order. If two blocks have the same label, a random one may be picked between runs.
Sort deterministically (e.g. byid) or throw when duplicates are found.- krsort($blocks); - return empty($blocks) ? NULL : end($blocks); + if (count($blocks) > 1) { + usort($blocks, fn(Block $a, Block $b) => $a->id() <=> $b->id()); + } + return $blocks ? reset($blocks) : NULL;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (7)
MIGRATION.md(1 hunks)src/BlockTrait.php(3 hunks)src/ContentBlockTrait.php(2 hunks)steps.md(2 hunks)tests/behat/features/block.feature(1 hunks)tests/behat/features/content_block.feature(6 hunks)tests/behat/features/wait.feature(2 hunks)
🔇 Additional comments (4)
tests/behat/features/wait.feature (1)
6-6: Good improvement to tag granularityMoving from feature-level tags to scenario-level tags provides better test isolation and more precise control over which scenarios require JavaScript capabilities.
Also applies to: 17-17
MIGRATION.md (1)
7-19: Well-structured migration documentation for BlockTraitThis comprehensive migration map clearly guides users from v2 to v3 step definitions, with consistent naming conventions and appropriate step types (Given/Then) that match their actual purpose.
src/ContentBlockTrait.php (1)
10-11: Improved robustness in entity cleanupGood enhancement to the cleanup process by adding exception handling and individual entity removal from the tracking array. This prevents test teardown failures when entities can't be deleted or have already been removed, making tests more reliable.
Also applies to: 39-47
tests/behat/features/content_block.feature (1)
6-171: Excellent expansion of test coverageThe addition of both positive and negative test scenarios greatly improves test coverage. The new scenarios systematically validate correct behavior as well as proper error handling for edge cases.
| * @return \Drupal\block\Entity\Block|null | ||
| * The loaded block entity or NULL if not found. | ||
| * | ||
| * @throws \Exception | ||
| * When no block with the specified label is found. | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Doc-block contradicts implementation
The @throws annotation claims an exception is thrown “when no block is found”, yet the method now returns null instead.
Update the comment (or restore the exception) to keep documentation accurate.
Updated
BlockTrait.Summary by CodeRabbit