Repository navigation
Conversation
Setting any key in `goog:chromeOptions`/`ms:edgeOptions` dropped all driver defaults there, including the click navigation fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
============================================
+ Coverage 88.86% 89.34% +0.47%
- Complexity 197 210 +13
============================================
Files 1 1
Lines 494 516 +22
============================================
+ Hits 439 461 +22
Misses 55 55
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| * | ||
| * @throws \InvalidArgumentException When a list and a map are given for the same capability | ||
| */ | ||
| private function mergeCapabilities(array ...$capabilitySets): array |
There was a problem hiding this comment.
I was under the impression that we want to solve the problem differently (e.g. via an additional hashmap that overwrites everything when set).
I'd rather go for that than the change here because it introduces behaviour that is:
- difficult to comprehend
- unusual/unexpected ("merge" kinda conflicts with php's notion of array merging)
- we're forced to keep and maintain this logic, when released
There was a problem hiding this comment.
What kind of hash map?
Right now driver per-browser default capabilities are always-on stuff that fixes problems that we don't want anyone to undo.
There would be no problem when user defines own --disable... flags because chromedriver does merge them into one comma-separated parameter when starting a browser.
Anything other you had in mind?
P.S.
I've made this PR because @stof said we need deep capability merging.
But I'm open to suggestions.
There was a problem hiding this comment.
What kind of hash map?
I guess I was thinking about the user-defined $desiredCapabilities. The current issue, (if I remember well), is that we allow the user to overwrite individual entries (or add new entries), but not erase them (array_merge() is always additive).
Right now driver per-browser default capabilities are always-on stuff that fixes problems that we don't want anyone to undo.
I got the impression that that is the issue - that we have "definitive settings that we want", but also "sane defaults that most people want, but not everyone wants".
I've made this PR because @stof said we need deep capability merging.
Maybe I'm missing the context, but to me it feels wrong to arbitrarily merge capabilities based on the array shape. Similar to the previous issue (about not being able to remove entries), we might end up with things like someone defining ["windowSize=123x456"] whereas someone else defines ["fullScreen"], ending up with a list containing both (and undefined behaviour).
There was a problem hiding this comment.
Thank you @uuf6429. These examples really helped me understand the flaws of the current deep merge implementation.
I agree that a blind array shape-based merge could silently combine incompatible values (structurally valid, but impossible for the browser to understand).
What we could do is to split the browser default capabilities (WebdriverClassicDriver::DEFAULT_CAPABILITIES keys other than 'default') into 2 groups:
- required (always-on) ones, which are always merged last on top of user-provided ones;
- the rest, which user-provided ones override as a whole (shallow merge, as now).
Only the required ones would be merged into user-provided arrays (e.g. our args added to the user's goog:chromeOptions.args). Since these are the only entries we define ourselves, we don't need to know all browser/driver-specific capability shapes/combinations to prevent incompatible merge results.
This way overriding binary or enabling/disabling a Chrome feature won't make Chrome navigation fail, because --disable-features=DeferRendererTasksAfterInput would still be applied.
And, yeah, this is likely what you've meant by additional hashmap that overwrites everything initially. Only now I got this.
@stof, does this work for you as well?
| && $this->isList($value) !== $this->isList($result[$key]) | ||
| ) { | ||
| throw new \InvalidArgumentException( | ||
| sprintf('Capability "%s" can\'t be merged: a list and a map were given.', $key) |
There was a problem hiding this comment.
this error message will be confusing for nested calls, as it reports the inner key only.
There was a problem hiding this comment.
Fixed: the list/map merge error now reports the full key path, e.g. Capability "goog:chromeOptions" > "args" can't be merged: a list and a map were given.
I didn't use dots as the separator, because Firefox prefs keys contain them (e.g. browser.download.dir).
If we go with the 2 groups approach, this merge would only be used for the required capabilities, but the full key path will stay in the error.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 participants
User capabilities were merged into driver defaults only at the top level, so setting anything in
goog:chromeOptions/ms:edgeOptions(e.g.binaryorargs) silently dropped all driver defaults there, including--disable-features=DeferRendererTasksAfterInputfrom #68.Now:
args,excludeSwitches) are appended toRemoving a driver default (Problem 3 from #54) is still not possible.
Solves Problem 1 of #54 issue.
🤖 Generated with Claude Code