Skip to content

Add support for executeScript/evaluateScript args#826

Draft
mvorisek wants to merge 1 commit intominkphp:masterfrom
mvorisek:allow_execute_args
Draft

Add support for executeScript/evaluateScript args#826
mvorisek wants to merge 1 commit intominkphp:masterfrom
mvorisek:allow_execute_args

Conversation

@mvorisek
Copy link
Copy Markdown
Contributor

The args support is necessary as webdriver objects must be passed natively to the underlaying drivers & cannot be serialized directly into the 1st script argument.

Example executeScript method prototype of one of the most popular driver - https://github.com/php-webdriver/php-webdriver/blob/8ffa927b270e932449e8015abf4d38bb0eff24b7/lib/Remote/RemoteWebDriver.php#L324

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 14, 2022

Codecov Report

❌ Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.17%. Comparing base (19e5890) to head (b37c535).
⚠️ Report is 60 commits behind head on master.

Files with missing lines Patch % Lines
src/Driver/DriverInterface.php 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #826      +/-   ##
============================================
- Coverage     98.47%   98.17%   -0.30%     
  Complexity      345      345              
============================================
  Files            23       24       +1     
  Lines           983      986       +3     
============================================
  Hits            968      968              
- Misses           15       18       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mvorisek mvorisek force-pushed the allow_execute_args branch from f49f3a1 to d4424d1 Compare April 14, 2022 22:21
@aik099
Copy link
Copy Markdown
Member

aik099 commented Apr 15, 2022

Adding more parameters to the DriverInterface interface might be a BC break (not sure if an optional argument causes BC breaks). I guess you'll need to create PR for every driver to pass through these arguments. I guess the Selenium2 is the only one capable of it.

@mvorisek mvorisek marked this pull request as draft April 15, 2022 11:26
@mvorisek
Copy link
Copy Markdown
Contributor Author

Yes, it is fatal error BC break - https://3v4l.org/kO2hW.

I will create PRs to these drivers - https://github.com/minkphp/MinkSelenium2Driver and https://github.com/silverstripe/MinkFacebookWebDriver shortly. What are the other popular drivers?

@mvorisek mvorisek force-pushed the allow_execute_args branch from d4424d1 to c5f9552 Compare April 15, 2022 11:31
@stof
Copy link
Copy Markdown
Member

stof commented Apr 15, 2022

We simply cannot do such BC break in a minor version. That would mean we stop applying semver.

@mvorisek mvorisek force-pushed the allow_execute_args branch from c5f9552 to b37c535 Compare April 15, 2022 11:35
@mvorisek
Copy link
Copy Markdown
Contributor Author

Are there any objections to release a v2?

@stof
Copy link
Copy Markdown
Member

stof commented Jun 9, 2023

even for doing a v2, we would want to provide a migration path

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants