Selenium driver: API to get remote file paths

I really like this idea. Thanks, for creating and working on the issue. Here are my suggestions:

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
   @@ -32,4 +34,73 @@ public function setCookie($name, $value = NULL) {
   +      // @todo Support other drivers when (if) they get remote file transfer
   +      // capability.
   

   Should we add some link here?

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
   @@ -32,4 +34,73 @@ public function setCookie($name, $value = NULL) {
   +    unlink($tempFilename);
   ...
   +      throw $e;
   

   I'd prefer doing it in catch block rather than having if. I don't mind using unlink twice once inside the catch block once outside it.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Tests/DrupalSelenium2DriverTest.php
   @@ -0,0 +1,102 @@
   +    $this->drupalCreateContentType(['type' => 'article', 'name' => 'Article']);
   

   Let's use entity test instead.

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Tests/DrupalSelenium2DriverTest.php
   @@ -0,0 +1,102 @@
   +    $this->xpath('//input[@name="title[0][value]"]')[0]->setValue('Test');
   ...
   +    $multiple_field->setValue(implode("\n", $remote_paths));
   +    $this->getSession()->getPage()->findButton('Save')->click();
   ...
   +    $this->xpath('//input[@name="title[0][value]"]')[0]->setValue('Test');
   ...
   +      $multiple_field->setValue(implode("\n", $real_paths));
   

   If we want to use setValue then why are we using node form? Why not just use Node::create() to create the node?

Thanks, for addressing the feedback.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
   @@ -32,4 +34,66 @@ public function setCookie($name, $value = NULL) {
   +    finally {
   

   I didn't know about. Good to know.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Tests/DrupalSelenium2DriverTest.php
   @@ -0,0 +1,101 @@
   +    $this->drupalGet($entity->toUrl('edit-form'));
   +    $multiple_field = $this->xpath('//input[@multiple]')[0];
   ...
   +    $this->drupalGet($entity->toUrl('edit-form'));
   +    $multiple_field = $this->xpath('//input[@multiple]')[0];
   

   In #3.4, my point was why to use FAPI at all we can test it just with entity API.

Bumping to critical as this is a blocker for the last simpletest to be converted to JTB #2809503: Convert AJAX part of \Drupal\file\Tests\FileFieldWidgetTest to JavascriptTestBase and an helper for #2644468: Multiple image upload breaks image dimensions which is critical on its own.

Needed a reroll and some waitFor stuff to make pass locally, lets see what the bot makes of this.

Reviewed it again didn't find anything objectionable so RTBC.

Can we please mention vaplas's name in the commit message?

+++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
@@ -41,4 +43,66 @@ public function setCookie($name, $value = NULL) {
+  public function getRemoteFilePath($path) {

Can we please move this to a trait so that Drupal testing trait can also use it?

@jibran good idea.

• moved the method to a trait
• renamed the method to uploadFileAndGetRemoteFilePath which might be a little verbose but I think is much clearer in its intent and purpose
• added some docs to explain why this is needed
• removed some vars that were only used once

edit:
Also removed the scenario were the new method is not used to test if it is still needed, since that fails when the local en remote environment are the same (like my local setup), which is really confusing I think.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Tests/DrupalSelenium2DriverTest.php
@@ -27,27 +27,11 @@
-    $this->webDriver = $this->getSession()->getDriver();
-    $this->fileSystem = \Drupal::service('file_system');

@@ -67,14 +49,14 @@
-      $real_paths[] = $this->fileSystem->realpath($file->uri);
+      $real_paths[] = \Drupal::service('file_system')->realpath($file->uri);
...
-      $remote_paths[] = $this->webDriver->getRemoteFilePath($path);
+      $remote_paths[] = $this->getSession()->getDriver()->uploadFileAndGetRemoteFilePath($path);

I think it is better to have local vars instead of calling methods again and again.

+++ b/core/tests/Drupal/Tests/Traits/Core/SeleniumUploadRemoteFileTrait.php
@@ -0,0 +1,78 @@
+      unlink($tempFilename);

should we also do this in the happy-path? i.e when no exceptions occur?

+1 to what jibran said in #15

+++ b/core/tests/Drupal/Tests/Traits/Core/SeleniumUploadRemoteFileTrait.php
@@ -0,0 +1,78 @@
+    finally {
+      unlink($tempFilename);
+    }

This bit is always run, so I think we are good.

Addressed #15

Thanks! do we need a change notice for the new trait?

Adding test for lowest PHP

I've tried to think of a better way to make this change - like maybe overriding \Behat\Mink\Driver\Selenium2Driver::attachFile() but we always hit the fact that \Behat\Mink\Driver\Selenium2Driver::uploadFile is private. So the solution of adding uploadFileAndGetRemoteFilePath() whilst clunky feels pragmatic.

That said I don't think we should add a trait. It adds complex and surface area and the argument that this is needed for https://gitlab.com/weitzman/drupal-test-traits doesn't reflect that they will still need to provided their own implementation of \Behat\Mink\Driver\Selenium2Driver in order to implement the trait. Once they do that they could chose to extend \Drupal\FunctionalJavascriptTests\DrupalSelenium2Driver or just use it if possible. Maybe something in \Drupal\FunctionalJavascriptTests\DrupalSelenium2Driver::setCookie or \Drupal\FunctionalJavascriptTests\DrupalSelenium2Driver::__construct() is doing something they don't want but in this case they can alter those methods.

That said I don't think we should add a trait.

I agree. I don't see any reason for DTT to not use DrupalSelenium2Driver instead of Selenium2Driver in Selenium2DriverTrait. https://gitlab.com/weitzman/drupal-test-traits/issues/42 can address that.

Following patch removes the trait.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
@@ -41,4 +43,69 @@ public function setCookie($name, $value = NULL) {
+    $archive = new \ZipArchive();

Do we need to add ext-zip in composer.json but this is test only change so I'm not sure? This is the only place in core where we are using it. Should we use \Drupal\Core\Archiver\Zip instead?

@alexpott replied in slack to my questions

Should we use \Drupal\Core\Archiver\Zip instead?

This is a c&p from the library

Do we need to add ext-zip in composer.json

http://php.net/manual/en/class.ziparchive.php is part of PHP since 5.2

I just moved the code in #21 so setting it back to RTBC.

Tiny nit

+++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
@@ -41,4 +43,69 @@ public function setCookie($name, $value = NULL) {
+   * @throws \WebDriver\Exception\UnknownError;

; at the end

other then that, RTBC +1

diff --git a/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
index 9fe1476966..55bae90382 100644
--- a/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
+++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
@@ -59,7 +59,7 @@ public function setCookie($name, $value = NULL) {
    *
    * @throws \Behat\Mink\Exception\DriverException
    *   When PHP is compiled without zip support, or the file doesn't exist.
-   * @throws \WebDriver\Exception\UnknownError;
+   * @throws \WebDriver\Exception\UnknownError
    *   When an unknown error occurred during file upload.
    * @throws \Exception
    *   When a known error occurred during file upload.

Fixed on commit.

Committed 2c4ca06 and pushed to 8.7.x.
Committed f481697ca1 and pushed to 8.6.x. Thanks!

FILE: ...tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 98 | ERROR | [x] Calling class constructors must always include
    |       |     parentheses
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 61ms; Memory: 6Mb


FILE: ...al/FunctionalJavascriptTests/Tests/DrupalSelenium2DriverTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
 5 | WARNING | [x] Unused use statement
 7 | WARNING | [x] Unused use statement
 8 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Also fixed this lot.