Migrated to 'drupal/drupal-extension' 5.4 and 'drupal/drupal-driver' 3.x.#631
Migrated to 'drupal/drupal-extension' 5.4 and 'drupal/drupal-driver' 3.x.#631AlexSkrypnyk wants to merge 7 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDependency management is updated to switch from fixed Drupal extension constraints to dev-feature aliases and replace the Selenium2 Mink driver. Driver capability checks are introduced in entity and field handling traits to enforce interface compliance. Behat CLI timeout is increased to accommodate longer bootstrap scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Around line 23-25: composer.json currently depends on unpinned dev branches
drupal/drupal-driver (dev-feature/ext-smoke) and drupal/drupal-extension
(dev-feature/drupal-driver-3x) which can break downstream installs; either
replace those entries with stable constraint strings (e.g., change
"drupal/drupal-driver": "dev-feature/ext-smoke as 3.0" to a released semver like
"drupal/drupal-driver": "^3.0" and similarly for "drupal/drupal-extension"), or
if you must keep fork branches, document the fork-based strategy and rationale
in README/CONTRIBUTING and convert the branch specs to explicit commit
references (dev-feature/ext-smoke#<commit-sha>) so installs are reproducible;
update the composer.json entries for drupal/drupal-driver and
drupal/drupal-extension accordingly and add a short note in the repository docs
explaining the chosen approach.
🪄 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: b3150f4e-8ef8-401d-a91a-f85995dd0401
📒 Files selected for processing (2)
composer.jsonsrc/Drupal/EckTrait.php
| "drupal/drupal-driver": "dev-feature/ext-smoke as 3.0", | ||
| "drupal/drupal-extension": "dev-feature/drupal-driver-3x as 5.4", | ||
| "lullabot/mink-selenium2-driver": "^1.7.4" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for unpinned dev branch requirements in composer.json..."
jq -r '
.require
| to_entries[]
| select(.value | startswith("dev-") and (contains("#") | not))
| "\(.key): \(.value)"
' composer.json
echo
echo "Checking branch/version presence on Packagist (read-only)..."
for pkg in drupal/drupal-driver drupal/drupal-extension; do
echo "== $pkg =="
curl -fsSL "https://repo.packagist.org/p2/${pkg}.json" \
| jq -r --arg pkg "$pkg" '.packages[$pkg][].version' \
| rg '^dev-feature/|^v[0-9]' \
| head -n 25
echo
doneRepository: drevops/behat-steps
Length of output: 803
🏁 Script executed:
cat composer.jsonRepository: drevops/behat-steps
Length of output: 2533
🏁 Script executed:
rg -i "repository|repositories|vcs|github" composer.json -A 3 -B 1Repository: drevops/behat-steps
Length of output: 525
🏁 Script executed:
# Check if branches exist on the upstream Drupal packages
curl -fsSL "https://api.github.com/repos/drupal/drupal-driver/branches" \
| jq -r '.[] | .name' | grep -E "feature/ext-smoke|feature/drupal-driver"
echo "---"
curl -fsSL "https://api.github.com/repos/drupal/drupal-extension/branches" \
| jq -r '.[] | .name' | grep -E "feature/ext-smoke|feature/drupal-driver"Repository: drevops/behat-steps
Length of output: 181
🏁 Script executed:
# Check if composer.lock exists and what it resolved
if [ -f "composer.lock" ]; then
jq '.packages[] | select(.name == "drupal/drupal-driver" or .name == "drupal/drupal-extension")' composer.lock | head -40
else
echo "No composer.lock file found"
fiRepository: drevops/behat-steps
Length of output: 1403
🏁 Script executed:
# Get the full drupal/drupal-extension entry from composer.lock
jq '.packages[] | select(.name == "drupal/drupal-extension")' composer.lock | head -50Repository: drevops/behat-steps
Length of output: 1850
🏁 Script executed:
# Check if composer.lock is tracked in git
git ls-files | grep -E "composer.lock"Repository: drevops/behat-steps
Length of output: 45
🏁 Script executed:
# More explicit check for composer.lock in git
git status composer.lock 2>&1 || echo "File not tracked or not found"Repository: drevops/behat-steps
Length of output: 128
🏁 Script executed:
# Check git history for composer.lock
git log --oneline -- composer.lock 2>&1 | head -5 || echo "No history found"Repository: drevops/behat-steps
Length of output: 45
Pin dev-branch dependencies to stable versions or document fork-based strategy.
Lines 23–24 reference unpinned dev-feature/* branches from forks (jhedstrom/DrupalDriver and jhedstrom/drupalextension). While your composer.lock pins these to concrete commits (ensuring reproducibility for this project), downstream consumers of this library will inherit the unpinned branch constraints. This exposes consumers to non-reproducible installs and potential breakage from force-pushes on development branches.
Consider:
- Pinning to stable versions (e.g.,
drupal/drupal-driver: "^3.0") if available - Or documenting the fork-based development strategy and the rationale for dev-branch dependencies in a README or CONTRIBUTING guide
- Or using inline commit SHAs in
composer.jsonitself (e.g.,dev-feature/ext-smoke#bc835adf...) for clarity
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composer.json` around lines 23 - 25, composer.json currently depends on
unpinned dev branches drupal/drupal-driver (dev-feature/ext-smoke) and
drupal/drupal-extension (dev-feature/drupal-driver-3x) which can break
downstream installs; either replace those entries with stable constraint strings
(e.g., change "drupal/drupal-driver": "dev-feature/ext-smoke as 3.0" to a
released semver like "drupal/drupal-driver": "^3.0" and similarly for
"drupal/drupal-extension"), or if you must keep fork branches, document the
fork-based strategy and rationale in README/CONTRIBUTING and convert the branch
specs to explicit commit references (dev-feature/ext-smoke#<commit-sha>) so
installs are reproducible; update the composer.json entries for
drupal/drupal-driver and drupal/drupal-extension accordingly and add a short
note in the repository docs explaining the chosen approach.
AlexSkrypnyk
force-pushed
the
feature/drupal-driver-3x
branch
from
fb7fc09 to
c88bcca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Drupal/MediaTrait.php`:
- Around line 380-386: The guard and type expectations are inconsistent:
mediaExpandEntityFieldsFixtures() checks for DrupalDriverInterface but
mediaExpandEntityFields() expects the concrete DrupalDriver class, causing
interface-implementing drivers to fail; update mediaExpandEntityFields() to use
the DrupalDriverInterface check instead of DrupalDriver (replace any "$driver
instanceof DrupalDriver" checks and related type hints or docblocks to
DrupalDriverInterface) so both mediaExpandEntityFieldsFixtures() and
mediaExpandEntityFields() consistently accept any implementation of
DrupalDriverInterface.
🪄 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: 939956d6-bb89-4e28-86b2-27d52bce1655
📒 Files selected for processing (3)
composer.jsonsrc/Drupal/EckTrait.phpsrc/Drupal/MediaTrait.php
| if (!$driver instanceof DrupalDriverInterface) { | ||
| // @codeCoverageIgnoreStart | ||
| throw new \RuntimeException(sprintf('The active Drupal driver "%s" does not support content operations required for media field expansion.', $driver::class)); | ||
| // @codeCoverageIgnoreEnd | ||
| } | ||
|
|
||
| $field_types = $driver->getCore()->getEntityFieldTypes('media', array_keys($fields)); | ||
| $field_types = $driver->getCore()->getEntityFieldTypes('media'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'instanceof\s+DrupalDriver\b|instanceof\s+DrupalDriverInterface\b' src/Drupal/MediaTrait.phpRepository: drevops/behat-steps
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check imports
echo "=== Imports (lines 1-20) ==="
head -20 src/Drupal/MediaTrait.php
# Check context around line 349
echo -e "\n=== Context around line 349 ==="
sed -n '340,360p' src/Drupal/MediaTrait.php | cat -n
# Check context around line 380
echo -e "\n=== Context around line 380 ==="
sed -n '370,395p' src/Drupal/MediaTrait.php | cat -n
# Get method names for these lines
echo -e "\n=== Method containing line 349 ==="
sed -n '1,349p' src/Drupal/MediaTrait.php | grep -n 'function\|public\|private' | tail -1
echo -e "\n=== Method containing line 380 ==="
sed -n '1,380p' src/Drupal/MediaTrait.php | grep -n 'function\|public\|private' | tail -1Repository: drevops/behat-steps
Length of output: 2892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check all uses of DrupalDriver in the file (not just instanceof)
echo "=== All uses of DrupalDriver in the file ==="
rg -n 'DrupalDriver' src/Drupal/MediaTrait.php
# Check if these methods call each other
echo -e "\n=== Check method relationships ==="
grep -n 'mediaExpandEntityFields' src/Drupal/MediaTrait.php
# Check the full method definition around line 346 to see method signature
echo -e "\n=== Full mediaExpandEntityFields method ==="
sed -n '346,366p' src/Drupal/MediaTrait.php | cat -n
# Check the full method definition around line 367 to see method signature
echo -e "\n=== Full mediaExpandEntityFieldsFixtures method start ==="
sed -n '367,400p' src/Drupal/MediaTrait.php | head -30 | cat -nRepository: drevops/behat-steps
Length of output: 3083
Align driver type checks across sequential method calls.
mediaExpandEntityFieldsFixtures() (line 380) uses DrupalDriverInterface, but the next method in the sequence, mediaExpandEntityFields() (line 349), requires the concrete DrupalDriver class. A driver implementing the interface will pass the first guard and fail the second, breaking the workflow.
Update both methods to use DrupalDriverInterface for consistent interface-based compatibility.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 382-382: Missing class import via use statement (line '382', column '17'). (undefined)
(MissingImport)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Drupal/MediaTrait.php` around lines 380 - 386, The guard and type
expectations are inconsistent: mediaExpandEntityFieldsFixtures() checks for
DrupalDriverInterface but mediaExpandEntityFields() expects the concrete
DrupalDriver class, causing interface-implementing drivers to fail; update
mediaExpandEntityFields() to use the DrupalDriverInterface check instead of
DrupalDriver (replace any "$driver instanceof DrupalDriver" checks and related
type hints or docblocks to DrupalDriverInterface) so both
mediaExpandEntityFieldsFixtures() and mediaExpandEntityFields() consistently
accept any implementation of DrupalDriverInterface.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/behat/features/drupal_paragraphs.feature`:
- Line 15: The Paragraphs happy-path scenario has been excluded from CI by the
"@api `@skipped`" tag; either remove the "@skipped" tag from that tag line to
re-enable the scenario in CI or replace it with a skip that references a
tracking issue (e.g., change "@skipped" to a named skip like
"@skipped:PAR-1234") so the skip is explicitly tracked; update the tag line
"@api `@skipped`" (or the scenario-level tags) and ensure the Paragraphs
happy-path scenario label remains unchanged.
In `@tests/behat/features/drupal_taxonomy.feature`:
- Line 262: Two vertical-format taxonomy creation scenarios are fully skipped by
the added "@skipped" tags, removing test coverage for single-term and multi-term
vertical-field workflows; remove the "@skipped" tag from those scenario examples
(the ones currently annotated "@api `@skipped`") or replace it with a
remediation-tracking tag (e.g., "@known_issue:<ticket-id>" or
"@todo:<ticket-id>") and add a short comment referencing the ticket so the tests
remain discoverable and the exclusion is traceable; ensure both scenarios
(single-term and multi-term vertical-field) are no longer silently excluded by
updating their tags in the feature file.
In `@tests/behat/features/file_download.feature`:
- Line 28: The new `@skipped` tags on download scenarios (e.g., occurrences of the
tag sequence "@api `@download` `@skipped`") create a gap in FileDownloadTrait
coverage by disabling link-download and ZIP validation paths; remove the broad
"@skipped" tag (or replace it with a narrow, scenario-specific skip marker) from
those download scenarios so the link-download paths and ZIP validation tests
(including negative assertions) are executed again, and verify the scenarios
that validate archive contents and negative download behaviors are restored and
un-skipped.
🪄 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: 65d268eb-7a02-49e2-b74c-1de96dd19629
📒 Files selected for processing (3)
tests/behat/features/drupal_paragraphs.featuretests/behat/features/drupal_taxonomy.featuretests/behat/features/file_download.feature
| """ | ||
|
|
||
| @api | ||
| @api @skipped |
There was a problem hiding this comment.
Avoid disabling both vertical-format taxonomy creation scenarios without remediation tracking.
These @skipped tags exclude both positive creation paths from execution, leaving a coverage gap for single-term and multi-term vertical-field workflows.
Suggested change
- `@api` `@skipped`
+ `@api`
...
- `@api` `@skipped`
+ `@api`Also applies to: 271-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/behat/features/drupal_taxonomy.feature` at line 262, Two
vertical-format taxonomy creation scenarios are fully skipped by the added
"@skipped" tags, removing test coverage for single-term and multi-term
vertical-field workflows; remove the "@skipped" tag from those scenario examples
(the ones currently annotated "@api `@skipped`") or replace it with a
remediation-tracking tag (e.g., "@known_issue:<ticket-id>" or
"@todo:<ticket-id>") and add a short comment referencing the ticket so the tests
remain discoverable and the exclusion is traceable; ensure both scenarios
(single-term and multi-term vertical-field) are no longer silently excluded by
updating their tags in the feature file.
… limitations." This reverts commit 36d9bc4.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #631 +/- ##
==========================================
+ Coverage 97.45% 97.48% +0.03%
==========================================
Files 44 44
Lines 3024 3024
==========================================
+ Hits 2947 2948 +1
+ Misses 77 76 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…3.x driver bootstrap.
…ed by 'DrupalDriver'.
AlexSkrypnyk
changed the title
1 participant
Summary
Migrated
drevops/behat-stepsfromdrupal/drupal-extension5.3 /drupal/drupal-driver2.x to the5.4 / 3.x line. The 3.x driver ships a
DrupalDriverInterface, aContentCapabilityInterfacecapabilitycontract, and a revised
entityCreate()/getEntityFieldTypes()API. This PR updatesEckTraitandMediaTraitto use those new interfaces and methods, replaces the vendored Selenium 2 driver with theLullabot fork required by drupal-extension 5.4, and raises the BehatCliContext subprocess timeout to give
the in-process Drupal bootstrap enough headroom on
@apiscenarios.Changes
composer.jsonbehat/mink-selenium2-driverwithlullabot/mink-selenium2-driver ^1.7.4- drupal-extension 5.4 depends on the Lullabot fork rather than the upstream Behat package.drupal/drupal-drivertodev-feature/ext-smoke as 3.0anddrupal/drupal-extensiontodev-feature/drupal-driver-3x as 5.4while the upstream packages reach a stable release.src/Drupal/EckTrait.phpuse Drupal\Driver\Capability\ContentCapabilityInterfaceimport.$this->getDriver()->createEntity()call (2.x API) with an explicitContentCapabilityInterfaceruntime check followed by$driver->entityCreate()(3.x API).RuntimeExceptionnaming the active driver class when the driver does not implementContentCapabilityInterface, making misconfiguration failures immediately actionable.src/Drupal/MediaTrait.phpDrupalDriverclass to theDrupalDriverInterfaceinterface, keeping the check working across driver implementations.getEntityFieldTypes('media', array_keys($fields))togetEntityFieldTypes('media')- the 3.x driver removed the$base_fieldsparameter from this method.filefield type, covering media bundles that store images via a plainfilefield rather than animagefield.tests/behat/bootstrap/BehatCliContext.phpProcess::setTimeout()from 20 s to 60 s. The 3.x driver bootstraps Drupal in-process before any step runs; on@apisubprocess scenarios that install or uninstall modules this easily exceeds 20 s, causing spurious test timeouts.Before / After
EckTrait - driver call migration
MediaTrait - driver type check widening
Test plan
ahoy lintcleanahoy test-bdd787/787 pass