Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for executeScript/evaluateScript args #826

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mvorisek
Copy link
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

codecov bot commented Apr 14, 2022

Codecov Report

Merging #826 (b37c535) into master (19e5890) will decrease coverage by 0.29%.
The diff coverage is 75.00%.

@@             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     
Impacted Files Coverage Δ
src/Driver/DriverInterface.php 0.00% <0.00%> (ø)
src/Driver/CoreDriver.php 100.00% <100.00%> (ø)
src/Session.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19e5890...b37c535. Read the comment docs.

@aik099
Copy link
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
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?

@stof
Copy link
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
Copy link
Contributor Author

Are there any objections to release a v2?

@stof
Copy link
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