Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
conf_path()
is marked to be deprecated for 8.0.0 release, meaning it should be replaced in some way and then removed from Drupal.
Proposed resolution
- Add a
site.path
service parameter. - Add a
site.path.factory
service which generates the parameter.
The site.path.factory
service will essentially wrap DrupalKernel::findSitePath()
in order to compute the current site path.
Remaining tasks
#2457469: Remove usages of conf_path()
User interface changes
API changes
Beta phase evaluation
Issue category | Task because this issue refactors conf_path() out of use. |
---|---|
Issue priority | Normal because nothing is broken. |
Prioritized changes | This issue removes a deprecated function from use. |
Comment | File | Size | Author |
---|---|---|---|
#134 | interdiff.txt | 8.28 KB | Mile23 |
#134 | 2384675_134.patch | 35.24 KB | Mile23 |
#131 | interdiff.txt | 409 bytes | Mile23 |
#131 | 2384675_131.patch | 33.52 KB | Mile23 |
#128 | interdiff.txt | 947 bytes | Mile23 |
Comments
Comment #1
dawehner.
Comment #2
dawehnerComment #3
cilefen CreditAttribution: cilefen commentedThis is install.core.inc only.
Comment #4
cilefen CreditAttribution: cilefen commentedComment #5
cilefen CreditAttribution: cilefen commentedComment #6
cilefen CreditAttribution: cilefen commentedReroll of #5. This still needs much work.
Comment #7
dawehnerGreat work!
Comment #9
cilefen CreditAttribution: cilefen commentedRerolled with no conflicts.
@dawehner There are still ~25 instances of conf_path() to be converted.
Comment #10
cilefen CreditAttribution: cilefen commentedThis converts UpdateManagerInstall and UpdateReady. Remaining usages:
Comment #11
cilefen CreditAttribution: cilefen commentedConverted SiteSettingsForm.
@dawehner: \Drupal\Core\Site\Settings is interesting because of:
Remaining:
Comment #14
cilefen CreditAttribution: cilefen commentedConverted ReadOnlyStreamWrapperTest, SettingsRewriteTest, DirectoryTest, SimpleTestTest
Comment #15
cilefen CreditAttribution: cilefen commentedConverted:
Remaining:
Comment #21
damiankloip CreditAttribution: damiankloip commentedReroll and a couple of changes.
Comment #23
damiankloip CreditAttribution: damiankloip commentedWhoops, that was two interdiffs! Here is the real patch for #21.
Comment #25
sidharrell CreditAttribution: sidharrell commentedrerolled #22
Comment #27
sidharrell CreditAttribution: sidharrell commentedwhoops.
Comment #29
sidharrell CreditAttribution: sidharrell commentedProblem 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.
Comment #31
sidharrell CreditAttribution: sidharrell commentedrewound the patch to the state it had in #14, the last time it passed.
Comment #32
sidharrell CreditAttribution: sidharrell commentedchanging one at a time, to try to find which one is causing the test fails.
Comment #34
sidharrell CreditAttribution: sidharrell commentedreverted changes to ExtensionDiscovery.php
changed DummyReadOnlyStreamWrapper.php and DummyStreamWrapper.php
Comment #36
sidharrell CreditAttribution: sidharrell commentedoops.
Comment #37
sidharrell CreditAttribution: sidharrell commentedchanged InstallerExistingSettingsTest.php
Comment #39
sidharrell CreditAttribution: sidharrell commentedoops
Comment #40
sidharrell CreditAttribution: sidharrell commentedchanged TestBase.php
Comment #42
sidharrell CreditAttribution: sidharrell commentedreverted changes to TestBase.php
changed locale.install
Comment #43
sidharrell CreditAttribution: sidharrell commentedchanged system.install
Comment #44
sidharrell CreditAttribution: sidharrell commentedchanged update.manager.inc
Comment #45
dawehnerNice work!
Comment #46
sidharrell CreditAttribution: sidharrell commentedok, 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
Comment #47
sidharrell CreditAttribution: sidharrell commentedattempting 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?
Comment #48
cilefen CreditAttribution: cilefen commentedTo inject, a class needs to be a service, or implement ContainerInjectionInterface, which will allow the create method.
Comment #49
sidharrell CreditAttribution: sidharrell commentedTrying 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.
Comment #50
sidharrell CreditAttribution: sidharrell commentedLooks like this fixes the problem with PublicStream
Comment #51
sidharrell CreditAttribution: sidharrell commentedExtensionDiscovery
Comment #52
sidharrell CreditAttribution: sidharrell commentedIf the previous 2 come back good, then this, the final removal of the function conf_path() should pass, too.
Comment #53
sidharrell CreditAttribution: sidharrell commentedI 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.
Comment #57
cilefen CreditAttribution: cilefen commentedDrush or run-tests.sh take about the same amount of time for me.
Comment #58
sidharrell CreditAttribution: sidharrell commentedI'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.
Comment #59
sidharrell CreditAttribution: sidharrell commentedThink I got it figured out.
Half the time it hits PublicStream, the kernel service is not available.
Comment #60
sidharrell CreditAttribution: sidharrell commentedtrying the change to ExtensionDiscovery again, now that PublicStream is cleared.
Comment #62
sidharrell CreditAttribution: sidharrell commentedLooks like ExtensionDiscovery is the same way. The kernel service is not always available.
Comment #63
sidharrell CreditAttribution: sidharrell commentedfinal removal of conf_path
Comment #64
sidharrell CreditAttribution: sidharrell commentedSadly, 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.
Comment #65
sidharrell CreditAttribution: sidharrell commentedattempting the same sort of cleaner client code in
\Drupal\system\Tests\Installer\InstallerExistingSettingsTest
Comment #67
sidharrell CreditAttribution: sidharrell commentedLooks like
\Drupal\system\Tests\Installer\InstallerExistingSettingsTest.
is lacking the kernel service for 1 frakin test.
Comment #68
sidharrell CreditAttribution: sidharrell commentedAttempting the cleaner client code refactor in
drupal/core/modules/system/system.install
Comment #70
sidharrell CreditAttribution: sidharrell commentedsigh.
Comment #71
sidharrell CreditAttribution: sidharrell commentedComment #72
sidharrell CreditAttribution: sidharrell commentedThis 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.
Comment #73
cilefen CreditAttribution: cilefen commented@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:
A little whitespace here.
Don't actually remove conf_path() yet, we usually remove deprecated functions in a single issue.
Spacing is wrong here.
@dawehner, your thoughts?
Comment #74
sidharrell CreditAttribution: sidharrell commentedDid those 3.
Sorry, switched editors towards the end from sublime to netbeans, and had not set netbeans up to remove trailing whitespace.
Comment #75
dawehnerIt would be great if you could explain how this can be the case ...
Comment #76
cilefen CreditAttribution: cilefen commented@sidharrell Is it the case that sometimes ExtensionDiscovery is used before the dependency container is up?
Comment #77
sidharrell CreditAttribution: sidharrell commented@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.
Comment #78
dawehnerMh I think its fine, as long we explicitly document why we need this in those explicit places.
Comment #79
sidharrell CreditAttribution: sidharrell commentedadded those comments.
Comment #80
jbrown CreditAttribution: jbrown commenteddeleted
Comment #81
Wim LeersThis is much simpler than before — there are many possible incantations of
conf_path()
; the non-deprecated approach is much simpler. Great :)Before I RTBC:
Nit: missing leading backslash.
conf_path()
in this issue. I saw this in #73:… but in this case, the function already is deprecated!
Comment #82
sidharrell CreditAttribution: sidharrell commentedthe 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
Comment #83
Wim LeersComment #84
alexpottCan 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.
This information would be useful in the change record.
Comment #85
Wim LeersSo you disagree with #81.2? I don't understand why removing an already-deprecated function separately makes rollback easier?
Comment #86
sidharrell CreditAttribution: sidharrell commentedThis 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?
Comment #87
JeroenT@sidharrell,
You can reference this issue when you edit the change record (https://www.drupal.org/node/2275139/edit)
Comment #88
alexpott@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.
Comment #89
Wim LeersAhh! Thanks for the explanation, that makes a ton of sense :)
Updated the CR. Back to RTBC.
Comment #90
alexpottLooking 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.
Comment #91
cilefen CreditAttribution: cilefen commentedCopied-and-pasted to create a @site.path injectable string but it still needs integrating.
Comment #92
cilefen CreditAttribution: cilefen commentedComment #93
cilefen CreditAttribution: cilefen commentedComment #94
cilefen CreditAttribution: cilefen commentedComment #95
cilefen CreditAttribution: cilefen commentedI missed one, and removed an unrelated change.
Comment #96
cilefen CreditAttribution: cilefen commentedComment #98
neclimdulI 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?
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.
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.
necessary to be spread out and assigned to variable?
Comment #99
Mile23Comment #100
Mile23Comment #101
cilefen CreditAttribution: cilefen commentedThis is a reroll of #95. The suggestions in #98 need looking-into.
Comment #102
cilefen CreditAttribution: cilefen commented@neclimdul #98-1 is a response to the suggestion in #90.
Comment #104
Mile23Seems like this needs a little refactoring.
Since we only need the request,
@app.root
, and maybe asites.php
file to determine the site directory, let's movefindSitePath()
away from the kernel to the factory service. It adds some shim methods to provide anSplString
, 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.
Comment #106
dawehnerSorry @mile23, but can we please keep things as much in scope of the issue as possible? The issue is primarily about reducing the amount of calls to
conf_path()
.Do you mind open up a follow up to do your refactoring there? You know, everyone wants to make some form of progress, but the only way to make progress
are small steps, and factoring getSitePath() out of the DrupalKernel is a separate task for its own.
Comment #108
cilefen CreditAttribution: cilefen commentedBased on @alexpott's suggestion in #90, #2384675-90: Deprecate conf_path() and the direction taken in #101, this issue may need a new title.
Comment #109
Mile23Updated 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.
Comment #110
Mile23Comment #111
dawehnerThank you @mile23!!!!!
Comment #117
Mile23Needed a reroll.
Comment #118
Mile23Comment #119
Mile23Comment #120
Mile23My mistake.
Comment #121
Mile23#117 passed on May 2. Retesting now.
Comment #123
larowlanCan this be injected? Would need to be optional I realise.
Do we have $this->container here?
Other than that - looks good to me
Comment #124
Mile23Will 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
Comment #125
Mile23Added an injected site path for PublicStream::basePath().
Comment #126
dawehnerI'm curious whether we should care about the difference of FALSE, vs. TRUE here? Isn't that something which could matter on the installer?
Can we explain here why the kernel is not available that early?
Comment #127
Wim LeersPer #126.
Comment #128
Mile23#126.1: I presume you mean the
'required' => TRUE
part in this: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.
Comment #129
Mile23Also 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.Comment #131
Mile23Woot. Symfony 2.7.
Comment #132
dawehnerThank you!
Comment #133
alexpottconf_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.Let's call the variable
$site_path
to be consistent.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...
Comment #134
Mile23#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.
Comment #135
dawehnerThe feedback from alex got adressed
Comment #136
alexpottMissed 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!
Comment #138
cilefen CreditAttribution: cilefen commentedWe missed a class #2503015: Remove usages of conf_path() in InstallerExistingSettingsNoProfileTest.
Comment #139
cilefen CreditAttribution: cilefen commented#2503017: Rename the migration setting conf_path to site_path
Comment #140
Mile23And this, mentioned in #129. #2503267: drupal_install_config_directories() uses path literals instead of settings to find config directory.