Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service

#1: Drupal\Core\Site might work.

Other idea:

Drupal\Core\Utility

Or just inject as a \SplString from DrupalKernel like app.root.

Are you running from a sub directory locally? bot does

-1 to put it into utility, given that its kinda a dumping ground for everything.

I think Site is totally fine, as similar to Settings() it depends on the loaded multi site environment.

+++ b/core/lib/Drupal/Core/App.php
@@ -0,0 +1,130 @@
+  public function getBaseUrl() {
...
+  public function getBasePath() {

We should explain exactly here what which bit is.

Don't we just need a version of \Drupal\Core\Routing\RequestContext that uses the application context and not the current request context, and then inject that where it is needed (mainly the link generator)?

Reroll b/c #2521852: Make it possible to use your own exception handler

$ git checkout 3f8677b24589e40793f27ca86e3976137e01d791
$ git apply --index remove-2529170-28.patch
$ git rebase 8.0.x
First, rewinding head to replay your work on top of it...
Applying: remove-2529170-28.patch
Using index info to reconstruct a base tree...
M	core/core.services.yml
M	core/includes/common.inc
M	core/includes/install.core.inc
M	core/includes/install.inc
M	core/includes/theme.inc
M	core/lib/Drupal/Core/DrupalKernel.php
M	core/modules/filter/src/Plugin/Filter/FilterHtml.php
M	core/modules/system/src/Tests/Pager/PagerTest.php
M	core/modules/views/src/Plugin/views/field/FieldPluginBase.php
M	sites/default/default.settings.php
Falling back to patching base and 3-way merge...
Auto-merging sites/default/default.settings.php
Auto-merging core/modules/views/src/Plugin/views/field/FieldPluginBase.php
Auto-merging core/modules/system/src/Tests/Pager/PagerTest.php
Auto-merging core/modules/filter/src/Plugin/Filter/FilterHtml.php
Auto-merging core/lib/Drupal/Core/DrupalKernel.php
CONFLICT (content): Merge conflict in core/lib/Drupal/Core/DrupalKernel.php
Auto-merging core/includes/theme.inc
Auto-merging core/includes/install.inc
Auto-merging core/includes/install.core.inc
Auto-merging core/includes/common.inc
Auto-merging core/core.services.yml
Failed to merge in the changes.
Patch failed at 0001 remove-2529170-28.patch

Just used the hunk from HEAD in the conflict in DrupalKernel::handleException().

Setting status so these can fail properly...

Spent some time with this today. The patch is failing because the installer is really broken. Most of the URLs for JS, CSS, etc, have 'core/core' in the paths. *REDACTED*. May look into this later tonight, but may not have time.

The interactive installer was broken (on my machine at least) because of some screwiness with the script name and symlinks between where Apache had its DOCROOT and where the Drupal installation actually was, which caused App::prepareBasePath() to not work as expected, resulting in general mayhem with paths and requests. Also added a helper to Drupal to get the service so autocomplete in PhpStorm work would, and so I could chase the functions properly... Let's see what this does.

Today I learned that you can't mock methods used in a constructor.

Methinks this will be green. AppTest works locally now.

I also think this could possibly be elevated to a major, because I suspect that it is the best solution for #2548095: Add option to Url() to force the site base_url to be used.

The patch looks great, this also seems to resolve #2481833: Remove LanguageNegotiationUrl's usage of base_path(), so I closed that issue in favour of this one. I think issue credit might need to be copied over from there though.

+++ b/core/lib/Drupal/Core/App.php
@@ -0,0 +1,142 @@
+  protected function prepareBasePath() {
+    $this->basePath = $this->request->getBasePath();
...
+      if ($script_subdir) {
+        // Base path is request basePath - script subdir
+        $this->basePath = substr($this->request->getBasePath(), 0, -strlen($script_subdir));
+      }

Apologies if this has been asked already:

Why is this necessary?

I thought Symfony had similar logic already in the request object.

It seems this could be very disruptive at this stage.

Tried a quick rebase, and there are lots of conflicts here.

Working on this!

-      'request_uri' => $base_root . \Drupal::request()->getRequestUri(),
-      'referer'     => \Drupal::request()->server->get('HTTP_REFERER'),
+      'request_uri' => 'http://example.com/some/location',
+      'referer'     => 'http://example.com/previous/location',

In previous comment, why the above lines has been replaced, I don't understand this, Can someone please help me with this.
As far as I am building new re-rolled patch, I am not including above changed lines.

After lots of issues, rerolled #41 patch.
Let's hope it will pass the QA test.

Since we are already extending the request context, can we just put the logic there?

Because this is not yet fixed, I had to add a hideous work-around to #2830326-5: Broken link to 'Put your site into maintenance mode' on update.php results in WSOD.

rerolled

Reroll.

DrupalKernel still has my favorite: DrupalKernel::guessApplicationRoot(), which should be the job of the app service.

Fixed the PHP error introduced in last patch.

Just reroll

Tagging with Bug Smash Initiative because this blocks a bug

Picking it now.

Updated patch with reroll diff added.

Please fix PHPCS issues

yes working on it.

Fixed PHPCS issues & hiding #78 patch. Added reroll diff against #70.

1. +++ b/core/core.services.yml
   @@ -725,6 +725,9 @@ services:
   +    arguments: ['@app.root']
   

   This should be changed to using the container parameter "app.root". The code should become like: arguments: ['%app.root%'].

2. +++ b/core/includes/theme.inc
   @@ -1421,7 +1421,7 @@ function template_preprocess_page(&$variables) {
   -  $variables['base_path'] = base_path();
   +  $variables['base_path'] = \Drupal::app()->getBasePath() . '/';
   

   We are not deprecation the function base_path() in this issue. Therefor replacing the function is out of scope for this issue and should not be done in this issue. This is done multiple times in this patch, please remove them all unless we meed to change it anyway for this patch. A followup issue for this should be created.

3. +++ b/core/lib/Drupal/Core/App.php
   @@ -0,0 +1,138 @@
   +    if ($request != $this->request) {
   

   I do not think that this check is necessary. The only thing it that $this->baseUrl and $this->basePath are recalculated.

4. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -940,6 +946,12 @@ protected function initializeContainer() {
   +      if ($request = $request_stack->getMasterRequest()) {
   

   The use of getMasterRerquest() is deprecated. Please use getMainRequest().

5. +++ b/core/tests/Drupal/Tests/Core/AppTest.php
   @@ -0,0 +1,171 @@
   +    $app = $this->getMock('Drupal\Core\App', ['realpath'], [$root]);
   ...
   +    $app = $this->getMock('Drupal\Core\App', ['realpath'], [$root]);
   

   Could we change these lines to:

       $app = $this->getMockBuilder('Drupal\Core\App')
         ->setConstructorArgs([$root])
         ->onlyMethods(['realpath'])
         ->getMock();
   

   As this is the way we are doing it in Drupal core. Doing the same way improves readability.

6. +++ b/core/tests/Drupal/Tests/Core/AppTest.php
   @@ -0,0 +1,171 @@
   +    $app->expects($this->any())
   +      ->method('realpath')
   +      ->will($this->returnCallback(function ($path) {
   +        return $path;
   +      }));
   

   This only works because we are testing with paths that do not have /./, /../ and extra / characters. I am not sure we should add testing for that.

7. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1074,46 +1086,6 @@ protected function initializeSettings(Request $request) {
   -    // For a request URI of '/index.php/foo', $_SERVER['SCRIPT_NAME'] is
   -    // '/index.php', whereas $_SERVER['PHP_SELF'] is '/index.php/foo'.
   

   We do not have testing for this in Drupal\Tests\Core\AppTest. Please add testing for this.

8. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1074,46 +1086,6 @@ protected function initializeSettings(Request $request) {
   -    $base_secure_url = str_replace('http://', 'https://', $base_url);
   -    $base_insecure_url = str_replace('https://', 'http://', $base_url);
   

   Contrib or custom code that is relying on these globals to be set will fail when this patch is committed.

9. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -589,8 +589,14 @@ public function preHandle(Request $request) {
   +    $GLOBALS['base_url'] = $app->getBaseUrl();
   +    $GLOBALS['base_path'] = $app->getBasePath() . '/';
   +    $GLOBALS['base_root'] = $request->getSchemeAndHttpHost();
   

   We need to add a comment that this lines are a BC layer that will be removed in Drupal 11 with a link to the be created "change record".

10. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -940,6 +946,12 @@ protected function initializeContainer() {
    +    if (($request_stack = $this->container->get('request_stack', ContainerInterface::NULL_ON_INVALID_REFERENCE))) {
    

    Nitpick: Why double "(("? A single one should be enough.

11. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
    @@ -25,17 +25,24 @@ class RedirectResponseSubscriber implements EventSubscriberInterface {
    -  public function __construct(UnroutedUrlAssemblerInterface $url_assembler, RequestContext $request_context) {
    +  public function __construct(UnroutedUrlAssemblerInterface $url_assembler, App $app) {
    

    We cannot make this change. If contrib or custom code call this class with the second parameter being the RequestContext a deprecation warning should be trigger. With the change as it now is the code will fail. Please remove the parameter typehint and add a if-statement testing for the instance type. If RequestContext the throw a deprecation warning and the parameter to the correct value.

12. +++ b/core/lib/Drupal/Core/Routing/LocalAwareRedirectResponseTrait.php
    @@ -27,32 +27,32 @@ trait LocalAwareRedirectResponseTrait {
    -  public function setRequestContext(RequestContext $request_context) {
    

    This is a public method. We cannot just remove it. It needs to be deprecated and when it is used by contrib or custom it should still work. A deprecation warning should be given.

13. +++ b/core/lib/Drupal/Core/Routing/RequestContext.php
    @@ -28,37 +20,4 @@ public function fromRequestStack(RequestStack $request_stack) {
    -  public function fromRequest(Request $request) {
    ...
    -  public function getCompleteBaseUrl() {
    ...
    -  public function setCompleteBaseUrl($complete_base_url) {
    

    These 3 public method cannot just be remove, they need to be properly deprecated.

14. +++ b/core/modules/language/tests/src/Unit/LanguageNegotiationUrlTest.php
    @@ -80,7 +95,7 @@ public function testPathPrefix($prefix, $prefixes, $expected_langcode) {
    -    $method = new LanguageNegotiationUrl();
    +    $method = new LanguageNegotiationUrl($this->app);
    
    @@ -169,7 +184,7 @@ public function testDomain($http_host, $domains, $expected_langcode) {
    -    $method = new LanguageNegotiationUrl();
    +    $method = new LanguageNegotiationUrl($this->app);
    

    We have added a create method to the class, therefor these changes should not be necessary.

Rerolled against 10.1.x

Testbot is not happy: https://www.drupal.org/pift-ci-job/2504335

MR meeds rebase

Tried rebase via the merge request GUI, it fails. Feb 23 status looks green from the above comment description but I don't see the job, not sure where to find it.

Changed properties and fixed last usage, token changes looks like needs discussion

The testbot is not happy

The issue appears on the Drupal\Tests\Core\Command\QuickStartTest run. It probably blocks the rest of the functional tests.
Any idea what is the cause of that behavior?

I guess it means that functional tests does not get request properly so url is not extracted, it reminds me somehow #2484991: Add the session to the request in KernelTestBase, BrowserTestBase, and drush

#2484991: Add the session to the request in KernelTestBase, BrowserTestBase, and drush looks great, let's wait when it will be fixed.