instaclick / php-webdriver

W3C and Selenium 2 webdriver "thin client" for php 5.3+ and namespaces.
Other
435 stars 62 forks source link

Draft: Add verification of response in case of unknown command response #117

Closed vasilake-v closed 2 years ago

vasilake-v commented 2 years ago

Fixes the behavior for version 4.3 of selenium-hub and node-chrome

based on my findings https://www.selenium.dev/documentation/webdriver/elements/interactions/#additional-validations, the version 4 of selenium/webdriver the click() doesn't required a moveto() call to ensure the element is in viewport.

Previous version required to have the element in view port and for that the moveto() was used. But in version 4 the command moveto is missing and click() behaves like 2 in 1

Possible Replacements There is moveToElement. I tested several payloads but i always receive the:

{
    "value": {
        "error": "invalid argument",
        "message": "Unable to determine element locating strategy",
        "stacktrace": ""
    }
}

So this is a dead end for me atm.

Tested with selenium/hub:4.3.0-20220726 selenium/node-chrome:4.3.0-20220726

..also I was considering changes in the \WebDriver\Session, to move the method moveto() from methods() --> obsoleteMethods(). While this seems to be a good approach, this will introduce breaking changes and if we leave the changes only in lib/WebDriver/AbstractWebDriver.php is backwards compatible.

I am open to suggestions

robocoder commented 2 years ago

In the master branch, we could try removing this block entirely since the latest webdriver spec says both 4xx and 5xx responses are JSON:

        // According to https://w3c.github.io/webdriver/webdriver-spec.html all 4xx responses are to be considered
        // an error and return plaintext, while 5xx responses are json encoded
        if ($httpCode >= 400 && $httpCode <= 499) {
            throw WebDriverException::factory(
                WebDriverException::CURL_EXEC,
                'Webdriver http error: ' . $httpCode . ', payload :' . substr($rawResult, 0, 1000)
            );
        }
robocoder commented 2 years ago

Commits:

vasilake-v commented 2 years ago

In the master branch, we could try removing this block entirely since the latest webdriver spec says both 4xx and 5xx responses are JSON:

        // According to https://w3c.github.io/webdriver/webdriver-spec.html all 4xx responses are to be considered
        // an error and return plaintext, while 5xx responses are json encoded
        if ($httpCode >= 400 && $httpCode <= 499) {
            throw WebDriverException::factory(
                WebDriverException::CURL_EXEC,
                'Webdriver http error: ' . $httpCode . ', payload :' . substr($rawResult, 0, 1000)
            );
        }

@robocoder thanks for checking this out !

I will double check this locally and report back

vasilake-v commented 2 years ago

@robocoder I tested it with master version of 1.x and it's working ! Based on the comment above, this adjustment also "landed" in the 1.x version.

And, if i may ask, when a next release is planned for... or when would happen ?

Why I am asking is that the behat/mink-selenium2-driver is relying on current package of instaclick/php-webdriver:1.4.14 which doesn't have the fix yet. And this error is the only thing keeping an update of the selenium-hub & selenium/node-chrome from 3.4 --> 4.3.0

Thanks!

robocoder commented 2 years ago

@slava-v : thanks for the feedback; I've tagged 1.4.15

vasilake-v commented 2 years ago

@slava-v : thanks for the feedback; I've tagged 1.4.15

@robocoder Thank you ! For quick :zap: and prompt support! :+1:

We are now on version 1.4.15 of php-webdriver and selenium/node-chrome 4.3.0 :tada: