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

php-webdriver 2.0 - RFC 💬 #657

Open
OndraM opened this issue Aug 5, 2019 · 19 comments
Open

php-webdriver 2.0 - RFC 💬 #657

OndraM opened this issue Aug 5, 2019 · 19 comments
Milestone

Comments

@OndraM
Copy link
Collaborator

OndraM commented Aug 5, 2019

This is request for comments about the next major version of php-webdriver (2.0). Once we understand what we want/need to do, we can divide the work among interested volunteers and get it rollin'! 🚀

Suggested objectives

Support only W3C WebDriver

Support only the new protocol and drop support of JsonWire protocol. This will allow us to focus our limited resources to make long-term working implementation of PHP language bindings.

In my opinion, maintaining compatibility with the old protocol is not effective, as the situation has developed rapidly since issue #469 was originally written:

  • W3C WebDriver standard was published on 2018-06-05 - its already a year ago
  • Support of the new protocol amongst browsers is getting better with every version of the respective browsers: see https://webdriver-herald.herokuapp.com/ or https://wpt.fyi/results/webdriver
    • Chrome uses W3C protocol by default since version 75 (released 2019-06-04); even though it could be disabled, it won't work with Selenium 3.9.0 and newer (because of the missing passThrough mode)
    • Chromedriver 76 will include W3C WebDriver Actions API, one of the last missing pieces of full implementation of the standard
    • Firefox (geckodriver) support only W3C protocol from the beginning
    • IEServerDriver was, in fact, the first one with W3C protocol support
  • Selenium 3 is now in maintenance-only mode, development is now focused on Selenium 4, which will even improve W3C protocol support (and may start breaking JsonWire protocol support).
  • The last Firefox working with the JsonWire protocol is Firefox 47 released in June 2016 (over 3 years ago). Even the last Firefox Extended Support (ESR) version which supported JsonWireProtocol is over two years old (45.x) and no longer supported.
  • Most people probably run php-webdriver against Chrome/Chromium, which supports both protocols.

So I don't see an actual need for the JsonWire protocol.

Drop PHP 5.6 and 7.0 support

Those version of PHP are no longer supported. We may also drop PHP 7.1 support, as it is in security-support mode (and only for few next months).

With PHP 7.1+ or 7.2+ we can use many of its new features, most importantly types in method parameters and return values.

If someone cannot use those new versions of PHP, he is probably working in rigid environment and maybe don't use the latest version of browsers - so one can still use the 1.x version of php-webdriver. Or has to upgrade 🤷‍♂️ .

EDIT: this was already done in php-webdriver 1.x.

Clean object dependencies

Historically, this library object model is based on interfaces, which often breaks LSP. It also causes headaches for static analysis. This could be taken care of in the new version.

Other suggestions?

Are there any other issues we should solve?
Any missing features (see Java version changelog)?
Or something which should be done together with BC break?

Please comment!

@andrewnicols
Copy link
Member

andrewnicols commented Aug 5, 2019

Thanks for this @OndraM ,

I agree that we should now drop the JSONWire support. Things have progressed rapidly since the last patch for this.

Personally I'd prefer to see us keep PHP7.1 support for a couple of reasons:

  1. I work for Moodle, and we'll be releasing our next version just before PHP7.1 goes out of support. We will therefore support 7.1 through to 7.3 for the next 2 years in some fashion. Having the same webdriver implementation for all of these releases would be a huge win. Otherwise we won't be able to switch to facebook webdriver for another 6 months. So selfish reason. We do work in a semi-rigid environment but the rigidity in this instance is supported versions of PHP. 7.1 is supported.
  2. PHP 7.1 is still in support. Many projects do still support its use
  3. There are few new features and breaking changes in PHP 7.2 so there is minimal benefit

I'll review the patch at the end of this week.

Thanks

@gabel
Copy link

gabel commented Aug 5, 2019

I guess JSONWire and PHP version support drops are fine.

Getting in touch with some of the bigger dependents might be useful...

@Naktibalda
Copy link

It would be better if PHP 7.1 was supported.

@OndraM
Copy link
Collaborator Author

OndraM commented Aug 19, 2019

Regarding Codeception I pinged @DavertMik.
For Panther we can ask @dunglas.
Unfortunately I don't know anyone from Dusk team 🤔.

@dunglas
Copy link
Contributor

dunglas commented Aug 19, 2019

Those changes are ok for Panther! Even dropping PHP 7.1 support.

One more thing that could be nice: we had to stop shipping Panther with the default Symfony installation because PHP WebDriver depends of ext-curl, and many users haven't ext-curl installed on their systems (symfony/test-pack#9).

Using Symfony HttpClient will allow the library to be used on all installations, even if CURL is missing (in this case Symfony HttpClient automatically switches to an implementation built on top of file_get_contents). As a bonus, HttpClient also provides a nicer, higher level API than ext-curl.

@driesvints
Copy link
Contributor

All good for Laravel Dusk. Not sure if @staudenmeir has any additional concerns?

PHP 7.1 is EOL in three months so it should be dropped. It only encourages people to use EOL versions of PHP when you keep on supporting them. We're dropping support for it in Laravel 6 which is due in a few weeks.

The biggest issue we've had lately with Laravel Dusk is version parity for the chrome driver with the Chrome browser: laravel/dusk#641

@dunglas
Copy link
Contributor

dunglas commented Aug 19, 2019

Actually, Symfony 4 (LTS) will still support PHP 7.1 until 2023 (because RedHat/Ubuntu/Debian will still support it for a few years too). For PHP WebDriver to be shipped with Symfony 4.4 LTS (to be released in November), it has to support PHP 7.1. (Symfony 5, to be released in November too, will require PHP 7.2).

@staudenmeir
Copy link

I don't see any issues for Dusk either.

@stof
Copy link

stof commented Aug 20, 2019

it would be great if some W3C support could be merged in 1.x first (with #560 for instance). This way, supporting W3C drivers (needed for modern browsers) would be independent from migrating to a new major version of the library (which could have BC breaks and require work).

@dunglas
Copy link
Contributor

dunglas commented Aug 20, 2019

I'm working on finishing #560. It's a real blocker for Panther (and most other libs I guess). While there is no ETA for v2, it would be great if we can merge it ASAP.

@DavertMik
Copy link
Contributor

I agree to make 2.0 with the only W3C protocol.

However, bringing such support without feature cut could be a tricky issue. This really depends on how strict implementation of W3C protocol will be. For instance, there is no uploadFile API in W3C protocol, while lots of tests rely on /session/:sessionId/file Selenium API. Other WebDriver APIs are completely different from JSONWire, and this will require some time to users to upgrade.

So, after all, I agree with @stof. To start moving completely to W3C we need to introduce W3C into 1.x branch, receive feedback from users, and then move to 2.0 dropping JSONWire support and cleaning things up with a better understanding of edge cases. Asking users to partially rewrite their code to WebDriver support with no extra benefits but only limitations can be a problem. So, migrating to php-webdrvier v2 should happen only when everyone is assured that this migration will go smoothly.

Also, just to note, we are not time-limited by new Selenium 4, it will continue supporting JSONWire protocol. So let's move slow, but break things :)

@OndraM
Copy link
Collaborator Author

OndraM commented Aug 24, 2019

Ok, thanks everyone for comments and ideas. So the plan I suggest is:

  1. Review and test W3C-compliant implementation, native geckodriver support #560 by @dunglas, and get it merged to 1.x branch soon. However I suggest marking the W3C support here as experimental - if I understand correctly @dunglas's intention, the focus was to keep BC and support current test scenarios with W3C protocol (but not to add new features). Also I may be mistaken, but I guess some part of W3C protocol are not implemented as part of the PR, am I right?
  2. This will be the last feature addition to the 1.x branch, and after this it will be bugfix-only.
  3. Re-focus development on branch for 2.x version - remove OSS protocol support, make it fully W3C compatible, cleanup the codebase and do other stuff discussed above. (Incl. removing support for PHP < 7.1. We probably can live without 7.2+ features.)

What do you think, is this viable for everyone?

@OndraM
Copy link
Collaborator Author

OndraM commented Aug 24, 2019

@dunglas About ext-curl vs. symfony/http-client... This library tries to have the smallest possible dependency tree (because many other libraries are built on top of it and every dependency increases chance of collision in dependent version). So far the only dependency is symfony/process. While I personally like and use symfony/http-client, it should be carefuly considered whether adding this dependency is the best option for this library and what are the tradeoffs.

But I agree we should think about the ext-curl requirement and maybe take care of it somehow - but rather as part of 2.x, ok?

@dunglas
Copy link
Contributor

dunglas commented Oct 3, 2019

Review and test #560 by @dunglas, and get it merged to 1.x branch soon.

Awesome :)

However I suggest marking the W3C support here as experimental -

I agree 💯

if I understand correctly @dunglas's intention, the focus was to keep BC and support current test scenarios with W3C protocol (but not to add new features).

Exactly

Also I may be mistaken, but I guess some part of W3C protocol are not implemented as part of the PR, am I right?

My main goal was to be able to use Panther with Geckodriver (which only supports the W3C flavor). With #560 the whole test suites of both Panther and PHP Webdriver are green with Geckodriver, without Selenoum (I added this case to the Travis matrix). So the implementation maybe don't cover some parts of the W3C protocol, but most of it should be covered.

I agree with the the process you describe. Regarding http-client, it will be marked as stable in Symfony 4.4 (to be released next month). Using it in 2.0 only looks like the best solution, indeed!

@andrewnicols
Copy link
Member

For those using php-webdriver with Mink, I have written a new Mink Driver for this. The SilverStripe driver had a number of bugs which were unaddressed.

https://packagist.org/packages/andrewnicols/mink-facebook-web-driver

I'm currently only focusing on W3C compliance with the aim to drop support for JSONWire at the same time as this facebook\php-webdriver in 2.0. Still a WIP but pull requests always considered.

@OndraM
Copy link
Collaborator Author

OndraM commented Jan 16, 2020

As part of 2.0, we should also change Facebook namespace in all files to avoid interfering with FB trademark (following repository transfer from FB - #730).

This will however mean that everyone using php-webdriver will have to update all file imports...

But I think we can prepare recipe for rector from @TomasVotruba, which will change the namespace (and possibly update also other BC breakes), so that the update to php-webdriver 2.0 could be almost automatic without need of manual work.

@joelharkes

This comment has been minimized.

@OndraM

This comment has been minimized.

@joelharkes

This comment has been minimized.

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

No branches or pull requests

10 participants