[pp-1] Deprecate system_check_directory()

Nice clean-up

+++ b/core/modules/locale/locale.module
@@ -758,7 +758,7 @@ function locale_form_system_file_system_settings_alter(&$form, FormStateInterfac
-    '#after_build' => ['system_check_directory'],
+    '#after_build' => ['locale_form_check_directory'],

@@ -767,6 +767,51 @@ function locale_form_system_file_system_settings_alter(&$form, FormStateInterfac
+function locale_form_check_directory($form_element, FormStateInterface $form_state) {

btw not sure that new "API" method in locale makes sense, usage in contrib is not big http://grep.xnddx.ru/search?text=system_check_directory

I was originally thinking we could just move it to an .inc file and load that in locale module, to get it out of the hotpath.

We should be doing the same here, putting it in locale.module also means its in the hotpath for every page.

actually, could we use a static method on a class to make it autoloaded on demand?

My 2c

Keep the static method, but make it do \Drupal::classResolver()->getInstanceFromDefinition(self::class)->{aProtectedMethodThatIsntStatic} and move the logic into a protected method that isn't static

If your class implements ContainerInjectionInterface, it will call the ::create method and pass the container

using \Drupal::service means its in the container for all requests, which it doesn't need to be

there's no harm in keeping it static fwiw

Just some quick thoughts:

#6 shows that is not used *often*, but it is used a few times and there are likely quite a few more use cases that could use it, they probably just don't know about it. Given that it's autoloaded now, any harm in putting it somewhere outside of locale module? maybe Drupal\Core\Form, given how tightly integrated into the form system it is.

The "its loaded on every request." argument feels like micro-optimization, there's nowway that the existence of a 30loc function is in any way relevant for performance. The bigger argument is that having it has a service directly would get mean that we'd have to serialize the object and its dependencies as part of the form for the callback definition, that's considerably more overhead than having a function or service defined. Thought about a trait, would have the advantage that you can just define it as '::checkDirectory', but that requires each form using it to inject the necessary dependencies. So the approach in the patch, although a bit uncommon, seems like an interesting solution, maybe we could document how it's supposed to be used on the method, specifically if we make it an actual API?

As usage shows it's used by form element to ensure that directory exists and writable (puts htaccess?)
So probably better create special element class for that?

Replace deprecated code.

Rerolled for 10.1

What file should that go to? The file SystemFunctionsLegacyTest is deleted

Makes sense! Can I mark RTBC

I'd want to rtbc but naming sounds weird, form helper class named as Settings*

No idea ATM for better name but I bet it will not pass commiters

Thinking just FormHelper?

+++ b/core/lib/Drupal/Core/Form/SettingsFormHelper.php
@@ -0,0 +1,112 @@
+ * Provides form helper methods.
...
+class SettingsFormHelper implements ContainerInjectionInterface {
...
+      $container->get('logger.factory')->get('file system'),
+      $container->get('file_system')

maybe instead of helper use a ElementFileSystem

OTOH there's only 8 usages in contrib so probably better to inline it into Locale module and deprecate without replacement

http://codcontrib.hank.vps-private.net/search?text=system_check_director...

Updated IS

maybe instead of helper use a ElementFileSystem

Not sure I follow

Or

better to inline it into Locale module and deprecate without replacement

per #43

Believe this is still valid.

Rerolled in MR. Please leave review comments in MR.

Small comment on MR

Looking at the CR, believe it should be fine but personally I'm big fan of before/after snippets.

Super close and will keep an eye out for this one.

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Addressed review comments and updated CR

Thanks feedback appears to be addressed

One comment on the MR.

Introduced logger channel as a dependency

A bunch of failures looks like a service is wrong.

The reason that only locale still uses this function is that we moved configuration of private/public/tmp stream wrappers to Settings in settings.php.

What if we do the same instead with this locale setting? Then we don't need any replacements.

(I had some concerns about removing as there are a handful of modules using this based on code search. But only 2 of those have a release that works on a supported core version, l10n_server and library_manager) and those don't seem to use it in current versions. So should be OK).

Thank you everyone for the great direction here.

I think we should postpone this even if it's older since the .module cleanup initiative is grouping these cleanup efforts to reduce conflict and churn.

I'll be sure to apply credit here before closing it out if we do go that direction.

There are only a few remaining functions and they are all relatively simple so we should just take care of all of them at once.

I created #3571593: Deprecate translation.path config in favor of settings, whether we deprecate this function here or in the other issue, that non-trivial but also not crazy-complex issue will make it trivial.

yeah maybe we postpone this on that one for now, and we address it there or here, I'll work on the other functions in the other issue.

better search link: system_check_directory, i see only 2 contrib use that looks legit

I think the other one is very close.

I took a pass at credit to prepare.

Thank you everyone! The other one landed so this is now outdated, I already applied credit in 65.