locale.settings:translation.path is the last system path that's still in configuration after private and I think originally also public moved to settings.
It's also the last usage of the pretty weird system_check_directory function. With that done, we can deprecate that function without replacement.
Do what we did for the private path setting, convert to Setting with fallback to config. #2170235: file_private_path should be in $settings, like file_public_path
Of course this is a bit different, this isn't a stream wrapper, but I'll see where it ends up.
Close #3073684: [pp-1] Deprecate system_check_directory() as outdated when this lands.
The configuration setting is deprecated and can no longer be changed in the UI. Set $settings['locale_translation_path'] = '/the/path' instead. The update will remove and notify about a customized value.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | Screenshot_20260207_094734.png | 104.93 KB | berdir |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !14644
Comments
Comment #3
berdirGot started with this. Works fairly well I think based on the tests.
Comment #4
berdirThe update path test deprecations are awkward.
I added a post update, but it still fails. The problem is arguably a bug in the current behavior, which is an actual path, and it's sites/default/files/translations. The problem is that in the test, public://translations resolves to the simpletest site path, so it's not the same and it's not cleaned up.
One option is we just unconditionally kill the old setting, then we could also possibly drop the runtime fallbacks. It doesn't really matter, it's just a cache really. We could display a message if it didn't match.
Comment #5
nicxvan commentedIs it that or is it the fixtures?
Comment #6
berdirNot exactly sure what you mean with "that", but the point is that the new default path is dynamically adjusted to the current test-multisite-path while the one in the fixture is not. It wouldn't be a update of the static fixture but would need to dynamically set it up after the test setup.
Doable, but before going that path, I want to decide whether or not we even want to support or consider the old configuration.
Comment #7
nicxvan commentedSorry by that I meant:
The failure I checked had the 10.3 fixture i searched through it and I think it had the paths config but I may have been mistaken.
Comment #8
berdirYes, it has the path config. The problem is that the expected new default path is now dynamic within a test and contains the test ID, we can not just update the static definition. We'd need to add something into \Drupal\FunctionalTests\Update\UpdatePathTestBase::installDrupal or so.
It's doable, but I think I'd prefer the alternative option, we just drop it and let the user know if they had a non-default value.
There typically aren't that many messages, not in the UI anyway on this page. This is how the output of that test looks like now:
Comment #9
berdirThat was the wrong file:
Comment #10
nicxvan commentedWould @gabor be the one to make that decision?
Comment #11
gábor hojtsyWe've discussed this extensively in Slack. If there would be a method to provide this kind of feedback that is higher level than the message but is lower level than throwing an error from the update function, we should use that, but there is none. I agree that this is very rarely customized for highly specialized scenarios (eg. distributions or SaaS products using their own translation server, multisites optimizing for one shared directory to download translations, etc). They would still need to take note. The message in the update function will help. Also we need a release note. There is already a change record which may be good to copy into the release note snippet too? :) Otherwise IMHO look good from an interaction perspective.
Comment #12
berdirComment #13
berdirThanks for the feedback. I've made a suggestion for the release notes, which is identical to the CR.
Comment #14
andypostLooks ready to go except of merge conflict
Comment #15
nicxvan commentedGot a suggestion that should solve the failures.
Release notes look good, I updated the cr as well.
Once this is green I think this is ready!
Comment #16
nicxvan commentedComment #17
berdirYes, that's been deprecated in the meantime.
Comment #18
nicxvan commentedLooks good!
Comment #19
alexpottAdded a long comment about the update process. I think we should not clear the value if it does not match the default. And we should add a requirements error if the path is set in config after the update and
TranslationsStream::basePath()is returning something different.Comment #20
nicxvan commentedThat is a pretty clever solution, we should do the same in the field purger service issue:
#2907780: Add a field purger service
Comment #21
alexpottActually we can use an update hook in the follow-up for main to tell people to update. We don't need to add a requirements hook here. That's what an update requirement is for :)
Comment #22
berdirThen I think this can go back to review, updated the update hook and "fixed" the update data to use the adjusted value so it matches in our update tests.
The default schema validator in development services will also complain about this.
Comment #23
nicxvan commentedThis is a little messier in the implementation but it's much more BC friendly since we keep the custom value.
I think this addresses alexpott's comments!
RTBC again.
I reviewed all of the commits since it was set back to needs work and the full change again.
Comment #24
nicxvan commentedWhoops, meant to change status.
Comment #25
alexpottCommitted and pushed 05ecfda70b1 to main and a7cbb9d8e33 to 11.x. Thanks!