Move drupal_generate_test_ua() into the test system

Added missed replacements in the code and comments.
Added trigger error.

Forgot to kill includes)

Missed dependency, fixed CS.

Looks like this breaks basically every web test and as a result of that testbot isn't even capable of showing the results, try running some of those locally to debug what the problem is.

See https://dispatcher.drupalci.org/job/drupal_patches/88173/artifact/jenkin... for example, somehow the page seems to be basically dead in the test and it is unable to log in.

You can't just visit a path, it simply doesn't work yet as it should. Had a look, the problem is that we need to update drupal_generate_test_ua() at the same time, these two functions are a tandem and they need to be in the same file, as they rely on the filectime() and fileinode() of __FILE__ and currently that doesn't match anymore.

Added replacements for drupal_generate_test_ua()

I've been wondering about a) moving this to a dedicated utility class. and b) expanding "ua" to UserAgent, possibly TestUserAgent::validate()/generate() or so.

Also would be nice if we could switch to a request object instead of using globals, not sure if that's always possible. Hope so :) Maybe it can remain static but we pass in the request object?

But first lets get some real test results, so far results look promising, just a few fails, mostly passes on the browser tests.

Yay just found this issue. :-)

1. +++ b/core/lib/Drupal/Core/CoreServiceProvider.php
   @@ -126,7 +126,7 @@ public function alter(ContainerBuilder $container) {
   +    if (!DrupalKernel::validTestUa()) {
   
   +++ b/core/lib/Drupal/Core/Session/SessionConfiguration.php
   @@ -139,15 +140,15 @@ protected function getCookieDomain(Request $request) {
   +  private function drupalValidTestUa() {
   

   Since, generally, our test layer shouldn't be available at runtime, we should have a version of CoreServiceProvider that is added by the kernel if $env == 'test'. So if you say new DrupalKernel('test') you get a kernel that uses CoreServiceProviderHotRoddedForTests instead of CoreServiceProvider. CoreServiceProviderHotRoddedForTests would then swap out session_configuration service with SessionConfigurationForTesting.

   SessionConfigurationForTesting then knows how to deal with test UA and so forth.

   I'm not sure how far out of scope that would be here, but if there's any time to do it, it's when we're trying to get rid of include files.

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -980,6 +1002,107 @@ protected function initializeContainer() {
   +  public static function validTestUa($new_prefix = NULL) {
   ...
   +  public static function generateTestUa($prefix) {
   

   Again, since these really shouldn't be reachable during non-test runtime, let's not put these on DrupalKernel, because they don't really need what the kernel encapsulates.

   So these should be on a helper class in Drupal\Core\Test, with a @todo to move it to Drupal\Test in #2750461: Remove Simpletest UI because we don't want to maintain a graphical test runner and/or the refactoring mentioned above.

   So +1 on #23.

> I'm not sure how far out of scope that would be here

That's definitely out of scope IMHO, the whole point of these methods is to actually figure out if it is a test run or not, they can't be depending on that.

That, I also proposed in my comment above to move them into a utility and not DrupalKernel, with better names.

Addressed #23 and #25

Let's see can we kill more includes now.

Looks like all update tests are broken?

Let's return all removed includes and see will this help.

No, test report shows failed tests.
https://dispatcher.drupalci.org/job/drupal_patches/90358/testReport/

No idea what is the reason for failed Functional and FunctionalJavascript tests.

Yeah, "PHP 7.1 & MySQL 5.7 Build Successful" actually means *very unsuccessful* unless a project has no tests.

1. +++ b/core/lib/Drupal/Core/Test/UserAgent.php
   @@ -0,0 +1,140 @@
   + *
   + * @todo Move this class in to the \Drupal\Test namespace. See https://www.drupal.org/project/drupal/issues/2750461.
   + *
   

   I'm not convinced this makes sense. This is something that we need to run at regular runtime as I wrote above, it is *not* a test-only thing, we don't want to register \Drupal\Tests at runtime.

2. +++ b/core/lib/Drupal/Core/Test/UserAgent.php
   @@ -0,0 +1,140 @@
   +      $key_file = \Drupal::root() . '/' . $test_db->getTestSitePath() . '/.htkey';
   +      if (!is_readable($key_file)) {
   +        header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
   +        exit;
   +      }
   +      $private_key = file_get_contents($key_file);
   +      // The file properties add more entropy not easily accessible to others.
   +      $key = $private_key . filectime(__FILE__) . fileinode(__FILE__);
   +      $time_diff = \Drupal::time()->getRequestTime() - $time;
   +      $test_hmac = Crypt::hmacBase64($check_string, $key);
   

   This stuff is called before we have a container, you can't use \Drupal:: here.

   I went with $_SERVER['REQUEST_TIME'] for now, that then passes functional tests again.

   Something we could try is require that a request object is passed to this and then read all the server/cookie stuff from that, that should always exist. But lets get it green first with minimal changes.

   Quickstart is only half-passing with this, somehow error handling/display is still broken.

missed replacement,
reverted include file removal from update.php
tried to include bootstrap inc in the UserAgent methods

Reroll for core/tests/Drupal/Tests/Core/DrupalKernel/DrupalKernelTest.php

Looks everything replaced correctly. Let's try to decrease includes.

Looks like other bootstrap includes can't be replaced now, because of used DRUPAL_ROOT and REQUEST_TIME constants.
This one should be ready for review.

Nope, looks like #40 should be the patch for review.

#40 looks nice to me, I think what we need is a @group legacy test that tests the deprecation and that the old functions still work?

Filled CR https://www.drupal.org/node/3044173
Added initial legacy test

Maybe defining DRUPAL_TEST_IN_CHILD_SITE will help with this test?

+++ b/core/tests/Drupal/KernelTests/Core/Test/UserAgentLegacyTest.php
@@ -0,0 +1,35 @@
+class UserAgentLegacyTest extends KernelTestBase {
...
+    include_once DRUPAL_ROOT . "/core/includes/bootstrap.inc";

If you revert everything from this patch except this test, and then run it without including bootstrap.inc, it tells you that DRUPAL_TEST_IN_CHILD_SITE is undefined.

That's because kernel tests don't make requests, so that whole infrastructure shouldn't be expected to exist. There's no such thing as a kernel test that calls drupal_generate_test_ua().

Turn this test into a functional test, since that's what' we're testing.

The fact that this is ambiguous and brittle? ::points at #25.1:: There's a reason the symfony kernel has an $env variable on construction. I started on a patch to do this, but so many other things.

Converted test. Thanks @Mile23 for the explanation.

1. +++ b/core/lib/Drupal/Core/Session/SessionConfiguration.php
   @@ -139,15 +140,15 @@ protected function getCookieDomain(Request $request) {
    
      /**
   -   * Wraps drupal_valid_test_ua().
   +   * Wraps \Drupal\Core\Test\UserAgent::validate().
       *
       * @return string|false
   

   That kind of wrapping is almost always done for unit tests. I guess that's still necessary because it is a static call that does all kinds of things that would likely not work in a unit test.

   However, changing from protected to private is kind of a break as well. *If* we do that, we could possibly also rename the method to reflect the new name. If we don't do that, we also shouldn't change it to private.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Session/SessionTest.php
   @@ -39,9 +39,11 @@ protected function setUp() {
       *
   -   * Makes sure that drupal_valid_test_ua() works for multiple requests
   -   * performed by the Mink browser. The SIMPLETEST_USER_AGENT cookie must always
   -   * be valid.
   +   * Makes sure that UserAgent::validate() works for multiple
   +   * requests performed by the Mink browser. The SIMPLETEST_USER_AGENT cookie
   +   * must always be valid.
   +   *
   +   * @see \Drupal\Core\Test\UserAgent::validate()
       */
   

   Either this should use the full name, then we don't need the @see or we can revert the rewrapping because UserAgent::validate() is actually shorter than the function name and we can keep the text as it was before.

3. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
   @@ -176,8 +176,9 @@ protected function setUp() {
   -      // from the site directory. To allow drupal_generate_test_ua() to write
   -      // a file containing the private key for drupal_valid_test_ua(), the site
   +      // from the site directory. To allow
   +      // \Drupal\Core\Test\UserAgent::generate() to write a file containing
   +      // the private key for \Drupal\Core\Test\UserAgent::validate(), the site
          // directory has to be writable.
          // BrowserTestBase::tearDown() will delete the entire test site directory.
   

   this is correct as it uses the full namespace, I think prefer that above too and then drop the @see.

4. +++ b/core/tests/Drupal/Tests/UiHelperTrait.php
   @@ -424,13 +425,13 @@ protected function getAbsoluteUrl($path) {
       * Prepare for a request to testing site.
       *
       * The testing site is protected via a SIMPLETEST_USER_AGENT cookie that is
   -   * checked by drupal_valid_test_ua().
   +   * checked by DrupalKernel::validTestUa().
       *
   -   * @see drupal_valid_test_ua()
   +   * @see \Drupal\Core\Test\UserAgent::validate()
   

   this reference wasn't updated yet for the new class name, I'd also suggest to use the namespace and drop the @see.

Rerolled.
Addressed #52
FIxed CS issues of deprecation messages.

1. +++ b/core/lib/Drupal/Core/Test/UserAgent.php
   @@ -0,0 +1,141 @@
   +
   +/**
   + * Class UserAgent for testing environments.
   + *
   + * @todo Move this class in to the \Drupal\Test namespace. See
   + *   https://www.drupal.org/project/drupal/issues/2750461.
   + *
   

   We usually don't do Class X, that's literally repeating what it says below. We should just describe what it does: "Generates and validates test user agents"?

   Also, as mentioned in #35, I still think this @todo makes no sense. This has nothing to do with the graphical UI runner, this is about identifying if it is a test request, it's not possible to only load this when running a test as we need it to decide that.

2. +++ b/core/lib/Drupal/Core/Test/UserAgent.php
   @@ -0,0 +1,141 @@
   +
   +    // A valid Simpletest request will contain a hashed and salted
   +    // authentication code. Check if this code is present in a cookie or custom
   +    // user agent string.
   +    $http_user_agent = isset($_SERVER['HTTP_USER_AGENT']) ? $_SERVER['HTTP_USER_AGENT'] : NULL;
   

   Maybe this is where the @todo above is from? It's not limited to "Simpletest", we should replace this with "test request" or "browser test request" or something like that.

   And also as mentioned in #35, I would suggest we pass in the Request object here, so we can replace all those super-globals with $request.

3. +++ b/core/lib/Drupal/Core/Test/UserAgent.php
   @@ -0,0 +1,141 @@
   +      if (!is_readable($key_file)) {
   +        header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden');
   +        exit;
   +      }
   

   also interesting. Are we far enough in the bootstrap process to do a throw an access denied exception?

4. +++ b/core/lib/Drupal/Core/Test/UserAgent.php
   @@ -0,0 +1,141 @@
   +      if ($time_diff >= 0 && $time_diff <= 600 && Crypt::hashEquals($test_hmac, $hmac)) {
   +        static::$testPrefix = $prefix;
   +      }
   

   This will conflict with #3053956: Deprecate \Drupal\Component\Utility\Crypt::hashEquals() in favour of PHP's builtin hash_equals(), we'll need to update it to use hash_equals(), if it happens to be updated before that gets committed, we could already update now, will be easier to reroll then.

5. +++ b/core/lib/Drupal/Core/Test/UserAgent.php
   @@ -0,0 +1,141 @@
   +      else {
   +        header($_SERVER['SERVER_PROTOCOL'] . ' 403 Forbidden (SIMPLETEST_USER_AGENT invalid)');
   +        exit;
   +      }
   

   same here.

6. +++ b/core/lib/Drupal/Core/Test/UserAgent.php
   @@ -0,0 +1,141 @@
   +    return 'simple' . $check_string . ':' . Crypt::hmacBase64($check_string, static::$testKey);
   

   I guess simple is also a reference to simpletest, but not sure if there are any implications to changing how the hash is built exactly.

Adding new patch fixing 1 to 5.

I am not sure how, but in 6, adding 'simpletest' is necessary. It seems related to the table prefix created by simpletest, maybe someone can clarify?

Reroll.

Re: #54.1:

+ * Class UserAgent for testing environments.
+ *
+ * @todo Move this class in to the \Drupal\Test namespace. See
+ *   https://www.drupal.org/project/drupal/issues/2750461.
+ *

I'm pretty sure this is about not wanting to be able to autoload this class if there's no Simpletest UI. The linked issue is #2750461: Remove Simpletest UI because we don't want to maintain a graphical test runner

It'd be best if UserAgent weren't autoloadable out of a testing context. But since we need it for simpletest UI right now, and since it's a non-trivial refactor, it would have to happen when/if we remove that testing form. For instance, see this from install_begin_request() that shows us why we shouldn't be able to autoload it, and also why it's not easy to change:

  // If the hash salt leaks, it becomes possible to forge a valid testing user
  // agent, install a new copy of Drupal, and take over the original site.
  // The user agent header is used to pass a database prefix in the request when
  // running tests. However, for security reasons, it is imperative that no
  // installation be permitted using such a prefix.
  $user_agent = $request->cookies->get('SIMPLETEST_USER_AGENT') ?: $request->server->get('HTTP_USER_AGENT');
  if ($install_state['interactive'] && strpos($user_agent, 'simpletest') !== FALSE && !drupal_valid_test_ua()) {
    header($request->server->get('SERVER_PROTOCOL') . ' 403 Forbidden');
    exit;
  }

#54.6: If we want to have an initiative to remove the word 'simpletest' from core, then let's get on that. :-)

My main review:

+++ b/core/lib/Drupal/Core/Test/UserAgent.php
@@ -0,0 +1,147 @@
+  public static function validate($new_prefix = NULL) {
...
+    try {
+      $request = \Drupal::request();
+    }
+    catch (ContainerNotInitializedException $e) {
+      $request = Request::createFromGlobals();
+    }

So a couple things here...

1) We don't have any isolated tests of the UserAgent class. We just need a basic sanity check type test. It will have to be a kernel test so we have some of the dependencies such as a database.

2) If we update the signature of validate() to be validate($new_prefix = NULL, Symfony\Component\HttpFoundation\Request $request = NULL) then we can test the validation.

> I'm pretty sure this is about not wanting to be able to autoload this class if there's no Simpletest UI. The linked issue is #2750461: Remove Simpletest UI because we don't want to maintain a graphical test runner

Yes, but that's my point, this code has nothing to do with the simpletest UI? It's to detect and validate that a request belongs to a certain test environment. It has to be loaded and used *before* we know that we are executing a test, which means it is impossible to have it in a test-only namespace or anything like that. This @todo is bogus, which is why I suggested to remove it :)

> #54.6: If we want to have an initiative to remove the word 'simpletest' from core, then let's get on that. :-)

The above is why I suggested to change this, having simpletest in there leads to those wrong conclusions that it as anything to do with simpletest.module, which is no longer true, not for phpunit tests. But I'm happy to leave that to another issue.

+1 on at least optionally allowing to pass in $request and I think we should make it required. The main check for this is very much in the critical path and is before we have a container *but* we have a request object.

Yes, but that's my point, this code has nothing to do with the simpletest UI?

Correct. It doesn't have anything directly to do with simpletest UI, but the UI allows you to run tests outside of a test-only context.

That's why drupal_valid_test_ua() is in bootstrap.inc, and we keep calling it in order to check whether or not we're in a test context. Core has a lot of places where it tries to figure out whether it's running under test or not, in order to work around simpletest.

We shouldn't be doing that, though. We should have an assumption that we're not running under tests. Over time we can remove complexity from core and from the testing system by decoupling them.

That's why we should eventually move UserAgent to Drupal\Tests\ so that it can't be autoloaded unless we're running tests. There's a test isolation reason for this, and there's also a security reason for this, see #58.

That's the explanation of my understanding of that @todo. It might make more sense to link to #2866082: [Plan] Roadmap for Simpletest instead. Or it might make more sense to just put UserAgent under Drupal\Tests\ now, if it's not too disruptive to the patch as it is.

But then how do you propose that we'd handle this instead?

We could change how in-test-requests are identified, but we will always need something like this to allow running tests without special hardcoded environments, being able to run tests in parallel and so on. It has nothing to do with simpletest, UI test runner or not. Not anymore.

I still believe that even if we would change this in the future, it has nothing to do with the removal of simpletest.module and there is no need for a @todo pointing to such an issue.

we will always need something like this to allow running tests without special hardcoded environments

The thing we're discussing *IS* the special hardcoded environment, and I'm proposing that we allow running tests without it. :-)

But, whatever. Just leave the todo out and then we'll eventually move it anyway.

So that leaves #58.1 and #58.2, and @Berdir at least agrees with 2.

Addressed 58.2

The first iteration of moving to Drupal\Tests namespace

Get rid of \Drupal\Core\Test\UserAgent in favor of \Drupal\Tests\UserAgent

Addressed #58.1 and #62

Addressed #58.1 over the patch from #64

Fix constant definition check.

Removed ::generate method coverage. Not sure how to test it properly.

A bit confused about the last few patches, is this still work in progress?

As I wrote in the last chapter in #59, I think the request argument should be required and the caller should be responsible to provide it.

I tried to move the UserAgent class into Drupal\Tests. Looks like there needs more work to do it. So the last patch reverts that movement. Let's left this for follow-up issue.

Here the patch addressing #73

1. +++ b/core/includes/bootstrap.inc
   @@ -645,23 +649,48 @@
     */
   -function drupal_valid_test_ua($new_prefix = NULL) {
   -  @trigger_error('drupal_valid_test_ua() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Test\UserAgent::validate(). See https://www.drupal.org/node/3044173', E_USER_DEPRECATED);
   -  return UserAgent::validate($new_prefix);
   +function drupal_valid_test_ua($new_prefix = NULL, Request $request = NULL) {
   

   no need to change the areguments to the old function, just always use the fallback there.

Will need to review the other parts when I can look through the code, that looks much more complicated than what I was hoping for :)

Addressed #76
Also rerolled after recent changes related to Drupal Scaffold.

Change status

Here's few fixes
- old implementation always using globals, so request should be taken not from \Drupal::request()
- fixed REQUEST_TIME should be taken from request too
- few docs improvements

Failed file upload tests indicates that more work needed to prevent using files when request is created from globals

1) Drupal\Tests\file\Functional\MultipleFileUploadTest::testMultipleFileFieldWithAllFileExtensions
Failed asserting that 'The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Symfony\Component\HttpFoundation\File\Exception\FileNotFoundException</em>: The file &quot;/tmp/phpDoBlKa&quot; does not exist in <em class="placeholder">Symfony\Component\HttpFoundation\File\File-&gt;__construct()</em> (line <em class="placeholder">37</em> of <em class="placeholder">vendor/symfony/http-foundation/File/File.php</em>). <pre class="backtrace">Symfony\Component\HttpFoundation\File\UploadedFile-&gt;__construct(&#039;/tmp/phpDoBlKa&#039;, &#039;file1.wtf&#039;, &#039;&#039;, 32, 0) (Line: 87)\n
Symfony\Component\HttpFoundation\FileBag-&gt;convertFileInformation(Array)\n
array_map(Array, Array) (Line: 90)\n
Symfony\Component\HttpFoundation\FileBag-&gt;convertFileInformation(Array)\n
array_map(Array, Array) (Line: 90)\n
Symfony\Component\HttpFoundation\FileBag-&gt;convertFileInformation(Array) (Line: 52)\n
Symfony\Component\HttpFoundation\FileBag-&gt;set(&#039;files&#039;, Array) (Line: 61)\n
Symfony\Component\HttpFoundation\FileBag-&gt;add(Array) (Line: 40)\n
Symfony\Component\HttpFoundation\FileBag-&gt;replace(Array) (Line: 31)\n
Symfony\Component\HttpFoundation\FileBag-&gt;__construct(Array) (Line: 277)\n
Symfony\Component\HttpFoundation\Request-&gt;initialize(Array, Array, Array, Array, Array, Array, NULL) (Line: 255)\n
Symfony\Component\HttpFoundation\Request-&gt;__construct(Array, Array, Array, Array, Array, Array, NULL) (Line: 2089)\n
Symfony\Component\HttpFoundation\Request::createRequestFromFactory(Array, Array, Array, Array, Array, Array) (Line: 314)\n
Symfony\Component\HttpFoundation\Request::createFromGlobals() (Line: 199)\n
Drupal\Core\Extension\ExtensionDiscovery-&gt;scan(&#039;profile&#039;) (Line: 104)\n
Drupal\Core\Config\ExtensionInstallStorage-&gt;getAllFolders() (Line: 129)\n
Drupal\Core\Config\InstallStorage-&gt;listAll() (Line: 38)\n
Drupal\Core\Config\Schema\ConfigSchemaDiscovery-&gt;getDefinitions() (Line: 284)\n
Drupal\Core\Plugin\DefaultPluginManager-&gt;findDefinitions() (Line: 175)\n
Drupal\Core\Plugin\DefaultPluginManager-&gt;getDefinitions() (Line: 160)\n
Drupal\Core\Config\TypedConfigManager-&gt;getDefinitionWithReplacements(&#039;test_theme_settings.settings&#039;, Array, 1) (Line: 200)\n
Drupal\Core\Config\TypedConfigManager-&gt;getDefinition(&#039;test_theme_settings.settings&#039;) (Line: 361)\n
Drupal\Core\Config\TypedConfigManager-&gt;hasConfigSchema(&#039;test_theme_settings.settings&#039;) (Line: 208)\n
Drupal\Core\Config\Config-&gt;save() (Line: 504)\n
Drupal\system\Form\ThemeSettingsForm-&gt;submitForm(Array, Object)\n
call_user_func_array(Array, Array) (Line: 111)\n
Drupal\Core\Form\FormSubmitter-&gt;executeSubmitHandlers(Array, Object) (Line: 51)\n

Reroll (simpletest.install file was updated)
Missed argument pass in one of the ::validate method call.
Also minor changes of the call replacements.

Looks all remaining failures are consequence of file upload handling by created request object

Files are processed from globals twice that's why second created request object throws exception - can't move already moved file

To solve it clearly the request object should be by-passed (not created each time) so it makes me think about other way to detect test ID

I'm not sure what you mean with different way? The test ID *is* in the request data. The only decision we can make is whether or not we use the request object. The problem with not adding the argument now is that we can then never refactor it later on.

To make this work, we need to avoid Request::createFromGlobals() at least during regular requests, not just to make it work but also for performance reasons. The main one is \Drupal\Core\DrupalKernel::bootEnvironment(), and for the calls to that that matter, the request is available in the caller, so I started to add an argument there, we'll have to figure out BC for that.

I've also added some checks to ExtensionDiscovery so that we can use the request when available. Got one test working with that, lets see about the others.

Re-roll still getting issue with

1) Drupal\Tests\update\Functional\UpdateUploadTest::testUploadModule
Behat\Mink\Exception\ExpectationException: Current page is "/admin/modules/install", but "/core/authorize.php" expected.

Rerolled against 9.0.x

Also had fixed the last issue with the patch. So the patch is finally ready for the review

Added missed $defaultTheme in the legacy test.

Just reroll against 9.1.x
Ready for the review.

Forgot to add update.php into the diff result.

#98 looks like a crosspost
#97 is the patch for the review. I can't find the patch in Files below, so just hiding unrelated files from IS.

Reupload #97 patch. Unable set testing for the #97 patch.

Unrelated test fail

Patch looks good. Some remarks:

1. When I do a code base search for "drupal_valid_test_ua", I still find them in the wrong places, like: "core/tests/Drupal/Tests/Core/Database/DatabaseTest.php"

2. +++ b/core/lib/Drupal/Core/Test/UserAgent.php
   @@ -0,0 +1,143 @@
   +      list(, $prefix, $time, $salt, $hmac) = $matches;
   

   This line can be changed to: [, $prefix, $time, $salt, $hmac] = $matches;

3. +++ b/core/lib/Drupal/Core/Test/UserAgent.php
   @@ -0,0 +1,143 @@
   + * @package Drupal\Core\Test
   

   What does this line? And can it be removed?

ignore this patch please

1. I see that drupal_valid_test_ua is part of a comment in core/tests/Drupal/Tests/Core/Database/DatabaseTest.php and this can be removed if not needed here.
2. Adding a patch for changes requested
3. Not sure why its there. Can some one suggest if this can be removed?

The points from comment #104.1 and #104.3 are still open.

Fixed #104.1 and #104.3 .

Oops, patch missed adding UserAgent.php file.
Fixed #104.1 and #104.3 .

Need to fix the last test fail. core/tests/Drupal/Tests/Core/Database/DatabaseTest.php setUp() method mocks container methods. So the argument 'request_stack' in 'has' method not expected.

Working on the failed test.

Removed mock container creation as this is now not needed.

The last test fail fixed. +1 for rtbc

Patch looks good, needs some minor improvements:

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -973,7 +974,7 @@ protected function initializeContainer() {
   -  public static function bootEnvironment($app_root = NULL) {
   +  public static function bootEnvironment($app_root = NULL, Request $request = NULL) {
   

   Missing the added parameter in the docblock.

2. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -195,7 +196,7 @@ public function scan($type, $include_tests = NULL) {
   +      $include_tests = Settings::get('extension_discovery_scan_tests') || UserAgent::validate(\Drupal::hasService('request_stack') && \Drupal::request() ? \Drupal::request() : Request::createFromGlobals());
   

   Can we put the whole request stuff on a separate line. For an improvement in readability.

3. +++ b/core/lib/Drupal/Core/Test/UserAgent.php
   @@ -0,0 +1,141 @@
   + * @see https://www.drupal.org/project/drupal/issues/2750461
   

   Why add link to an unrelated issue?

4. +++ b/core/tests/Drupal/FunctionalTests/UserAgentLegacyTest.php
   @@ -0,0 +1,32 @@
   + * Test legacy UA functions.
   

   Can we change "UA" to "user agent".

5. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -263,11 +264,12 @@ protected function bootEnvironment() {
   +    // \Drupal\Core\Test\UserAgent::validate() can still be
        // safely executed. This primarily affects the (test) site directory
   

   Outlining to the 80 character maximum can be improved.

6. When I do a code base search for "drupal_valid_test_ua(", I got one in "core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php"

@daffie Addressed the #118
Please review

Sorry @Deepak Goyal, I did not check that is assigned to you.

I have also created patch for the same please review.

Review of the patch from comment #120.

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -195,7 +196,9 @@ public function scan($type, $include_tests = NULL) {
   -      $include_tests = Settings::get('extension_discovery_scan_tests') || drupal_valid_test_ua();
   +      $include_tests = Settings::get('extension_discovery_scan_tests') ||
   +        UserAgent::validate(\Drupal::hasService('request_stack')
   +        && \Drupal::request() ? \Drupal::request() : Request::createFromGlobals());
   

   The change that I wanted was:

         $request = \Drupal::hasService('request_stack') && \Drupal::request() ? \Drupal::request() : Request::createFromGlobals();
         $include_tests = Settings::get('extension_discovery_scan_tests') || UserAgent::validate($request);
   

2. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -259,15 +260,17 @@ protected function bootEnvironment() {
   -    Database::addConnectionInfo('default', 'test-runner', $this->getDatabaseConnectionInfo()['default']);
   +    Database::addConnectionInfo('default', 'test-runner',
   +     $this->getDatabaseConnectionInfo()['default']);
   

   This change is not necessary. Out of scope for this issue.

3. +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
   @@ -27,7 +27,7 @@ class UrlConversionTest extends UnitTestCase {
   +    // Mock the container so we don't need to mock \Drupal\Core\Test\UserAgent::validate().
   

   The comment goes over the 80 character limit.

4. Point #118.5 is still open.

Hi @daffie
Updated patch please review.

Solving test case , kindly review a new patch.

@Hardik_Patel_12: thanks for the patches here and elsewhere, but your interdiffs seem to be diffs-of-diffs which are quite hard to read - please see https://www.drupal.org/documentation/git/interdiff for a number of ways of creating a more useful interdiff which lets reviewers see the changes between patches more easily.

Fixed the failing test in #126.

Only #124.3 is still open.

Sorry, wrong status. Needs to be "needs work", because #124.3 is still open.

I am working on this as per #133 comment

Hi @daffie,
I have updated the patch and fix #124.3 issue. Please take a look.

@Raunak.singh: I think your patch is missing some parts.

Yes @daffie
I am Working on it.

Due to internet issue patch file not added

Resolve previous patch issue please take a look.

+++ b/core/tests/Drupal/FunctionalTests/UserAgentLegacyTest.php
@@ -0,0 +1,32 @@
+  protected $defaultTheme = 'classy';

classy is not encouraged to use. even this is a test testing legacy code, but newly added. Changed to stark.

All my remarks are addressed.
All code changes look good to me.
All usages of drupal_valid_test_ua() and drupal_generate_test_ua() have been replaced.
Both functions are deprecated and have deprecation message testing.
Updated the CR and IS.
For me it is RTBC.