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.
Follow-up to #2384675: Deprecate conf_path()
Original change record: https://www.drupal.org/node/2275139
Problem/Motivation
conf_path()
should be completely removed from core as of #2384675: Deprecate conf_path() and has been marked deprecated for some time.
In this issue we will only remove conf_path()
usages in core.
Proposed resolution
Refactor usages of conf_path()
Remaining tasks
Remove conf_path()
itself.
User interface changes
none
API changes
removal of deprecated function, conf_path().
Beta phase evaluation
Issue category | Bug because conf_path() is deprecated and it's not yet removed. |
---|---|
Issue priority | Major because deprecated code must be removed before release. |
Prioritized changes | Prioritized because it removes usages of deprecated code, enabling the eventual removal. |
Comment | File | Size | Author |
---|---|---|---|
#41 | core-remove_conf_path_calls-2457469-41.patch | 1.52 KB | Luukyb |
#22 | remove_conf_path-2457469-22.patch | 3.5 KB | nlisgo |
#22 | interdiff-2457469-17-22.txt | 1.52 KB | nlisgo |
#18 | 2457469-17-remove-conf_path.patch | 1.98 KB | ianthomas_uk |
#7 | 2457469-6.patch | 1.93 KB | joshi.rohit100 |
Comments
Comment #1
tim.plunkettNeeds patch.
Comment #2
sidharrell CreditAttribution: sidharrell commentedsorry, this should be blocked on #2384675: Deprecate conf_path()
Comment #3
sidharrell CreditAttribution: sidharrell commentedComment #4
tim.plunkettComment #5
cilefen CreditAttribution: cilefen commentedThis is un-postponed but note the comments on #2384675-136: Deprecate conf_path().
Comment #6
cilefen CreditAttribution: cilefen commentedRe-postponed on #2503015: Remove usages of conf_path() in InstallerExistingSettingsNoProfileTest.
Comment #7
joshi.rohit100Once https://www.drupal.org/node/2503015 is committed.
Comment #8
cilefen CreditAttribution: cilefen commented@joshi.rohit100 This would have been a good novice issue. It needs a change record or a change record update.
Comment #9
joshi.rohit100I think, CR (https://www.drupal.org/node/2275139) already contains this.
Comment #10
cilefen CreditAttribution: cilefen commentedI see. That is probably the only relevant CR. I think for a removal like this we usually post a CR, something like "conf_path() has been removed". We can even reference the other CR for the new usage, to not be repetitive. But we could go either way. Actually the title of the older CR could simply be edited to indicate conf_path() is removed. That would also work for me.
Comment #12
cilefen CreditAttribution: cilefen commentedPostponed on #2487592: CMI: don't ship with a default "active" directory that is empty in most Drupal installations
Comment #13
Mile23Opening this up so it's not overlooked. We should still work on #2487592: CMI: don't ship with a default "active" directory that is empty in most Drupal installations
Comment #14
Mile23Comment #15
Mile23Added beta eval.
Comment #16
JeroenT#2487592: CMI: don't ship with a default "active" directory that is empty in most Drupal installations is in so this issue is unblocked.
Comment #17
cilefen CreditAttribution: cilefen commentedIs everyone ok with this CR: https://www.drupal.org/node/2275139?
conf_path()
has been deprecated for three months. The other option is a new CR specifying conf_path() is now removed and linking to the detailed CR on its replacement code.Comment #18
ianthomas_ukChange request looks good - it explicitly mentions what to do if you're calling conf_path().
Comment #19
ianthomas_ukComment #20
cilefen CreditAttribution: cilefen commented#19 is fine with me if it passes.
Comment #21
JeroenTThere are still some occurrences of conf_path in example.sites.php and Drupal\KernelTests\KernelTestBase.
Comment #22
nlisgo CreditAttribution: nlisgo commentedComment #23
JeroenTPatch looks good. All occurrences of conf_path() are gone so RTBC.
Comment #24
alexpottSo this will break drush which will break my workflow for developing patches which will slow down D8 development. Let's fix drush before we do this.
Comment #25
dawehnerAgree, there are hosting platforms which rely on drush and have a really "rapid" deployment cycle of new drush versions.
Comment #26
ianthomas_ukOpened https://github.com/drush-ops/drush/issues/1607
My reading of @alexpott's comment is that he'd be happy to review/commit this as soon as a version of drush without this dependency is available. We can't really afford to wait for hosting providers to catch up, if we want this in before RC1.
Comment #27
cilefen CreditAttribution: cilefen commentedThe blocking issue https://github.com/drush-ops/drush/issues/1607 has a clear next-step if anyone wants to try it. I will do it if I can find a little time.
Comment #28
Mile23a) This is why drush is a bad dependency for a maintainer especially, and why we should have as much as possible of that behavior in an internal-to-Drupal console with tests.
b) Un-removed deprecations should be a blocker on RC1, or they were a lie when they were marked as @deprecated for removal before 8.0.0.
Comment #29
ianthomas_ukThe deprecations (or at least the dates) were always an aspiration. If people updated their modules and then we didn't end up removing the function then nothing's lost, they've still upgraded to the new, recommended API. If we didn't deprecate the function and then removed it then we'd have broken a promise we made to module maintainers.
We should do our best to remove the functions, but if we can't then we have to ask if we're prepared to accept their maintenance burden until Drupal 9. Only if we're not do they need to block Drupal 8.
Comment #30
Mile23Disagree.
Comment #31
catchEspecially (but not only) early in the use of @deprecated, a lot of functions were deprecated before all conversions had been finished.
Some functions were deprecated before they had any viable replacement in core at all, and before any conversions had been done at all, just because someone really hated them and was hoping to get rid of them.
Later on, we realised that this was an awful way to handle deprecation and tidied things up a lot (in fact ianthomas_uk did a lot of that work), but aspirational is a really, really polite way to describe some of those earlier @deprecated patches.
The fact that this patch removes two calls to conf_path() is a symptom of exactly that. Certainly not the worst one, but the fact that conf_path() isn't a single line wrapper around something else means this patch might not have been straightforward to roll - had something in core been relying on that specific behaviour. The only way to know for sure would be for conf_path() to be completely uncalled dead code.
So yes, 'aspirational'.
Comment #32
cilefen CreditAttribution: cilefen commentedIt is certainly postponed on #2573443: Remove conf_path() from core and effectively postponed on https://github.com/drush-ops/drush/issues/1607. We typically remove all usages, then remove the deprecated function, true?
Comment #33
cilefen CreditAttribution: cilefen commentedRemoving the Novice tag because this is not actionable right now.
Comment #34
Mile23Seems like it would have been easier to rescope here and then create a new deletion issue.
Comment #35
Mile23We're not actually removing conf_path() here due to rescope.
Comment #36
cilefen CreditAttribution: cilefen commentedRe #34, Duh, true. Should I repurpose the other issue for the removal?
Comment #37
cilefen CreditAttribution: cilefen commentedOh, you got there. THanks.
Comment #38
nlisgo CreditAttribution: nlisgo commentedWorking on a patch.
Comment #39
nlisgo CreditAttribution: nlisgo commentedAssigning to @luukyb
Comment #40
Luukyb CreditAttribution: Luukyb commentedWorking on a patch.
Comment #41
Luukyb CreditAttribution: Luukyb commentedHi,
Here is an updated patch to remove the calls to "conf_path()" and replace them with "\Drupal::service('site.path')".
Documentation updated too on example.sites.php.
Cheers,
Luc
Comment #42
Luukyb CreditAttribution: Luukyb commentedComment #43
nlisgo CreditAttribution: nlisgo commentedI will review the patch and confirm that there are no other instances of conf_path() in the code.
Comment #44
nlisgo CreditAttribution: nlisgo commentedThe attached interdiff confirms that the only difference with the patch in #41 and the patch in #22 is the reinstating of the conf_path() function which is expected.
The successfully test run of the patch in #22 confirms that Drupal core tests pass without the conf_path() being present in the PHP code. I also searched the codebase for uses of the function and couldn't find any.
This looks good to me!
Comment #45
alexpottCommitted 37bcc5a and pushed to 8.0.x. Thanks!