Provide forward compatibility layer for BrowserTestBase::xpath

Maybe a stupid idea, but is it an idea to do #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 first and make all the tests HTML5.

+++ b/core/modules/system/src/Tests/System/PageTitleTest.php
@@ -102,15 +102,15 @@ public function testRoutingTitle() {
-    $this->assertEqual('Foo', (string) $result[0]);
...
+    $this->assertEqual('Foo', $result[0]->getText());

Looking at this change, I don't understand what this FC brings. My initial understanding was that $this->xpathUsingMink() will return exactly the same structure as WebTestBase::xpath() did so that we can completely avoid this kind of change.

Nice work, that looks easier than I would have expected!

1. +++ b/core/modules/simpletest/src/AssertContentTrait.php
   @@ -235,6 +237,38 @@ protected function xpath($xpath, array $arguments = []) {
      /**
   +   * Performs an xpath search on the contents of the internal browser.
   +   *
   +   * The search is relative to the root element (HTML tag normally) of the page.
   +   *
   

   We should mention here that this is a forward compatibility helper for the conversion to BrowserTestBase.

   "This method is a forwards compatibility helper for the conversion to \Drupal\Tests\BrowserTestBase".

2. +++ b/core/modules/simpletest/src/GoutteWithManualResponse.php
   @@ -0,0 +1,27 @@
   +  /**
   +   * Creates a crawler and sets it to the object.
   +   *
   +   * This method returns null if the DomCrawler component is not available.
   +   *
   

   hm, the DomCrawler component is always available in our case, so this comment should be removed.

3. +++ b/core/modules/simpletest/src/GoutteWithManualResponse.php
   @@ -0,0 +1,27 @@
   +   * @return \Symfony\Component\DomCrawler\Crawler|null
   +   */
   

   Why does this method return something? We are not even interested in the return value?

4. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -1715,6 +1715,25 @@ protected function xpath($xpath, array $arguments = []) {
   +   * @return \Behat\Mink\Element\NodeElement[]
   +   *   The list of elements matching the xpath expression.
   +   */
   +  protected function xpathUsingMink($xpath, array $arguments = []) {
   

   The method on BrowserTestBase should have an @deprecated tag because people should not use it for new code. They should use the xpath method.

   "@deprecated Scheduled for removal in Drupal 9.0.0. Use self:xpath() instead."

Fixed #9 issues.

Thanks, almost ready! I'm not 100% sold on the method name "xpathUsingMink", but I also don't have a better idea.

+++ b/core/modules/simpletest/src/GoutteWithManualResponse.php
@@ -0,0 +1,23 @@
+
+class GoutteWithManualResponse extends Client {

Class comment is missing. We should mention here that this is used in WebTestBase::xpathUsingMink() for forwards compatibility. We should also mention that we need to have this class because there is no way to set an artificial response on the Goutte client otherwise because createFromCrawler() is protected.

Just a round of nitpicks, otherwise looks good to go to me.

1. +++ b/core/modules/simpletest/src/AssertContentTrait.php
   @@ -235,6 +237,40 @@ protected function xpath($xpath, array $arguments = []) {
   +   * @return \Behat\Mink\Element\NodeElement[]|bool
   +   *   The return value of the xpath search or FALSE on failure. For details on
   +   *   the xpath string format and return values see the domdocument.
   +   *   documentation.
   +   *
   +   * @see http://php.net/manual/class.domdocument.php
   

   The last part of the sentence is broken. Did you mean see the Mink documentation or DOMDocument?

   And the data type should be "...|false" because this never returns TRUE.

2. +++ b/core/modules/simpletest/src/GoutteWithManualResponse.php
   @@ -0,0 +1,33 @@
   +/**
   + * Used by \Drupal\simpletest\AssertContentTrait::xpathUsingMink as a FC layer.
   + *
   

   I think we should avoid uncommon abbreviations such as "FC", let's use forward compatibility layer.

   Can be just "Used as part of the forward compatibility layer for BrowserTestBase."

3. +++ b/core/modules/simpletest/src/GoutteWithManualResponse.php
   @@ -0,0 +1,33 @@
   + * This class allowed you to set the response content manually onto goutte
   + * responses.
   

   Should be "This class allows you to set the response content manually on a Goutte client."

4. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -1719,6 +1719,28 @@ protected function xpath($xpath, array $arguments = []) {
   +   * @deprecated Scheduled for removal in Drupal 9.0.0.
   +   *   Use self:xpath() instead.
   

   Should have 2 "::" after self.

Cool, one minor doc nit pick that can be cleaned up by a novice contributor:

+++ b/core/modules/simpletest/src/AssertContentTrait.php
@@ -208,6 +210,9 @@ protected function buildXPathQuery($xpath, array $args = array()) {
+   * For future compatibility with BrowserTestBase you could use xpathUsingMink
+   * instead as wel..

"For future compatibility with BrowserTestBase you can use the method xpathUsingMink() instead."

Thanks, looks god to me.

1. I think we need a CR

2. +++ b/core/modules/simpletest/src/GoutteWithManualResponse.php
   @@ -0,0 +1,33 @@
   +/**
   + * Used as part of the forward compatibility layer for BrowserTestBase.
   + *
   + * This class allows you to set the response content manually on a Goutte
   + * client.
   + *
   + * @internal
   + *
   + * @see \Drupal\simpletest\AssertContentTrait::xpathUsingMink
   + */
   +class GoutteWithManualResponse extends Client {
   

   I think we should mark this as deprecated too - so we don't forget to remove it in 9.0.x :)

3. +++ b/core/modules/simpletest/src/AssertContentTrait.php
   @@ -208,6 +210,9 @@ protected function buildXPathQuery($xpath, array $args = array()) {
   +   * For future compatibility with BrowserTestBase you can use the method
   +   * xpathUsingMink() instead.
   

   I think we should deprecate the method specifically so people know to use xpathUsingMink(). So their IDEs tell the easy to upgrade. It would also be useful if this documentation gave the developer some idea about what they might need to change given the new return type.

4. +++ b/core/modules/simpletest/src/AssertContentTrait.php
   @@ -235,6 +240,41 @@ protected function xpath($xpath, array $arguments = []) {
   +   *   The return value of the xpath search or FALSE on failure. For details on
   +   *   the xpath string format checkout the DomDocument documentation. For more
   +   *   details on the return values, checkout the mink documentation.
   

   Should checkout have a comma before it yes or no. It looks like this documentation is hedging its bets :).

+++ b/core/modules/simpletest/src/AssertContentTrait.php
@@ -235,6 +240,41 @@ protected function xpath($xpath, array $arguments = []) {
+  protected function xpathUsingMink($xpath, array $arguments = []) {
+    $client = new GoutteWithManualResponse();
+    $client->setCrawlerFromContent($this->getUrl(), $this->getRawContent(), $this->drupalGetHeader('Content-Type'));
+
+    $driver = new GoutteDriver($client);
+    $session = new Session($driver);
+
+    $session->start();
+
+    $xpath = $this->buildXPathQuery($xpath, $arguments);
+    return $session->getPage()->findAll('xpath', $xpath);
+  }
+

Just discovered this issue, and don't want to hijack it, but could this technique be used to update the handful of places where \Drupal\simpletest\AssertContentTrait::setRawContent() is used?

Now that #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits landed I think this forward compatibility is not necessary anymore. The hope was that we could use this to convert even more tests with the big bang patch.

Now this forward compatibility layer would just create more work:
1. Update test case to use forward compatibility layer.
2. Move test case to use BrowserTestBase

While without the forward compatibility layer we just do this in one step. Maybe I'm missing something?

Re-rolled.

Excluded changes to ContactAuthenticatedUserTest.php because they introduced calls to a deprecated function (in favour of the replacement function).

I'm closing this because right now it seems we can work through converting Simpletests without this layer. As I said above it is probably too late for this and just creates more work.