Deprecate conf_path()

This is install.core.inc only.

Reroll of #5. This still needs much work.

Rerolled with no conflicts.

@dawehner There are still ~25 instances of conf_path() to be converted.

This converts UpdateManagerInstall and UpdateReady. Remaining usages:

$ grep -rl 'conf_path(' core/ 
core//includes/bootstrap.inc
core//lib/Drupal/Core/Database/Driver/sqlite/Install/Tasks.php
core//lib/Drupal/Core/Extension/ExtensionDiscovery.php
core//lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
core//lib/Drupal/Core/Site/Settings.php
core//lib/Drupal/Core/StreamWrapper/PublicStream.php
core//modules/file/tests/file_test/src/StreamWrapper/DummyReadOnlyStreamWrapper.php
core//modules/file/tests/file_test/src/StreamWrapper/DummyStreamWrapper.php
core//modules/locale/locale.install
core//modules/simpletest/src/TestBase.php
core//modules/simpletest/src/Tests/SimpleTestTest.php
core//modules/system/src/Tests/File/DirectoryTest.php
core//modules/system/src/Tests/File/ReadOnlyStreamWrapperTest.php
core//modules/system/src/Tests/Installer/InstallerExistingSettingsTest.php
core//modules/system/src/Tests/System/SettingsRewriteTest.php
core//modules/system/system.install
core//modules/update/update.manager.inc

Converted SiteSettingsForm.

@dawehner: \Drupal\Core\Site\Settings is interesting because of:

    // Make conf_path() available as local variable in settings.php.
    if (is_readable($app_root . '/' . $site_path . '/settings.php')) {
      require $app_root . '/' . $site_path . '/settings.php';
    }

Remaining:

$ grep -rl 'conf_path(' core/ 
core//includes/bootstrap.inc
core//lib/Drupal/Core/Database/Driver/sqlite/Install/Tasks.php
core//lib/Drupal/Core/Extension/ExtensionDiscovery.php
core//lib/Drupal/Core/Site/Settings.php
core//lib/Drupal/Core/StreamWrapper/PublicStream.php
core//modules/file/tests/file_test/src/StreamWrapper/DummyReadOnlyStreamWrapper.php
core//modules/file/tests/file_test/src/StreamWrapper/DummyStreamWrapper.php
core//modules/locale/locale.install
core//modules/simpletest/src/TestBase.php
core//modules/simpletest/src/Tests/SimpleTestTest.php
core//modules/system/src/Tests/File/DirectoryTest.php
core//modules/system/src/Tests/File/ReadOnlyStreamWrapperTest.php
core//modules/system/src/Tests/Installer/InstallerExistingSettingsTest.php
core//modules/system/src/Tests/System/SettingsRewriteTest.php
core//modules/system/system.install
core//modules/update/update.manager.inc

Converted ReadOnlyStreamWrapperTest, SettingsRewriteTest, DirectoryTest, SimpleTestTest

Converted:

• ExtensionDiscovery
• simpletest/src/TestBase.php
• locale.install
• system.install
• update.manager.inc
• InstallerExistingSettingsTest.php

Remaining:

$ grep -rl 'conf_path(' core/ sites/default/default.settings.php 
core//includes/bootstrap.inc
core//lib/Drupal/Core/Database/Driver/sqlite/Install/Tasks.php
core//lib/Drupal/Core/Site/Settings.php
core//lib/Drupal/Core/StreamWrapper/PublicStream.php
core//modules/file/tests/file_test/src/StreamWrapper/DummyReadOnlyStreamWrapper.php
core//modules/file/tests/file_test/src/StreamWrapper/DummyStreamWrapper.php
sites/default/default.settings.php

Reroll and a couple of changes.

Whoops, that was two interdiffs! Here is the real patch for #21.

rerolled #22

whoops.

Problem with the change to PublicStream.
It makes a failure when testbot goes to enable the simpletest module. It's like it's to early in the process, and the kernel service is not available, yet.
and there's still core/lib/Drupal/Core/Database/Driver/sqlite/Install/Tasks.php left to do.

rewound the patch to the state it had in #14, the last time it passed.

changing one at a time, to try to find which one is causing the test fails.

reverted changes to ExtensionDiscovery.php
changed DummyReadOnlyStreamWrapper.php and DummyStreamWrapper.php

oops.

changed InstallerExistingSettingsTest.php

oops

changed TestBase.php

reverted changes to TestBase.php
changed locale.install

changed system.install

changed update.manager.inc

ok, so the changes to ExtensionDiscovery.php, PublicStream.php, and TestBase.php all cause test failures.
I don't think Tasks.php has been attempted.
We'd have to finish off those 4 before getting rid of the function in bootstrap.inc

attempting the Tasks.php change.
I think I'm right in that there's no create method on the Tasks class or it's parent, so we cannot convert it to the DI pattern in this case.
Am I right in that?

To inject, a class needs to be a service, or implement ContainerInjectionInterface, which will allow the create method.

Trying a slightly different patch to TestBase.
It looks from what I can tell that those last 2 calls to conf_path() are trying to reset something, but the conf_path function has been refactored since then, and the 2nd parameter, reset, is not used in it at all.

Looks like this fixes the problem with PublicStream

ExtensionDiscovery

If the previous 2 come back good, then this, the final removal of the function conf_path() should pass, too.

I wonder if the way that it's done in these four, however:

locale.install
InstallerExistingSettingsTest
system.install
update.manager.inc

is optimal? Even if they can't use the DI pattern, I think doing
\Drupal::service('kernel')->getSitePath();
would still be preferable to how they are doing it in the patch right now, messing around with getting the request in the client code and such. Might need to try switching them one at a time and retesting. I wish I knew specifically what test they might fail on, and I would run that one locally. but if I try to run all of them, my machine takes forever. Is it faster though, to run them from drush? I'm just using the admin interface.

Drush or run-tests.sh take about the same amount of time for me.

I'm really stumped how the change in #50 is leading to the errors.
I can reproduce the failing test locally, but why is really eluding me.
In the stacktrace, I can see it failing in TestBase.php, line 977. That is actually calling FileFieldPathTest.php and failing at line 33, cause $node_file is null.
Now as to why, I haven't a clue.

Think I got it figured out.
Half the time it hits PublicStream, the kernel service is not available.

trying the change to ExtensionDiscovery again, now that PublicStream is cleared.

Looks like ExtensionDiscovery is the same way. The kernel service is not always available.

final removal of conf_path

Sadly, I think testbot is faster than my i7. Am I doing something completely wrong? I have run-tests.sh running successfully, but it is so slow. by the time testbot is done, in 20 minutes, my machine isn't out of the A's, yet.
So sending this up instead of trying it locally.

attempting the same sort of cleaner client code in
\Drupal\system\Tests\Installer\InstallerExistingSettingsTest

Looks like
\Drupal\system\Tests\Installer\InstallerExistingSettingsTest.
is lacking the kernel service for 1 frakin test.

Attempting the cleaner client code refactor in
drupal/core/modules/system/system.install

sigh.

This should be the last of these four. Sorry for having to throw them against testbot.

I am painfully aware that trying to move these lase four instances to first try to use the kernel service, may lead to violation of the DRY principle.

I ain't one of them all fancy-schmancy computer scientist types who knows all about software architechtures.
I switched into this about 5 years ago and am fightin up from the trenches.

So, by all means, correct me if there is a better pattern that I should be using. I was thinking maybe a try-catch block, so it could try for the most optimal call first, and only if that fails, use the code that would have gone in the else block.
Which is of course, syntactic sugar for putting the optimal call in the if conditional clause, and the subobtimal call inside the conditioned block. But we are all conditioned that that is a Bad, Bad pattern. No statements inside conditional clauses.
Anyways, I welcome some constructive critisisim, but please be gentle. Thanks.

@sidharrell: Congratulations, you got all of the implementations out! This is a difficult issue for a new contributor and I think you've done an awesome job at it. You have figured out two of the dependency injection patterns in Drupal 8 and that is no small feat in and of itself. I am not an awesome reviewer. Here are just a few tiny things I noticed:

+++ b/core/modules/system/src/Tests/Installer/InstallerExistingSettingsTest.php
@@ -41,14 +43,21 @@ protected function setUp() {
+    ¶

A little whitespace here.

+++ b/core/includes/bootstrap.inc
@@ -137,47 +137,6 @@
-function conf_path($require_settings = TRUE, $reset = FALSE, Request $request = NULL) {

Don't actually remove conf_path() yet, we usually remove deprecated functions in a single issue.

+++ b/core/modules/update/src/Form/UpdateReady.php
@@ -50,11 +58,14 @@ class UpdateReady extends FormBase {
+    $this->kernel = $kernel;

Spacing is wrong here.

@dawehner, your thoughts?

Did those 3.
Sorry, switched editors towards the end from sublime to netbeans, and had not set netbeans up to remove trailing whitespace.

@sidharrell Is it the case that sometimes ExtensionDiscovery is used before the dependency container is up?

@cilefen exactly.
@dawehner I first tried changing it to use the kernel service. That was the test result on #60. I also tried changing it to use the other method all the time, see #32.
I really didn't want to put the if-else in there, believe me. especially having to do it in 3 or 4 places, but I didn't see an alternative.
If we do end up having to do that, would it be better as a try-catch instead? Try the assignment with the kernel service, and if that fails, do the assignment the old fashioned way.

added those comments.

deleted

This is much simpler than before — there are many possible incantations of conf_path(); the non-deprecated approach is much simpler. Great :)

Before I RTBC:

1. +++ b/core/modules/system/src/Tests/File/ReadOnlyStreamWrapperTest.php
   @@ -2,7 +2,7 @@
   + * Contains Drupal\system\Tests\File\ReadOnlyStreamWrapperTest.
   

   Nit: missing leading backslash.

2. IMHO we should remove conf_path() in this issue. I saw this in #73:

   Don't actually remove conf_path() yet, we usually remove deprecated functions in a single issue.

   … but in this case, the function already is deprecated!

the reroll had a conflict in update.inc where the line changed in the patch was in a function deleted in Head.
Did #81.1 and #81.2

Can we remove the conf_path() function in a separate issue - this makes rollback far less painful. Also can we attach https://www.drupal.org/node/2275139 to this issue and the issue to actually remove conf_path(). Thanks.

- * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
- *   Use \Drupal\Core\DrupalKernel::getSitePath() instead. If the kernel is
- *   unavailable or the site path needs to be recalculated then
- *   Drupal\Core\DrupalKernel::findSitePath() can be used.

This information would be useful in the change record.

So you disagree with #81.2? I don't understand why removing an already-deprecated function separately makes rollback easier?

This must be what it's like for my daughter when she hears her mother and I argue. :)
It's ok, I made another issue for removing the deprecated function, and put it back in on this issue.
#2457469: Remove usages of conf_path()
@alexpott, not sure how to attach that node, as it's a CR, and not an issue, the UI won't let me attach it as a related issue. maybe just link to it in the IS?

@sidharrell,

You can reference this issue when you edit the change record (https://www.drupal.org/node/2275139/edit)

@Wim Leers/#85 it makes it easier because if something gets in adding a call to conf_path() then all we have to do is rollback the removal. The rtbc queue at the time this is committed can contain green patches which add calls to conf_path() and we might not have caught the use of a deprecated function.

it makes it easier because if something gets in adding a call to conf_path() then all we have to do is rollback the removal

Ahh! Thanks for the explanation, that makes a ton of sense :)

Updated the CR. Back to RTBC.

Looking at this issue in detail i'm not too thrilled by the ending of lots of dependencies on the kernel - this is not really any different to depending on the container. I think we might be able to add something similar to app.root and app.root.factory to get around this.

Copied-and-pasted to create a @site.path injectable string but it still needs integrating.

I missed one, and removed an unrelated change.

1. +++ b/core/core.services.yml
   @@ -542,6 +542,16 @@ services:
   +    factory_service: 'site.path.factory'
   +    factory_method: 'get'
   +    tags:
   +      - { name: parameter_service }
   +  site.path.factory:
   +    class: Drupal\Core\SitePathFactory
   +    arguments: ['@kernel']
   +    public: false
   
   +++ b/core/lib/Drupal/Core/Site/Settings.php
   @@ -116,7 +116,6 @@ public static function initialize($app_root, $site_path, &$class_loader) {
   diff --git a/core/lib/Drupal/Core/SitePathFactory.php b/core/lib/Drupal/Core/SitePathFactory.php
   
   diff --git a/core/lib/Drupal/Core/SitePathFactory.php b/core/lib/Drupal/Core/SitePathFactory.php
   new file mode 100644
   

   I don't have a lot of experience with this. Does this have to happen or could we just pointed to the kernel as the factory?

2. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -152,8 +154,14 @@ public function scan($type, $include_tests = NULL) {
   +    if (\Drupal::hasService('kernel')) {
   +      $searchdirs[static::ORIGIN_SITE] = \Drupal::service('site.path');
   +    }
   

   This doesn't look right. Also, I wonder if it will be necessary since we're compiling it onto the Container. Turns out this happens several times. We should make sure they're all fixed.

3. +++ b/core/modules/locale/locale.install
   @@ -15,7 +15,8 @@
   -    $directory = conf_path() . '/files/translations';
   +    $conf_path = \Drupal::service('site.path');
   +    $directory = $conf_path . '/files/translations';
   

   Most of the time we're assigning this we're reusing it later but there are a couple cases like this one that seem unnecessary.

4. +++ b/core/modules/simpletest/src/Tests/SimpleTestTest.php
   @@ -161,6 +161,7 @@ function stubTest() {
   +    $site_path = $this->container->get('site.path');
   
   @@ -192,7 +193,7 @@ function stubTest() {
   +    $this->assertTrue(file_exists($site_path . '/settings.testing.php'));
   

   necessary to be spread out and assigned to variable?

This is a reroll of #95. The suggestions in #98 need looking-into.

@neclimdul #98-1 is a response to the suggestion in #90.

Seems like this needs a little refactoring.

Since we only need the request, @app.root, and maybe a sites.php file to determine the site directory, let's move findSitePath() away from the kernel to the factory service. It adds some shim methods to provide an SplString, and a way to get the current request from @request_stack.

This patch passes unit testing but no idea how the testbot will barf. Presented here as an alternate direction without much docblock editing or such.

Please ignore if it's the totally wrong direction.

Based on @alexpott's suggestion in #90, #2384675-90: Deprecate conf_path() and the direction taken in #101, this issue may need a new title.

Updated issue summary, added beta evaluation.

It turns out #101 needed a re-roll anyway, so here it is. It's @cilefen's work and folks can go ahead and ignore #104.

Needed a reroll.

My mistake.

#117 passed on May 2. Retesting now.

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -161,8 +163,14 @@ public function scan($type, $include_tests = NULL) {
   +      $searchdirs[static::ORIGIN_SITE] = \Drupal::service('site.path');
   
   +++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
   @@ -61,8 +63,15 @@ public function getExternalUrl() {
   +    if (\Drupal::hasService('kernel')) {
   +      $site_path = \Drupal::service('site.path');
   

   Can this be injected? Would need to be optional I realise.

2. +++ b/core/modules/simpletest/src/TestBase.php
   @@ -1140,7 +1140,8 @@ private function prepareEnvironment() {
   +    $site_path = \Drupal::service('site.path');
   

   Do we have $this->container here?

Other than that - looks good to me

Will look into #123.1

As far as #123.2: There are two containers, one for the test runner and one for the system under test. I believe that line is discovering the system under test. When you combine that with these two issues, you get a big WTF: #2066993: Use \Drupal consistently in tests #2087751: [policy, no patch] Stop using $this->container in web tests

Added an injected site path for PublicStream::basePath().

Per #126.

#126.1: I presume you mean the 'required' => TRUE part in this:

    $settings['config_directories'][CONFIG_ACTIVE_DIRECTORY] = (object) [
      'value' => \Drupal::service('site.path') . '/files/config_' . $config_directories_hash . '/active',
      'required' => TRUE,
    ];

I have no idea whether it's needed or not. I'm not sure that's in the scope of this issue.

Otherwise, ContainerInterface::get() throws an exception if the service doesn't exist, so we don't need to check for FALSE, if that's what you mean.

#126.2: Okie.

Also there's a real problem here in that the user can change the public:// path in settings before installation, so therefore we shouldn't even use this service in drupal_install_config_directories(). But that should be a follow-up issue.

Woot. Symfony 2.7.

1. I'm +1 on removing conf_path() since it is truly legacy code and it clashes with the notion of configuration and configuration directories in Drupal 8. This is looking like nice work.

2. +++ b/core/includes/install.core.inc
   @@ -1902,8 +1908,9 @@ function install_check_translations($langcode, $server_pattern) {
   +  $conf_path = \Drupal::service('site.path');
   
   @@ -2081,7 +2088,7 @@ function install_check_requirements($install_state) {
   -    $conf_path = './' . conf_path(FALSE);
   +    $conf_path = './' . \Drupal::service('site.path');
   
   +++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
   @@ -28,7 +55,7 @@ public function getFormId() {
   -    $conf_path = './' . conf_path(FALSE);
   +    $conf_path = './' . $this->sitePath;
   
   +++ b/core/modules/locale/locale.install
   @@ -15,7 +15,8 @@
   +    $conf_path = \Drupal::service('site.path');
   +    $directory = $conf_path . '/files/translations';
   
   +++ b/core/modules/system/src/Tests/Installer/InstallerExistingSettingsTest.php
   @@ -48,14 +50,23 @@ protected function setUp() {
   +    if (\Drupal::hasService('kernel')) {
   +      $conf_path = \Drupal::service('site.path');
   +    }
   +    else {
   +      $conf_path = DrupalKernel::findSitePath(Request::createFromGlobals());
   +    }
   ...
   +        'value' => $conf_path . '/files/config_active',
   ...
   +        'value' => $conf_path . '/files/config_staging',
   
   +++ b/core/modules/system/system.install
   @@ -250,7 +252,14 @@ function system_requirements($phase) {
   -    $conf_path = conf_path();
   ...
   +      $conf_path = \Drupal::service('site.path');
   ...
   +      $conf_path = DrupalKernel::findSitePath(Request::createFromGlobals());
   
   @@ -391,7 +400,9 @@ function system_requirements($phase) {
   +      $conf_path = DrupalKernel::findSitePath($request);
   +      $directories[] = $conf_path . '/files';
   
   +++ b/core/modules/update/update.manager.inc
   @@ -306,7 +306,8 @@ function update_manager_local_transfers_allowed() {
   +  $conf_path = \Drupal::service('site.path');
   +  $local_transfers_allowed = fileowner($temporary_file) === fileowner($conf_path);
   

   Let's call the variable $site_path to be consistent.

3. +++ b/core/modules/system/src/Tests/Installer/InstallerExistingSettingsTest.php
   @@ -48,14 +50,23 @@ protected function setUp() {
   +    // Find the site path. Kernel service is not always available at this point,
   +    // but is preferred, when available.
   +    if (\Drupal::hasService('kernel')) {
   +      $conf_path = \Drupal::service('site.path');
   +    }
   +    else {
   +      $conf_path = DrupalKernel::findSitePath(Request::createFromGlobals());
   +    }
   

   Hmmm... this is a test and whether or not it is available should be known. I guess we have this code because it is not available...

#133.2: Good catch.

#133.3: 10 to 1 that check was in there due to copy-paste. The test implodes if we force the use of the service, so I'm pretty sure it needs the kernel's version of the site path.

diff --git a/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
index 30509ce..4d0f9fe 100644
--- a/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
+++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
@@ -55,8 +55,7 @@ public function getFormId() {
    * {@inheritdoc}
    */
   public function buildForm(array $form, FormStateInterface $form_state) {
-    $conf_path = './' . $this->sitePath;
-    $settings_file = $conf_path . '/settings.php';
+    $settings_file = './' . $this->sitePath . '/settings.php';
 
     $form['#title'] = $this->t('Database configuration');
 

Missed one. Fixed on commit.

Also can we get a followup to change the migration setting conf_path to be site_path so that it matches. It's out-of-scope for this issue though.

Plus before we do the removal can we ensure that Drush is not broken and it is fix it first.

Committed fd8c8cd and pushed to 8.0.x. Thanks!

We missed a class #2503015: Remove usages of conf_path() in InstallerExistingSettingsNoProfileTest.

#2503017: Rename the migration setting conf_path to site_path

And this, mentioned in #129. #2503267: drupal_install_config_directories() uses path literals instead of settings to find config directory.