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.
config_get_config_directory() does not use file public path setting but hard-codes it instead
Comment | File | Size | Author |
---|---|---|---|
#76 | 1856766_76.patch | 8.02 KB | mtift |
#76 | interdiff.txt | 753 bytes | mtift |
#74 | 1856766_74.patch | 7.29 KB | chx |
#74 | interdiff.txt | 3.35 KB | chx |
#71 | 1856766_71.patch | 5.03 KB | mtift |
Comments
Comment #1
adammaloneComment #2
gddReopening this since the public file path was removed from #1496480: Convert file system settings to the new configuration system
Comment #3
BerdirHere's a start to convert this. Added a helper function to avoid duplicating the default value in a million places and support the same within-test override as we have for config_get_config_directory().
Comment #5
BerdirOk, the PhpStorageFactory.php obviously also need to check for an in-test override. So we either need to make sure that we always pass in a directory (at least when we're in a test) or also use file_public_path(). I've done the second for now, to make sure that this does fix the tests, which I think it should.
Comment #6
BerdirI think two things should be answered to get this in:
- Should we add that file_public_path() method to handle the default and in-test override? Alternatively, we could also write the default/test-value into $settings.
- What do we want to do with PhpStorageFactory? Ensuring the settings()->get('file_public_path') always returns the right path would allow to simply inject that, or we could make sure that the directory is always set outside of it or keep it like that...
Comment #7
Dave ReidI like the addition of the helper function, but considering it contains testing-only code just really sticks out to me as wrong. Is it absolutely necessary?
Comment #8
BerdirIt is the same logic as config_get_config_directory() basically, in both cases, we need *something* as it depends on something that can only be changed in settings.php otherwise and we currently can not write a custom one for tests.
The only alternative is overwriting the setting from the outside during bootstrap, similar to how we overwrite the database settings.
Comment #9
gddI actually think this is fine as it is, if we want to tweak it we can do it in followups. Thanks!
Comment #10
sunHm. I understand the intention, but this looks extremely dynamic to me.
E.g., the test setup + teardown procedures need to be really really careful and call file_public_path() in the right/correct moments. If it's called too early, problem. If it's called too late, problem.
At the very least, we'd have to add some extra validation to the test environment setup/teardown functions to make sure that the returned path is the expected path (i.e., when the simpletest path is expected, it must contain the literal string "/simpletest/").
Additionally, the first returned path appears to be wrong: The public files path of a test environment is within the public files path of the parent-site/test-runner. The file_public_path from settings is the base path in both cases.
1) I do not see a change to settings.php in this patch? If this setting is expected to be contained and configured there, then we should have a proper section for it by default in settings.php.
2) Likewise, the installer does not write this value?
3) Upgrade path?
4) The #after_build seems to be obsolete, since the setting cannot be changed. EDIT: #maxlength, too.
Comment #11
gddThis patch does the following:
- Modifies base path of simpletest public file path
- Adds the setting to default.settings.php with a default value
- Removes #after_build and #maxlength
- Fixes testFileConfigurationPage() that had been recently added but was broken with this patch. I think this is the right way to do that since its just a read-only value now, but verification would be nice.
I'm not really sure this is necessary, or I don't understand why this different than what we're doing in config_get_config_directory().
Since we provide a default in default.settings.php in this patch, the installer shouldn't have to write anything.
None of the other $settings conversions has provided an upgrade path either, and it looks a bit hairy. Ideally we could change drupal_rewrite_settings() to allow passing in $settings variables but that seems like a bigger issue than we want to tackle here. I'd rather push that to a followup and do the upgrade path for all the $settings at once there. I've created #1921818: Modify drupal_rewrite_settings() to allow writing $settings values and #1921822: Take account of drupal_hash_salt during migration path from 7.x for that.
Comment #12
chx CreditAttribution: chx commentedI have more than once hit the wall with trying to write $conf['foo'] = 'bar' from install with drupal_rewrite_settings. This is not easy so moving it to a followup or followups is absolutely sensible.
Comment #14
gddI started looking into those fails but I have to admit I'm dumbfounded. Any one else?
Comment #15
BerdirThe problem with this is that this now returned nested directories within the test (as we explicitly set the file public path there) but not on page callbacks.
However, doing this means that we don't need to set the file_public_path at all in setUp(), so removed that. And we also need to directly use settings()->get() when detecting the original directory.
Note that this change means that simpletest directories for nested tests are now no longer nested but are on the same level with a different test id, just like parallel tests are too.
Comment #17
BerdirHm, looks like this breaks other tests, so removing the setting doesn't fix it properly either.
Comment #18
BerdirHm.
I think I got it this time. It's a bit ugly, but I'm not sure if anything else will work.
Comment #19
kim.pepperRe-roll of #18
Kim
Comment #20
chx CreditAttribution: chx commentedI am reasonably sure that
$settings['file_public_path'] = conf_path() . '/files';
insettings.php
will a) eventually have some really unfun side effect -- the thing is, if you have a multisite, it might not be a terribly good idea to have it dynamic... b) totally unnecessary anyways cos settings()->get() default just contains it. What about adding# $settings['file_public_path'] = 'sites/default/files';
instead?Comment #22
BerdirRe-roll with settings definition in default.settings.php changed as suggested.
Wondering if we can use the new test settings.php/confg path for this now. It might just work if we move the public files path to $base_path . '/simpletest/' . substr($test_prefix, 10) . '/files'?
Comment #24
BerdirMessed up the settings set in setUp() during the re-roll.
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedI support the approach in #24, but retitling issue to reflect it.
Comment #26
effulgentsia CreditAttribution: effulgentsia commented:)
Comment #27
Berdir#24: file-public-path-settings-1856766-24.patch queued for re-testing.
Comment #29
moshe weitzman CreditAttribution: moshe weitzman commented#24: file-public-path-settings-1856766-24.patch queued for re-testing.
Comment #31
chx CreditAttribution: chx commentedComment #33
moshe weitzman CreditAttribution: moshe weitzman commented#31: 1856766_31.patch queued for re-testing.
Comment #35
dburiak CreditAttribution: dburiak commentedWe are working now with this issue during Code Sprint UA.
Comment #36
chx CreditAttribution: chx commentedGreat! I am not even sure whether that was a valid test fail; please find me in IRC if you need any help.
Comment #37
jibran#31: 1856766_31.patch queued for re-testing.
Comment #39
chx CreditAttribution: chx commentedI am not sure how the hell this can be "normal" when #1859110: Circular dependencies and hidden bugs with configuration overrides probably depends on this...
Comment #41
chx CreditAttribution: chx commentedWhat just happened?
Comment #43
chx CreditAttribution: chx commentedOK, that's a stupid mistake.
Comment #44
chx CreditAttribution: chx commentedAnd here's a version for the OOP zealots. Obviously, the patch and the resulting code is some 15% percent larger but we can't have a function call in a class, right? Everything, including code bloat and inscrutable code is better, isn't it?
Comment #45
BerdirMissing docs :)
Comment #47
chx CreditAttribution: chx commentedComment #49
jibran#47: 1856766_47.patch queued for re-testing.
Comment #50
chx CreditAttribution: chx commentedI have written an update function. It's pointless to test it. Also, run-tests.sh test coverage is um. Not stellar. Not sure what can be done, of course.
Comment #51
BerdirOk, this is fixing a pretty serious crazyness that is currently blocking #1862202: Objectify the language system, see comment #142. Most notably, the public file path is not always within the simpletest files directory, so we're looking in different places depending on when it's called during bootstrap, that results in too many container dumps and flushes/deleteAll() calls that don't work.
Raising to critical, need to find someone that can RTBC this to unblock the other issue.
Comment #52
tim.plunkett\Drupal
Missing trailing .
Capital T in The
Most of our statics are
public static function
Missing blank line at the end of the file
Womp womp. This is going to suck for site builders...
This is in the original code, but why only one slash?
Nice!
Comment #53
Anonymous (not verified) CreditAttribution: Anonymous commentedoverall, this looks great. one tiny nitpick:
missing space.
Comment #54
BerdirFixed nitpicks.
- I don't think it's a big problem for site builders, how many are actually changing this? And it's actually getting quite complicated to do it in 8.x, the config directory is within the *default* public site directory, so you can't move that. I have a separate issue to separate that from this completely and use a relative path to DRUPAL_ROOT instead but that didn't make it :(
- The single slash in public:/ is because we want to replace public://whatever with '/some/path/bla' . '/' . 'whatever', so the path we're replacing it with doesn't have a trailing slash and so we keep the second of public:// instead of replacing it with a hardcoded /.
Comment #55
catchThis isn't remotely a problem for sitebuilders, the fact we allow this to be set in the UI at all is the problem. When you change this setting, no files get moved, no hard-coded links in textareas get updated etc. etc. so you can easily end up with a broken site. The only case where you might be OK is if you don't have any public files at all yet.
Comment #56
tim.plunkettHmmm, I guess you're right.
Anyway, the code looks great. Thanks!
Comment #57
alexpottCommitted 5460c03 and pushed to 8.x. Thanks!
We need a change notice detailing this change because whilst a site builder should never alter this setting they could in the past.
Comment #58
chx CreditAttribution: chx commentedErm, nope. We call drupal_install_config_directories(); before drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE);
Comment #59
chx CreditAttribution: chx commentedComment #60
chx CreditAttribution: chx commentedActually, I need to renumber the rest of UPGRADE.txt
Comment #61
Berdirconfig_get_config_directory() and therefore drupal_install_config_directories() does not rely on the file_public_path at the moment. It hardcodes "conf_path() . '/files/'". However, that won't actually work and will explode if you don't have a files directory there.
But we either need to change that and use the setting there or we can keep he upgrade path, knowing that it will be broken ;)
Comment #62
chx CreditAttribution: chx commentedComment #63
tstoecklerTypo: 2ö -> 20
Comment #64
chx CreditAttribution: chx commentedChanged config_get_config_directory to use settings()->get('file_public_path', conf_path() . '/files');
Comment #66
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedMissing : after https.
Comment #67
chx CreditAttribution: chx commentedComment #68
BerdirStill missing a ":" in here :(
Is it not possible to use the static method here?
Should at least use Drupal::settings().
Comment #69
webchickTagging all critical D8 upgrade path issues as "beta blocker."
Comment #70
catchComment #71
mtiftRe-roll that addresses comments in #68.
Comment #73
chx CreditAttribution: chx commentedI will fix this.
Comment #74
chx CreditAttribution: chx commentedThis patch is dedicated to the wonderful music sToa: Candide without which I could never figure this out.
Comment #76
mtiftSo maybe something like this will get us closer. I have some serious doubts.
Comment #78
ianthomas_ukI spotted this in \Drupal\system\Tests\InstallerTest::setUp()
$config_directories is not defined though. I don't know enough about this code to know how/where it should be defined (my guess would be that we just call global $config_directories, as there is a similar change to another test in this patch.
Comment #79
chx CreditAttribution: chx commentedComment #80
catchRe-titled/updated issue summary.
Comment #81
alexpottI think we should close this in favour of #1887750: Use path relative to DRUPAL_ROOT in configuration directories
Comment #82
BerdirRestoring the original issue title, this is one of the critical-follow-ups that you like so much ;)
Comment #84
sun