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

Reference: https://www.drupal.org/core/beta-changes
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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Needs work » Active

Needs patch.

sidharrell’s picture

sorry, this should be blocked on #2384675: Deprecate conf_path()

sidharrell’s picture

Issue tags: +Novice, +needs patch
tim.plunkett’s picture

Status: Active » Postponed
Parent issue: #2384675: Deprecate conf_path() »
cilefen’s picture

Status: Postponed » Active

This is un-postponed but note the comments on #2384675-136: Deprecate conf_path().

cilefen’s picture

joshi.rohit100’s picture

Status: Postponed » Needs review
FileSize
1.93 KB
cilefen’s picture

@joshi.rohit100 This would have been a good novice issue. It needs a change record or a change record update.

joshi.rohit100’s picture

I think, CR (https://www.drupal.org/node/2275139) already contains this.

cilefen’s picture

I 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.

Status: Needs review » Needs work

The last submitted patch, 7: 2457469-6.patch, failed testing.

cilefen’s picture

Mile23’s picture

Status: Postponed » Needs work
Issue tags: +@deprecated
Parent issue: » #2205673: [META] Remove all @deprecated functions marked "remove before 8.0"
Mile23’s picture

Mile23’s picture

Category: Task » Bug report
Issue summary: View changes

Added beta eval.

JeroenT’s picture

cilefen’s picture

Is 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.

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
1.98 KB

Change request looks good - it explicitly mentions what to do if you're calling conf_path().

ianthomas_uk’s picture

cilefen’s picture

Status: Needs review » Reviewed & tested by the community

#19 is fine with me if it passes.

JeroenT’s picture

Status: Reviewed & tested by the community » Needs work

There are still some occurrences of conf_path in example.sites.php and Drupal\KernelTests\KernelTestBase.

nlisgo’s picture

Status: Needs work » Needs review
FileSize
1.52 KB
3.5 KB
JeroenT’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good. All occurrences of conf_path() are gone so RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Postponed

So 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.

dawehner’s picture

Agree, there are hosting platforms which rely on drush and have a really "rapid" deployment cycle of new drush versions.

ianthomas_uk’s picture

Opened 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.

cilefen’s picture

The 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.

Mile23’s picture

Status: Postponed » Needs work

We can't really afford to wait for hosting providers to catch up, if we want this in before RC1.

a) 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.

ianthomas_uk’s picture

The 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.

Mile23’s picture

The deprecations (or at least the dates) were always an aspiration.

Disagree.

catch’s picture

The deprecations (or at least the dates) were always an aspiration.

Disagree.

Especially (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'.

cilefen’s picture

Status: Needs work » Postponed
Related issues: +#2573443: Remove conf_path() from core

It 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?

cilefen’s picture

Issue tags: -Novice

Removing the Novice tag because this is not actionable right now.

Mile23’s picture

Title: Remove conf_path() » Remove usages of conf_path()
Issue summary: View changes

Seems like it would have been easier to rescope here and then create a new deletion issue.

Mile23’s picture

Status: Postponed » Needs work
+++ b/core/includes/bootstrap.inc
@@ -122,47 +122,6 @@
-function conf_path($require_settings = TRUE, $reset = FALSE, Request $request = NULL) {

We're not actually removing conf_path() here due to rescope.

cilefen’s picture

Re #34, Duh, true. Should I repurpose the other issue for the removal?

cilefen’s picture

Oh, you got there. THanks.

nlisgo’s picture

Assigned: Unassigned » nlisgo

Working on a patch.

nlisgo’s picture

Assigned: nlisgo » Unassigned

Assigning to @luukyb

Luukyb’s picture

Assigned: Unassigned » Luukyb

Working on a patch.

Luukyb’s picture

Status: Needs work » Needs review
Issue tags: +Barcelona2015
FileSize
1.52 KB

Hi,
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

Luukyb’s picture

Assigned: Luukyb » Unassigned
nlisgo’s picture

Assigned: Unassigned » nlisgo

I will review the patch and confirm that there are no other instances of conf_path() in the code.

nlisgo’s picture

Assigned: nlisgo » Unassigned
Status: Needs review » Reviewed & tested by the community

The 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!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 37bcc5a and pushed to 8.0.x. Thanks!

  • alexpott committed 37bcc5a on 8.0.x
    Issue #2457469 by nlisgo, ianthomas_uk, Luukyb, joshi.rohit100, cilefen...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.