Deprecate drupal_set_time_limit() and file_upload_max_size() and move to Environment component

Do we even strictly need this? Can't we just deprecate and suggest people use the built in functions directly?

The 240 seconds in node.module feels a bit arbitrary as well.

+++ b/core/lib/Drupal/Core/Php.php
@@ -0,0 +1,46 @@
+  public static function setTimeLimit($time_limit) {
+    if (function_exists('set_time_limit')) {
+      $current = ini_get('max_execution_time');

Btw it more about Environment then php itself

If we do new implementation that let's polish it a bit

+++ b/core/lib/Drupal/Core/Php.php
@@ -0,0 +1,46 @@
+        @set_time_limit($time_limit);

why error is not handled in new implementation, it makes sense to return success status from function call at least

I am also wondering whether this could belong into \Drupal\Component\Utility?

I think it does belong in Utility, this is something that can be used in another application without needing any other drupal things.

I struggle a bit how utility becomes our dumping ground for all things we can't put into a better place.
On the other hand, this allows us to potentially see an order.

As per #6, #8 and #9, it seems creating a new class is not desirable.

Rerolled.
Addressed #10

What about adding it to \Drupal\Component\Utility\Environment? That already has a php settings/memory limit method, so I think this would work quite well there?

I re-rolled #11 and moved it to Environment as @Berdir suggested. Makes more sense there I think.

Also, we plan on moving file_upload_max_size() here too, which I will do in the next patch.

This moves file_upload_max_size() to \Drupal\Component\Utility\Environment::getUploadMaxSize() and deprecates it.

Fix FormBuilder.

Need to update the IS and CR if we are going down this path.

> Why do we keep this protected method? It was designed only to wrap the procedural function call.

Was about to post the same, but sometimes these kind of functions are then mocked and used to provide different values.

Was about to post the same, but sometimes these kind of functions are then mocked and used to provide different values.

Yep, that's exactly what FormBuilderTest is doing.

I've updated the title, IS and CR.

Ok, then lets see what others think about my suggestion to move them to the Environment class. I've also fixed the wrong namespace in the CR.

FWIW, +1 to expanding Environment instead of adding a whole new Component.

"Provides PHP environment helper methods." says the phpDoc for that class. That's what these are. ;)

No time for a careful review of #17 but given who's participating in here, I assume it's fine. ;) If I get a chance I'll take a closer look later today.

Thanks,
-Derek

FWIW, in #3000050: Replace \Drupal\Component\Utility\Environment::checkMemoryLimit with a PHP memory checking helper class and service I was suggesting to move the existing Environment class to an instantiable one so that it could be unit tested with mocks. The concept there was to enable testing memory checks outside of real PHP memory. With static methods that's not possible https://github.com/sebastianbergmann/phpunit-documentation/issues/77.

Had a chance for a deeper look. Overall, still +1 to this. All I have I are nits:

1. General: I agree both of these functions should be deprecated and moved into Environment, but I wonder if we should do them in separate issues to make them easier to review/commit? Maybe we should have kept the file_upload_max_size() work at #3021652: Deprecate upload-related functions file.inc and move to a file upload service instead of merging into this patch? *shrug*

2. +++ b/core/includes/common.inc
   @@ -11,16 +11,17 @@
   -use Drupal\Core\Render\HtmlResponseAttachmentsProcessor;
   +use Drupal\Core\Render\BubbleableMetadata;
   +use Drupal\Core\Render\Element;
    use Drupal\Core\Render\Element\Link;
   +use Drupal\Core\Render\HtmlResponseAttachmentsProcessor;
    use Drupal\Core\Render\Markup;
    use Drupal\Core\StringTranslation\TranslatableMarkup;
   -use Drupal\Core\Render\BubbleableMetadata;
   -use Drupal\Core\Render\Element;
   

   All of this is unrelated and makes the patch harder to read. Maybe this should be spun off to a novice CS follow-up?

3. +++ b/core/lib/Drupal/Component/Utility/Environment.php
   @@ -37,4 +37,68 @@ public static function checkMemoryLimit($required, $memory_limit = NULL) {
   +   * because it could be disabled by the server administrator. We also hide all
   +   * the errors that could occur when calling set_time_limit(), because it is
   +   * not possible to reliably ensure that PHP or a security extension will not
   +   * issue a warning/error if they prevent the use of this function.
   

   This comment is no longer true given #5. We no longer use @set_time_limit() and we are returning FALSE if the function doesn't exist or if it fails.

4. +++ b/core/lib/Drupal/Component/Utility/Environment.php
   @@ -37,4 +37,68 @@ public static function checkMemoryLimit($required, $memory_limit = NULL) {
   +   *   An integer specifying the new time limit, in seconds. A value of 0
   +   *   indicates unlimited execution time.
   

   Maybe just "An integer time limit in seconds, or 0 for unlimited execution time." ? That'd probably fit on a single line.

5. +++ b/core/lib/Drupal/Component/Utility/Environment.php
   @@ -37,4 +37,68 @@ public static function checkMemoryLimit($required, $memory_limit = NULL) {
   +   * @return bool
   +   *   Returns TRUE on success, or FALSE on failure.
   

   Maybe just: "Whether set_time_limit() was successful or not." ?

6. +++ b/core/lib/Drupal/Component/Utility/Environment.php
   @@ -37,4 +37,68 @@ public static function checkMemoryLimit($required, $memory_limit = NULL) {
   +   *   A file size limit in bytes based on the PHP upload_max_filesize and
   +   *   post_max_size
   

   Missing " settings." at the end.

7. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -3,16 +3,17 @@
   -use Drupal\Core\Queue\QueueWorkerManagerInterface;
   -use Drupal\Core\Queue\RequeueException;
   -use Drupal\Core\State\StateInterface;
    use Drupal\Core\Lock\LockBackendInterface;
    use Drupal\Core\Queue\QueueFactory;
   -use Drupal\Core\Session\AnonymousUserSession;
   -use Drupal\Core\Session\AccountSwitcherInterface;
   +use Drupal\Core\Queue\QueueWorkerManagerInterface;
   +use Drupal\Core\Queue\RequeueException;
    use Drupal\Core\Queue\SuspendQueueException;
   +use Drupal\Core\Session\AccountSwitcherInterface;
   +use Drupal\Core\Session\AnonymousUserSession;
   +use Drupal\Core\State\StateInterface;
   

   More noise potentially for a follow-up.

8. +++ b/core/tests/Drupal/KernelTests/Core/Common/LegacyFunctionsTest.php
   @@ -23,4 +23,11 @@ public function testFormatDate() {
   +  /**
   +   * @expectedDeprecation drupal_set_time_limit() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Environment::setTimeLimit() instead. See https://www.drupal.org/node/3000058.
   +   */
   

   Doesn't this want a 1-liner before the @expectedDeprecation message?

9. +++ b/core/tests/Drupal/KernelTests/Core/File/FileSystemDeprecationTest.php
   @@ -90,4 +90,11 @@ public function testDeprecatedFileCreate() {
   +  /**
   +   * @expectedDeprecation file_upload_max_size() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Component\Utility\Environment::getUploadMaxSize() instead. See https://www.drupal.org/node/3000058.
   +   */
   

   And here.

Maybe also NW to consider #25 which seems like a legit request.

Thanks!
-Derek

p.s. In other deprecation-related issues I've seen, we seem to rip out the full docblock for the deprecated function and leave the bare-bones: 1-liner, @params and @return. But the gory details are purged. *shrug*

1. The patch is only 18K so I'd be surprised if that is difficult for committers to review. It might make more sense for them to split it. I'm not sure?
2. I don't think alphabetically re-ordering the imports is enough of a reason for a separate issue.
3. This change was discussed by @andypost in #5. I've removed the comments about handling errors. Generally I try to keep the comments the same from the original code where possible.
4. Fixed
5. Fixed
6. Fixed
7. See point 2.
8. None of the deprecation issues I've worked on required a comment above the @expectedDeprecation line. (See \Drupal\KernelTests\Core\File\FileSystemDeprecationTest)
9. Same

Re #27 In the deprecation issues I've worked on, we've kept the full docblock. Not sure whether there is a preferred way?
Re #25 making this an class to be created each time it is used won't necessarily solve your problems. It's unlikely you would want to inject a utility component, and thereby be able to mock it. Instead I would follow the example of FormBuilder which has a protected function getFileUploadMaxSize() method which wraps Environment::getUploadMaxSize() and then mocks it out in FormBuilderTest.

1. Fair enough. Let's see what the core maintainers say.

2. Okay. I've seen folks NW patches for similar, but if the core committers are happy, so am I. ;)

3. Indeed, I linked to #5 in my comment. Was just pointing out we needed to change the comment in this case since we're changing the behavior.

4: Dang, you missed. ;) You "fixed" the wrong comment:

-      // If upload_max_size is less, then reduce. Except if upload_max_size is
-      // zero, which indicates no limit.
+      // An integer time limit in seconds, or 0 for unlimited execution time.

I was talking about the @param in the phpDoc above.

5-6: Thanks!

7: Cool.

8-9: *shrug*. The existing test in core/tests/Drupal/KernelTests/Core/Common/LegacyFunctionsTest.php does this:

  /**
   * Tests format_date().
   *
   * @expectedDeprecation format_date() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal::service('date.formatter')->format() instead. See https://www.drupal.org/node/1876852
   */

Re: #27 dunno. I guess core committers will decide.

Re: #25 Good point! Thanks for addressing.

So, only NW for re-fixing 4. Then this is RTBC to my eyes.

Thanks!
-Derek

4. Ooops! Fixed.

Thanks!

"Checkout Error". Re-running #30

interdiff looks great

Unless the bot objects, this is now RTBC.

Thanks!
-Derek

Note to committers, make sure you check comment #25 and that issue which conflicts with this, before committing this, we should make a decision about whether the Environment class should remain static or not. We could do both issues (deprecate the existing method and add more static methods, but possibly that doesn't make too much sense.

As I wrote there, the patch in the other issue seems a bit over-engineered for the pretty simple thing it does, with lots of methods, but that's just my first impression.

Looking on usage of Environment::checkMemoryLimit() in core it's clear that most of time second parameter is not needed, so refactoring of this method makes sense but other issues states

cannot be unit tested under assumption of unavailable memory.

But current function already do that and I also feel like no reason to make it so complicated just to fix #2583041: GD toolkit & operations should catch \Throwable to fail gracefully in case of errors

Adding tags in relation to #34

Will flag with other FMs

My 2c - creating the class that can be instantiated purely for testing sake (i.e with no concrete use case in production) feels like overkill

Can we call ini_set before the test to simulate the required mocked behaviour?

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -1390,7 +1391,7 @@ protected function buttonWasClicked($element, FormStateInterface &$form_state) {
   protected function getFileUploadMaxSize() {
-    return file_upload_max_size();
+    return Environment::getUploadMaxSize();
   }

This is a wrapper for the procedural function - and so it can be deprecated and the call to it replaced with Environment::getUploadMaxSize()

For me we should postpone #3000050: Replace \Drupal\Component\Utility\Environment::checkMemoryLimit with a PHP memory checking helper class and service on this as this is making concrete changes and deprecating code.

@alexpott we need that in order for FormBuilderTest to stub a getUploadMaxSize() return value.

Re: #37 we can't call ini_set for `post_max_size` or `upload_max_filesize` because that is a PHP_INI_PERDIR mode: "Entry can be set in php.ini, .htaccess, httpd.conf or .user.ini "

Re: #38 -- Indeed, @kim.pepper addresses it in #39, as was already stated at the end of #28. I don't think we should deprecate this. No real harm in leaving it as-is, and it makes testing easier/possible.

Back to RTBC for further consideration.

Thanks,
-Derek

for posterity, here is the mock in tests

/**
   * @covers ::buildForm
   */
  public function testExceededFileSize() {
    $request = new Request([FormBuilderInterface::AJAX_FORM_REQUEST => TRUE]);
    $request_stack = new RequestStack();
    $request_stack->push($request);
    $this->formBuilder = $this->getMockBuilder('\Drupal\Core\Form\FormBuilder')
      ->setConstructorArgs([$this->formValidator, $this->formSubmitter, $this->formCache, $this->moduleHandler, $this->eventDispatcher, $request_stack, $this->classResolver, $this->elementInfo, $this->themeManager, $this->csrfToken])
      ->setMethods(['getFileUploadMaxSize'])
      ->getMock();
    $this->formBuilder->expects($this->once())
      ->method('getFileUploadMaxSize')
      ->willReturn(33554432);

    $form_arg = $this->getMockForm('test_form_id');
    $form_state = new FormState();

    $this->setExpectedException(BrokenPostRequestException::class);
    $this->formBuilder->buildForm($form_arg, $form_state);
  }

Adding issue credit.

Committed 55c96a6 and pushed to 8.7.x. Thanks!

This triggered failures in JSON:API due to new deprecation errors. Fixing at #3036772: Drupal core compatibility: file_upload_max_size() deprecated in Drupal >= 8.7.

Nice work here!

Missed the last call in the run-tests.sh
See #3092268: Replace call of deprecated drupal_set_time_limit() function

Published change record.