Move drupal_check_memory_limit() and parse_size() functionality to components

+++ b/core/tests/Drupal/Tests/Core/Bootstrap/MiscUnitTest.phpundefined
@@ -22,6 +22,12 @@ public static function getInfo() {
+    // @todo think about removing this and converting to components.
+    // Some procedural functions called from within require common.inc.
+    require_once __DIR__ . '/../../../../../includes/common.inc';

which are the functions called? we should convert them prior to switching test to phpunit

drupal_static makes no sense being a component. its only useful in procedural code.
class should use their properties instead

i am not sure parse_size() fits into the bootstrap class.
maybe a Bytes component?

+++ b/core/includes/bootstrap.incundefined
@@ -453,64 +447,11 @@ function config_get_config_directory($type = CONFIG_ACTIVE_DIRECTORY) {
+ *   Drupal\Core\Bootstrap\Bootstrap::overrideServerVariables() directly instead.

@@ -3053,31 +2994,12 @@ function _drupal_shutdown_function() {
+ *   Use Drupal\Core\Bootstrap\Bootstrap::checkMemoryLimit() directly

Nitpicking: This should have a starting "\"

+++ b/core/lib/Drupal/Component/Utility/Bytes.phpundefined
@@ -0,0 +1,43 @@
+ * Definition of Drupal\Component\Bytes\Bytes.

Should be "Contains \..."

+++ b/core/lib/Drupal/Component/Utility/Bytes.phpundefined
@@ -0,0 +1,43 @@
+   * @param $size
+   *   A size expressed as a number of bytes with optional SI or IEC binary unit
...
+   * @return

Let's document being a string/int

+++ b/core/lib/Drupal/Core/Bootstrap/Bootstrap.phpundefined
@@ -0,0 +1,109 @@
+ * Definition of \Drupal\Core\Bootstrap\Bootstrap.

"Contains \"

+++ b/core/lib/Drupal/Core/Bootstrap/Bootstrap.phpundefined
@@ -0,0 +1,109 @@
+   * @param $required
+   *   The memory required for the operation, expressed as a number of bytes with
...
+   * @param $memory_limit

should be string.

+++ b/core/lib/Drupal/Core/Bootstrap/Bootstrap.phpundefined
@@ -0,0 +1,109 @@
+   * @return

that is a bool

+++ b/core/lib/Drupal/Core/Bootstrap/Bootstrap.phpundefined
@@ -0,0 +1,109 @@
+    // Allow the provided URL to override any existing values in $_SERVER.
+    if (isset($variables['url'])) {
+      $url = parse_url($variables['url']);
+      if (isset($url['host'])) {
+        $_SERVER['HTTP_HOST'] = $url['host'];
+      }
+      if (isset($url['path'])) {
+        $_SERVER['SCRIPT_NAME'] = $url['path'];
+      }
+      unset($variables['url']);
+    }
+    // Define default values for $_SERVER keys. These will be used if $_SERVER
+    // does not already define them and no other values are passed in to this
+    // function.
+    $defaults = array(
+      'HTTP_HOST' => 'localhost',
+      'SCRIPT_NAME' => NULL,
+      'REMOTE_ADDR' => '127.0.0.1',
+      'REQUEST_METHOD' => 'GET',
+      'SERVER_NAME' => NULL,
+      'SERVER_SOFTWARE' => NULL,
+      'HTTP_USER_AGENT' => NULL,
+    );
+    // Replace elements of the $_SERVER array, as appropriate.
+    $_SERVER = $variables + $_SERVER + $defaults;

It seems to be that we should at least have a new todo which tells that $_SERVER should be entirely replaced by a symfony request object.

+++ b/core/tests/Drupal/Tests/Core/Bootstrap/OverrideServerVariablesUnitTest.phpundefined
@@ -0,0 +1,73 @@
+ * Definition of Drupal\system\Tests\Bootstrap\OverrideServerVariablesUnitTest.

Contains

+++ b/core/tests/Drupal/Tests/Core/Bootstrap/OverrideServerVariablesUnitTest.phpundefined
@@ -0,0 +1,73 @@
+  function testDrupalOverrideServerVariablesProvidedURL($url, $expected_server_values) {

it seems to make more sense to be on a single BootstrapTest test? At least this should be a public method.

+++ b/core/lib/Drupal/Component/Utility/Bytes.phpundefined
@@ -0,0 +1,43 @@
+  const KILOBYTE = 1024;

yay, this makes a lot more sense being here:)

+++ b/core/lib/Drupal/Core/Bootstrap/Bootstrap.phpundefined
@@ -0,0 +1,111 @@
+   * @param $variables

@param array

+++ b/core/tests/Drupal/Tests/Core/Bootstrap/MiscUnitTest.phpundefined
@@ -5,14 +5,15 @@
  * Definition of Drupal\system\Tests\Bootstrap\MiscUnitTest.

file is moved so this should changed right? it should also be Contains \..

+++ b/core/tests/Drupal/Tests/Core/Bootstrap/MiscUnitTest.phpundefined
@@ -5,14 +5,15 @@
-class MiscUnitTest extends UnitTestBase {
+class MiscUnitTest extends UnitTestCase {

Actually what you think about just moving the whole thing to BootstrapTest, since they are testing the same class?

+++ b/core/tests/Drupal/Tests/Core/Bootstrap/MiscUnitTest.phpundefined
@@ -23,24 +24,24 @@ public static function getInfo() {
   function testCheckMemoryLimit() {

lets add public visibility, since we are here

Just some small bits.

As we still have the old methods this seriously does not change the API.

+++ b/core/tests/Drupal/Tests/Core/Bootstrap/BootstrapTest.phpundefined
@@ -0,0 +1,97 @@
+    $memory_limit = ini_get('memory_limit');
+    // Test that a very reasonable amount of memory is available.
+    $this->assertTrue(Bootstrap::checkMemoryLimit('30MB'), '30MB of memory tested available.');
+
+    // Get the available memory and multiply it by two to make it unreasonably
+    // high.
+    $twice_avail_memory = ($memory_limit * 2) . 'MB';
+    // The function should always return true if the memory limit is set to -1.
+    $this->assertTrue(Bootstrap::checkMemoryLimit($twice_avail_memory, -1), 'Bootstrap::checkMemoryLimit() returns TRUE when a limit of -1 (none) is supplied');
+
+    // Test that even though we have 30MB of memory available - the function
+    // returns FALSE when given an upper limit for how much memory can be used.
+    $this->assertFalse(Bootstrap::checkMemoryLimit('30MB', '16MB'), 'Bootstrap::checkMemoryLimit() returns FALSE with a 16MB upper limit on a 30MB requirement.');
+
+    // Test that an equal amount of memory to the amount requested returns TRUE.
+    $this->assertTrue(Bootstrap::checkMemoryLimit('30MB', '30MB'), 'Bootstrap::checkMemoryLimit() returns TRUE when requesting 30MB on a 30MB requirement.');

Couldn't this also be using a dataprovider, as it just calls the same methods all the time.

+++ b/core/tests/Drupal/Tests/Core/Bootstrap/BootstrapTest.phpundefined
@@ -0,0 +1,97 @@
+  /**
+   * Provide data for self::testDrupalOverrideServerVariablesProvideURL().
+   */
+  public function providerTestOverrideServerVariables() {

Should be @return array

an additional nit:

+++ b/core/tests/Drupal/Tests/Core/Bootstrap/BootstrapTest.phpundefined
@@ -0,0 +1,97 @@
+  function testCheckMemoryLimit() {

needs visibility

And yeah the API does not change here, since we keep the procedural functions, so there shouldnt be a problem

Great!

Considering we're changing very low level Drupal and code that runs on every request I think we should be profiling this change...

Tagtastic.... What else will be part of the Bootstrap class? Do we have some follow ups?

I don't think this actually really needs profiling as it's pretty much just moving the function calls to a class. But profiling never hurts! Thanks a lot, guys. This looks great.

Not sure about the status of this idea, but the patch doesn't apply currently.

Rerolled, what else do we need to do here? I think I agree that this doesn't need profiling, but don't mind.

.

Looks pretty good to me. If we introduce a nice issue summary, I'd vote this pretty much RTBC.

i think it allso needs profiling.
also a reroll:/

Back to RTBC.

Patch no longer applies.

Applies and passes. RTBC other than missing @groups.

Just on the new PHPUnit test class.

Sadly this issue is still tagged as needs profiling.

Woot groups!

The reason you need the tests is so you can have a behavioral baseline for when you make it go faster.

Patch no longer applies.

rerolled.

Back to RTBC.

Patch no longer applies.

Rerolled.

Could we check whether this triggers the raw composer autoloader before the APC loader is available? Apart from that I don't think this needs profiling but it'd be good to check that.

The patch doesn't change drupal_classloader() so whatever behavior is there will be preserved. Far as I can tell.

54: 2003800-54.patch queued for re-testing.

Rerolled, patch no longer applied.

1. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -0,0 +1,43 @@
   +   * @return integer
   
   +++ b/core/lib/Drupal/Core/Bootstrap/Bootstrap.php
   @@ -0,0 +1,111 @@
   +   * @param $memory_limit
   ...
   +   * @return boolean
   

   We use "int", "string", and "bool" instead.

2. +++ b/core/lib/Drupal/Core/Bootstrap/Bootstrap.php
   @@ -0,0 +1,111 @@
   +        $_SERVER['HTTP_HOST'] = $url['host'];
   

   You need to rerole that for #1998638: Replace almost all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object

3. +++ b/core/tests/Drupal/Tests/Core/Bootstrap/BootstrapTest.php
   @@ -0,0 +1,141 @@
   +class BootstrapTest extends UnitTestCase {
   +  public static function getInfo() {
   

   All those unit tests are such great to look at. Let's use an empty line here and one {@inheritdoc}

Reroll - I tested the patch but unfortunately the one on the PHPUnit test failed.

The BootstrapTest->testCheckMemoryLimit() fails searching for Bytes.php file.

This is because the patch applies the Bytes.php file was applied to the wrong location of the core. Patch applies the Bytes.php to Drupal/Tests/Component/Utility instead of Drupal/Component/Utility

1. +++ b/core/includes/bootstrap.inc
   @@ -1,3 +1,8 @@
   + * @deprecated as of Drupal 8.0.
   + *   Use \Drupal\Core\Bootstrap\Bootstrap::checkMemoryLimit() directly
   + *   instead.
   +function drupal_check_memory_limit($required, $memory_limit = NULL) {
   +  return Bootstrap::checkMemoryLimit($required, $memory_limit);
    <?php
   

   Guessing this shouldn't be here :)

2. +++ b/core/lib/Drupal/Component/Utility/Bytes.php
   @@ -0,0 +1,43 @@
   + * Contains \Drupal\Component\Bytes\Bytes.
   

   This should be Utility\Bytes, not Bytes\Bytes

I have done a re-roll on this issues and found no errors.

Trying to sort out the status here, so did some interdiffs. and a txt file showing the commands I did to make them. (there are lots of ways to do it, just showing one)

uploading 74 again, to get the bot to run on it.

I haven't done a full review of the reroll to see if it's right..

looks like the reroll emptied files, then 74 removed the emptied files. ok.
72 had Bytes.php not in the place it was in 67, so 74 moved it back to the right spot. ok.
67 had done the things i see in the interdiff of 72 and 74: removing docs on a deprecated function, and changing drupal_check_memory_limit. so that is ok for the reroll, cause we want to get a patch that is doing what 67 was. And then we want to review it. ...

but noting here, before I forget: that 67 probably should not have removed the docs, it should have just added the deprecated doc. but also, there is a discussion about deprecating... in #2158871: [Policy, no patch] Clearly denote when @deprecated code is slated to be removed Could use another opinion on that, but this might be needs work to put the docs back in, and maybe some other things.

so. I went to look at the 74 I reuploaded, and it looked like it still had the wrong doc for Bytes.php, this really confused me, cause it was right in the file... then I realized that the .diff for 74 had a history of commits.

This one is made with just a
git diff 8.x

talked with @steeloctopus and they didn't have more work to post at this time, so un assigning. No worries.

Patch does not apply anymore to head. First needs a reroll and after that I suggest exploring the following options:

1. Instead of introducing a (somewhat poorly named) Bootstrap utility component, consider extracting drupal_override_server_variables to a Cli or Script utility component. Later on we could move drupal_is_cli and probably other script-related functions over there.
2. Instead of introducing a Bytes utility component, consider extracting drupal_check_memory_limit and parse_size into a Memory utility component.

When rerolling, please do not incorporate additional changes. And also when making changes supply a proper interdiff. Otherwise it can get tedious to keep up with the development of a patch. Thank you @YesCT for puzzling together the pieces.

Not able to work on reroll.. patch not applying...

Well, we need a reroll because the patch is not applying anymore. A comprehensive guide on how to reroll patches and resolve merge conflicts is available here.

will take a shot on this and thanks @znerol for the link

here's the reroll. hopefully it works as planned.

• The addition of use Drupal\Component\Utility\Url in bootsrap.inc looks suspicious. Probably it this was not intended.
• drupal_override_server_variables() and OverrideServerVariablesUnitTest was removed in the meantime (see #2213643: Remove dysfunctional drupal_override_server_variables()).
• It seems that the hunk in color.module was lost during the reroll. Replace the call to parse_size in color_scheme_form_submit with Bytes::parseSize

a new reroll baseed on #79 and help from @YesCT

The reroll looks great so far. Comparing to #79 I see two unrelated changes which makes it more difficult to actually diff the patches:

1. +++ b/core/includes/bootstrap.inc
   @@ -337,7 +332,9 @@ function config_get_config_directory($type = CONFIG_ACTIVE_DIRECTORY) {
      if (!empty($config_directories[$type])) {
        return $config_directories[$type];
      }
   +
      throw new \Exception(format_string('The configuration directory type %type does not exist.', array('%type' => $type)));
   +
    }
   

   Do not change unrelated code, it makes diffing more difficult. Those two newlines are not there in HEAD.

2. +++ b/core/modules/color/color.module
   @@ -363,6 +364,7 @@ function color_scheme_form_submit($form, &$form_state) {
      }
    
      // Make sure enough memory is available.
   +
      if (isset($info['base_image'])) {
        // Fetch source image dimensions.
        $source = drupal_get_path('theme', $theme) . '/' . $info['base_image'];
   @@ -377,7 +379,7 @@ function color_scheme_form_submit($form, &$form_state) {
   

   Same here.

sorry @znerol discovered that after ul the patch but didn't have time to get here and tell you that.

Be careful with hand-editing patches :) I bet this will not apply (there is now a hunk without any changes in color_scheme_form_submit). While rereading I also noted that Bootstrap::overrideServerVariables can be removed from the patch (see #2213643: Remove dysfunctional drupal_override_server_variables()).

removed

about the Bootstrap::overrideServerVariables, haven't removed it yet, I guess that would be in boostrap module but is that really a part of this issue? Shouldn't there be a new issue for that?

This one will need to have another reroll.

93: system-bootstrap-phpunit-2003800-92.patch queued for re-testing.

just making sure it needs a reroll

ok so made a reroll based on patch in #93.

I never got an reply from znerol about the overrideServerVariables but I went along and removed it and also its tests.

Sorry for the silence. The removal overrideServerVariables from the patch is certainly correct, this code simply does not exist anymore in Drupal 8. The reroll looks almost perfect. I've noticed the following things while comparing #97 to #79:

1. +++ b/core/includes/bootstrap.inc
   @@ -336,7 +331,7 @@ function config_get_config_directory($type = CONFIG_ACTIVE_DIRECTORY) {
   -  throw new \Exception(format_string('The configuration directory type %type does not exist.', array('%type' => $type)));
   +  throw new Exception(format_string('The configuration directory type %type does not exist.', array('%type' => $type)));
   

   This looks like an accidental change.

2. +++ b/core/modules/file/lib/Drupal/file/Plugin/Field/FieldType/FileItem.php
   @@ -7,6 +7,8 @@
   +use Drupal\Core\Field\FieldDefinitionInterface;
   

   This looks unrelated too.

3. +++ b/core/modules/color/color.module
   @@ -4,8 +4,9 @@
   -use Drupal\Core\Asset\CssOptimizer;
   +use Drupal\Component\Utility\Bytes;
    use Drupal\Component\Utility\String;
   +use Drupal\Core\Asset\CssOptimizer;
   

   I'm not sure whether we have coding standards about the order of use statements. That said, I'd suggest not moving existing lines around if there is no good reason to do so.

I wouldn't view these methods as "Bootstrap." They exist in bootstrap.inc not because they are explicitly part of the bootstrap but because they where needed during the bootstrap process. Once they move to classes they can be loaded at anytime. As such I would do 2 things.

1) Look at the naming again
2) Not name the test "bootstrap" a build individual tests for the classes you are testing.

@neclimdul: I fully agree, see #80. The renaming should be based on a proper reroll, though.

Hi

comments on #99

1. I'm a bit surprised cause the throw exception was something I had removed I though in #93 which it obviously weren't.

2. I wasn't sure so wanted feedback, thanks it should be removed now (when next patch is coming)

3. I was adviced to do so but I did read through the coding standards doc and couldn't find anything about so I kept it as is.

Since there were stuff already wrong in patch #88 that were still in latest patch I simply rerolled this based on #79 instead and hopefully I got it correct this time.

The removal of functions as discussed isn't removed in this due to that it doesn't seems to be totally clear on what to do based on comment #100 and #101.

Oy. This issue should be split into multiple. The current patch touches tons of functional code. Those changes are completely unrelated to tests.

I'm just going to make sure that the patch is rerolled correctly and not anything more. #102 is very wrong so just ignore that one..

reroll based on #79

bootstrap.inc: use statement think there are about 11 to many now..

The Boostrap.php file with overrideServervariables and checkmemory isn't removed here..just wanted to make sure first to get a clean reroll. When we get that we can begin to remove or not in next patch or new issue..but first a clean reroll which I'm hopefully closer to now.

I'm rerolling 79 also so we can compare. I'll take notes while I do this. Please read them and ask questions if there is something about resolving conflicts that is confusing.

----------------------------
conflict 1
core/modules/file/lib/Drupal/file/Plugin/Field/FieldType/FileItem.php

<<<<<<< HEAD
use Drupal\Core\Field\FieldStorageDefinitionInterface;
=======
use Drupal\Component\Utility\Bytes;
use Drupal\Core\Field\FieldDefinitionInterface;
>>>>>>> 79 on feb 3
use Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem;
use Drupal\Core\TypedData\DataDefinition;

when looking at the *patch*

 
 namespace Drupal\file\Plugin\Field\FieldType;
 
+use Drupal\Component\Utility\Bytes;
 use Drupal\Core\Field\FieldDefinitionInterface;
 use Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem;
 use Drupal\Core\TypedData\DataDefinition;

can see that the only thing this patch was doing is adding Bytes.
use Drupal\Core\Field\FieldDefinitionInterface; is a context line in the patch, the patch was not adding it, so we do not add it.

So we keep the new stuff in head, and re-do what the patch was trying to do.
We can put Bytes in alpha order without reordering any other use statements.

conflict resolved by:

use Drupal\Component\Utility\Bytes;
use Drupal\Core\Field\FieldStorageDefinitionInterface;
use Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem;
use Drupal\Core\TypedData\DataDefinition;

------------------------------------------------------
conflict 2
core/modules/color/color.module

use Drupal\Core\Asset\CssOptimizer;
<<<<<<< HEAD
use Drupal\Component\Utility\String;
=======
use Drupal\Component\Utility\Bytes;
>>>>>>> 79 on feb 3

/**

looking at the patch:

 
 use Drupal\Core\Asset\CssOptimizer;
+use Drupal\Component\Utility\Bytes;
 
 /**

the patch was just adding the use for Bytes

so we just do that, head has in it Utility/String, so we should leave that there and just add Bytes. (here we cannot insert in alpha order, so just do the best we can and dont worry about it)

I resolved it like:

use Drupal\Core\Asset\CssOptimizer;
use Drupal\Component\Utility\Bytes;
use Drupal\Component\Utility\String;

/**

before moving on to another file, check if there are other conflicts in this file, there are.
-----------------------------------------------
conflict 3
core/modules/color/color.module

  // Make sure enough memory is available.
<<<<<<< HEAD
  if (isset($info['base_image'])) {
    // Fetch source image dimensions.
    $source = drupal_get_path('theme', $theme) . '/' . $info['base_image'];
    list($width, $height) = getimagesize($source);

    // We need at least a copy of the source and a target buffer of the same
    // size (both at 32bpp).
    $required = $width * $height * 8;
    // We intend to prevent color scheme changes if there isn't enough memory
    // available.  memory_get_usage(TRUE) returns a more accurate number than
    // memory_get_usage(), therefore we won't inadvertently reject a color
    // scheme change based on a faulty memory calculation.
    $usage = memory_get_usage(TRUE);
    $memory_limit = ini_get('memory_limit');
    $size = parse_size($memory_limit);
    if (!drupal_check_memory_limit($usage + $required, $memory_limit)) {
      drupal_set_message(t('There is not enough memory available to PHP to change this theme\'s color scheme. You need at least %size more. Check the <a href="@url">PHP documentation</a> for more information.', array('%size' => format_size($usage + $required - $size), '@url' => 'http://www.php.net/manual/ini.core.php#ini.sect.resource-limits')), 'error');
      return;
    }
=======
  // Fetch source image dimensions.
  $source = drupal_get_path('theme', $theme) . '/' . $info['base_image'];
  list($width, $height) = getimagesize($source);

  // We need at least a copy of the source and a target buffer of the same
  // size (both at 32bpp).
  $required = $width * $height * 8;
  // We intend to prevent color scheme changes if there isn't enough memory
  // available.  memory_get_usage(TRUE) returns a more accurate number than
  // memory_get_usage(), therefore we won't inadvertently reject a color
  // scheme change based on a faulty memory calculation.
  $usage = memory_get_usage(TRUE);
  $memory_limit = ini_get('memory_limit');
  $size = Bytes::parseSize($memory_limit);
  if (!drupal_check_memory_limit($usage + $required, $memory_limit)) {
    drupal_set_message(t('There is not enough memory available to PHP to change this theme\'s color scheme. You need at least %size more. Check the <a href="@url">PHP documentation</a> for more information.', array('%size' => format_size($usage + $required - $size), '@url' => 'http://www.php.net/manual/ini.core.php#ini.sect.resource-limits')), 'error');
    return;
>>>>>>> 79 on feb 3
  }

this looks like a big difference, but when checking the patch:

   // scheme change based on a faulty memory calculation.
   $usage = memory_get_usage(TRUE);
   $memory_limit = ini_get('memory_limit');
-  $size = parse_size($memory_limit);
+  $size = Bytes::parseSize($memory_limit);
   if (!drupal_check_memory_limit($usage + $required, $memory_limit)) {
     drupal_set_message(t('There is not enough memory available to PHP to change this theme\'s color scheme. You need at least %size more. Check the <a href="@url">PHP documentation</a> for more information.', array('%size' => format_size($usage + $required - $size), '@url' => 'http://www.php.net/manual/ini.core.php#ini.sect.resource-limits')), 'error');
     return;

we see the only thing it was doing was changing the like with parse_size. the large "conflict" is because some other issue added an if and indented the code in the if. no big deal.

So we keep what is in head, and just redo that change.

which makes it resolved like:

  if (isset($info['base_image'])) {
    // Fetch source image dimensions.
    $source = drupal_get_path('theme', $theme) . '/' . $info['base_image'];
    list($width, $height) = getimagesize($source);

    // We need at least a copy of the source and a target buffer of the same
    // size (both at 32bpp).
    $required = $width * $height * 8;
    // We intend to prevent color scheme changes if there isn't enough memory
    // available.  memory_get_usage(TRUE) returns a more accurate number than
    // memory_get_usage(), therefore we won't inadvertently reject a color
    // scheme change based on a faulty memory calculation.
    $usage = memory_get_usage(TRUE);
    $memory_limit = ini_get('memory_limit');
    $size = Bytes::parseSize($memory_limit);
    if (!drupal_check_memory_limit($usage + $required, $memory_limit)) {
      drupal_set_message(t('There is not enough memory available to PHP to change this theme\'s color scheme. You need at least %size more. Check the <a href="@url">PHP documentation</a> for more information.', array('%size' => format_size($usage + $required - $size), '@url' => 'http://www.php.net/manual/ini.core.php#ini.sect.resource-limits')), 'error');
      return;
    }

-------------------------------------
conflict 4
core/includes/common.inc

<<<<<<< HEAD
/**
 * @file
 * Common functions that many Drupal modules will need to reference.
 *
 * The functions that are critical and need to be available even when serving
 * a cached page are instead located in bootstrap.inc.
 */

use Drupal\Component\Serialization\Json;
use Drupal\Component\Serialization\Yaml;
use Drupal\Component\Serialization\Exception\InvalidDataTypeException;
use Drupal\Component\Utility\Crypt;
=======
use Drupal\Component\Utility\Bytes;
use Drupal\Component\Utility\Crypt;
use Drupal\Component\Utility\Json;
>>>>>>> 79 on feb 3
use Drupal\Component\Utility\Number;
use Drupal\Component\Utility\SortArray;

patch was:

 <?php
 
+use Drupal\Component\Utility\Bytes;
+use Drupal\Component\Utility\Crypt;
 use Drupal\Component\Utility\Json;
 use Drupal\Component\Utility\Number;

so, since #79, some other issue added the @file doc block and changed the use's, and some other issue put in Crypt. fine.
this patch was just adding Bytes and Crypt. Crypt is already added, so keeping head and adding Bytes is all we do.

resolved like:

/**
 * @file
 * Common functions that many Drupal modules will need to reference.
 *
 * The functions that are critical and need to be available even when serving
 * a cached page are instead located in bootstrap.inc.
 */

use Drupal\Component\Serialization\Json;
use Drupal\Component\Serialization\Yaml;
use Drupal\Component\Serialization\Exception\InvalidDataTypeException;
use Drupal\Component\Utility\Crypt;
use Drupal\Component\Utility\Bytes;
use Drupal\Component\Utility\Number;
use Drupal\Component\Utility\SortArray;

check for other conflicts in this file.. none. moving on.

-----------------------------------
conflict 5
core/includes/bootstrap.inc

use Drupal\Component\Utility\Timer;
use Drupal\Component\Utility\Unicode;
<<<<<<< HEAD
use Drupal\Component\Utility\UrlHelper;
=======
use Drupal\Component\Utility\Url;
use Drupal\Component\Utility\UrlValidator;
use Drupal\Core\Bootstrap\Bootstrap;
>>>>>>> 79 on feb 3
use Drupal\Core\DrupalKernel;
use Drupal\Core\Database\Database;

patch was

 use Drupal\Component\Utility\Timer;
 use Drupal\Component\Utility\Unicode;
 use Drupal\Component\Utility\Url;
+use Drupal\Component\Utility\UrlValidator;
+use Drupal\Core\Bootstrap\Bootstrap;
 use Drupal\Core\DrupalKernel;
 use Drupal\Core\Database\Database;

just adding UrlValidator and Bootstrap.

the Url was just a context line in the patch that confused git.

so resolved like:

use Drupal\Component\Utility\Timer;
use Drupal\Component\Utility\Unicode;
use Drupal\Component\Utility\UrlHelper;
use Drupal\Component\Utility\UrlValidator;
use Drupal\Core\Bootstrap\Bootstrap;
use Drupal\Core\DrupalKernel;
use Drupal\Core\Database\Database;

check for more conflicts.. yep.

--------------------------------------
conflict 6
core/includes/bootstrap.inc

  if (!empty($config_directories[$type])) {
    return $config_directories[$type];
  }
<<<<<<< HEAD
  throw new \Exception(format_string('The configuration directory type %type does not exist.', array('%type' => $type)));
=======
  throw new Exception(format_string('The configuration directory type %type does not exist.', array('%type' => $type)));
}

/**
 * Sets appropriate server variables needed for command line scripts to work.
 *
 * @param $variables
 *   (optional) An associative array of variables within
 *   \Drupal::request()->server that should be replaced. If the special element
 *   'url' is provided in this array, it will be used to populate some of the
 *   server defaults; it should be set to the URL of the current page request,
 *   excluding any GET request but including the script name
 *   (e.g., http://www.example.com/mysite/index.php).
 *
 * @see conf_path()
 * @see request_uri()
 * @see \Symfony\Component\HttpFoundation\Request::getClientIP()
 *
 * @deprecated as of Drupal 8.0. Use
 *   Drupal\Core\Bootstrap\Bootstrap::overrideServerVariables() directly instead.
 */
function drupal_override_server_variables($variables = array()) {
  Bootstrap::overrideServerVariables(\Drupal::request(), $variables);
>>>>>>> 79 on feb 3
}

patch was taking out some docs from drupal_override_server_variables()

 /**
  * Sets appropriate server variables needed for command line scripts to work.
  *
- * This function can be called by command line scripts before bootstrapping
- * Drupal, to ensure that the page loads with the desired server parameters.
- * This is because many parts of Drupal assume that they are running in a web
- * browser and therefore use information from the global PHP $_SERVER variable
- * that does not get set when Drupal is run from the command line.
- *
- * In many cases, the default way in which this function populates the $_SERVER
- * variable is sufficient, and it can therefore be called without passing in
- * any input. However, command line scripts running on a multisite installation
- * (or on any installation that has settings.php stored somewhere other than
- * the sites/default folder) need to pass in the URL of the site to allow
- * Drupal to detect the correct location of the settings.php file. Passing in
- * the 'url' parameter is also required for functions like request_uri() to
- * return the expected values.
- *
- * Most other parameters do not need to be passed in, but may be necessary in
- * some cases; for example, if \Drupal::request()->getClientIP()
- * needs to return anything but the standard localhost value ('127.0.0.1'),
- * the command line script should pass in the desired value via the
- * 'REMOTE_ADDR' key.
- *
  * @param $variables
  *   (optional) An associative array of variables within
  *   \Drupal::request()->server that should be replaced. If the special element

but that function is not in head anymore.
so git put back the context lines, and took out the lines it though it should. we should not put them back.
And the patch was not touching the Exception line there, so we should not change that in head either.

See can find if those docs got moved somewhere...
git log -S "Sets appropriate server variables needed for" core/includes
#2213643: Remove dysfunctional drupal_override_server_variables()
Read that patch. The lines are removed, not moved.
so, we know it wasn't moved, it was removed. And that part of our patch has nothing to change, since that function is gone.

so resolved by *just* keeping what is in head and nothing from this hunk.

  if (!empty($config_directories[$type])) {
    return $config_directories[$type];
  }
  throw new \Exception(format_string('The configuration directory type %type does not exist.', array('%type' => $type)));
}

/**
 * Initializes the PHP environment.
 */
function drupal_environment_initialize() {

no more conflicts in this file, and no more conflicts from the rebase,
so added the changes and did a rebase --continue, and made a new patch based on git diff 8.x

=============

I did a diff of patch 106 against this 107.

only real difference was patch in 106 did:

< -  throw new \Exception(format_string('The configuration directory type %type does not exist.', array('%type' => $type)));
< +  throw new Exception(format_string('The configuration directory type %type does not exist.', array('%type' => $type)));

Which is good. We mostly agreed on how to resolve the conflicts. :)

-----------------------------

so I think we should use this 107 as the reroll.

=========================

Now, there is some valid feedback that this approach may not be the correct one.
I think we should as quick as possible, really look at this and see what needs to be done.

Hi

sorry for making this a tremendous idiotic issue but as I said to znerol I did this to learn and whenever anyone felt we needed to get going it was just to override me and so it's done :)

As you may understand I have done this several times without ul to see what happends and the only thing that do confuses me is this line that actually differs 106 and 107. Simpy because and one try it's not there and next one there is. very confusing.

Well next time I need to reroll it will most likely be quicker.

@filijonka Yes. I think there is a lot to learn about rerolling and conflicts, but I think you have got it now. :)

I think the next steps here are to look at comments #80 by @znerol, #100 by @neclimdul and #103 by @sun

and it needs profiling.

https://drupal.org/contributor-tasks/profiling
and
https://drupal.org/profiling

might help.

But also, exactly *what* needs profiling?
Installing drupal?
Can we link to another issue that had similar profiling done on it so we see more about how to do it?

changing to needs work to try doing some of those.

just a thought I got here in the evening that perhaps it's not really useful to do profiling before we actually got #80, #100, #103 cleared out. Also looking through quickly the other childissues of the same parent and couldn't find any profiling tests of them.

Maybe first step would be to remove the overrideServerVariables that is right now added in this patch and +the test?

Second would be to decide whether the class bootstrap is necessary at all, see #2016629: Refactor bootstrap to better utilize the kernel and either open anew issue for that or the named issue should take care of that?

Well that was just some thoughts.

for easier reading I hidden the non interesting patches bettwen 79 and 107.

Sorry, my previous comment was terribly confused by the "bootstrap" keyword that happens to be contained in the code in this patch.

Here are some suggestions to mitigate that and improve the conversion in general:

1. +++ b/core/lib/Drupal/Core/Bootstrap/Bootstrap.php
   @@ -0,0 +1,120 @@
   + * Contains \Drupal\Core\Bootstrap\Bootstrap.
   + */
   +namespace Drupal\Core\Bootstrap;
   ...
   +class Bootstrap {
   

   Let's move + rename this class into

   Drupal\Component\Utility\Memory

   please :)

   Even better: MemoryHelper

2. +++ b/core/lib/Drupal/Core/Bootstrap/Bootstrap.php
   @@ -0,0 +1,120 @@
   +use Symfony\Component\HttpFoundation\Request;
   ...
   +  public static function overrideServerVariables(Request $request, $variables = array()) {
   

   This function/method is gone and obsolete, and thus can be removed entirely from this patch.

3. +++ b/core/modules/color/color.module
   @@ -377,7 +378,7 @@ function color_scheme_form_submit($form, &$form_state) {
   -    $size = parse_size($memory_limit);
   +    $size = Bytes::parseSize($memory_limit);
   

   A parseSize() method on a Bytes utility class is not really self-descriptive; i.e.:

   - it's not clear what the input value is

   - it's not clear what the output value is.

   After reading up the functional code, how about this?

   Bytes::toInt()

   e.g.:

   $bytes = Bytes::toInt('5MB');
   

   ...seems more accurate + clear to me?

@YesCT (Re #107): The reroll is almost accurate, however there is an important detail missing. The problem with the systematic approach driven by solely resolving the conflicts and not touching anything else is the following: #2213643: Remove dysfunctional drupal_override_server_variables() removed a function completely from head, therefore it also needs to be removed completely from this patch. Though #107 correctly resolves the conflict in bootstrap.inc, it fails to reproduce the changes from #2213643 in other places (where there are no conflicts). Therefore in addition the following pieces must be removed from the patch in order to produce a correct reroll (otherwise the function and tests will be reintroduced into core and thus revert parts of #2213643): Bootstrap::overrideServerVariables(), BootstrapTest::testOverrideServerVariables and providerTestOverrideServerVariables.

I realize that this reroll is not trivial and therefore I provide a reroll which I think is correct, such that the real work on this issue can continue.

Still needs work (per #112), but also needs feedback from testbot.

I was going to write additional thoughts of @suns comment but it took me some time to understand what's going on in the color.module , function color_scheme_form_submit we have now have with the latest patch:

  if (isset($info['base_image'])) {
    ......
    $memory_limit = ini_get('memory_limit');
    $size = Bytes::parseSize($memory_limit);
    if (!drupal_check_memory_limit($usage + $required, $memory_limit)) {
      drupal_set_message(t('There is not enough memory available to PHP to change this theme\'s color scheme. You need at least %size more. Check the <a href="@url">PHP documentation</a> for more information.', array('%size' => format_size($usage + $required - $size), '@url' => 'http://www.php.net/manual/ini.core.php#ini.sect.resource-limits')), 'error');
      return;
    }
  }

1. $size is only used if the if-statment return true. move that into the if-statement. it' a small thing but more clear.
2. drupal_check_memory is deprecated and we should therefor use the correct way right now.
3. we don't need to have $memory_limit since if not set that is the default value in checkMemoryLimit.
so above code would be

  if (isset($info['base_image'])) {
    ......
    if (!Bootstrap::checkMemoryLimit($usage + $required)) {
      $memory_limit = ini_get('memory_limit');
      $size = Bytes::parseSize($memory_limit);
      drupal_set_message(t('There is not enough memory available to PHP to change this theme\'s color scheme. You need at least %size more. Check the <a href="@url">PHP documentation</a> for more information.', array('%size' => format_size($usage + $required - $size), '@url' => 'http://www.php.net/manual/ini.core.php#ini.sect.resource-limits')), 'error');
      return;
    }
  }

ok so if we create a class MemoryHelper we would have Bootstrap::checkMemoryLimit and Bytes::parsesize moved to that class. so we would have MemoryHelper::checkMemoryLimit and MemoryHelper::toInt instead.

Those the code in color would be

  if (isset($info['base_image'])) {
    ......
    if (!MemoryHelper::checkMemoryLimit($usage + $required)) {
      $memory_limit = ini_get('memory_limit');
      $size = MemoryHelper::toInt($memory_limit);
      drupal_set_message(t('There is not enough memory available to PHP to change this theme\'s color scheme. You need at least %size more. Check the <a href="@url">PHP documentation</a> for more information.', array('%size' => format_size($usage + $required - $size), '@url' => 'http://www.php.net/manual/ini.core.php#ini.sect.resource-limits')), 'error');
      return;
    }
  }

A even nicer solution would be to have toInt function being private and let checkMemoryLimit to return the value used in the drupal_set_message. Not sure it's possible though.

sorry I accidently removed a znerols patch

this is an interdiff from the just-a-strict reroll in 107 to 113. and as @znerol says in 113, work should be based off of 113.

----

(the patch in 116 is exactly the same as 113, and does not have the changes from 115).
@filijonka you can make changes you think are good, but start with 113, and then make the changes. and please provide both a new patch, and an interdiff.

[ For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff | Microbranching workflow: http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-... ]

I can't make sense to merging the Bytes and Memory functionality into a single class.

The Bytes functionality is used in many other contexts unrelated to memory. Just one example, in the File and Image fields, to configure and validate the maximum file upload size.

Likewise, a dedicated Memory[Helper] class could very well be enhanced with additional memory-specific helper methods later on. — That said, we still have some other legacy functions related to PHP environment issues (e.g., drupal_set_time_limit()), so as a perhaps more sensible forward-looking alternative, we could name the class Environment instead of Memory, and consolidate all those PHP environment related helper functions in there.

Hi

just a thought on this issue; maybe it would be better to try to get this rtbc as it is right now and create a new issue for the remaining stuff we are discussing?

In that way we might get more people to have an opinion about the structure, names etc etc

edit:
I'm sorry if I'm messy but had a short chat with @YesCT to get some feedback and one thing was, make a big patch solving the stuff that is discussed and then we can deal with it. making smaller patches of that etc..
So I figured I better get a better grip of this patch and the issue and imho I don't think this patch is doing what it should do. I reviewed and read the summary and based on the #1938068: Convert UnitTestBase to PHPUnit it says that three things should be done:

• If a test is in the system module, and it tests a class not in system module, then it should be moved to core/tests/Drupal/Tests/ directory
• If a class contains Unit in its name, remove it when converting to PHPUnit..there is no point keeping it.
• If a class can be converted to use dataproviders it should

i.e the GetFilenameUnitTest.php isn't following these guidelines as far as I can see, am I wrong?

so step one, review the patch in 116 aka 113 so we know that it at least deals with the issue and then we can do the changes too.
As soon someone has reviewed I'm going to give it a try.

Hrm. We're reaching an insane amount of comments on this issue, which really ought to be a trivial conversion only.

1. Moved Drupal\Core\Bootstrap\Bootstrap into Drupal\Component\Utility\Environment.
2. Renamed Bytes::parseSize() to Bytes::toInt().
3. Added BytesTest.
4. Added missing @covers for EnvironmentTest.

I don't really see how this needs profiling, since there is no functional change here. The two converted helper functions are not and should not be invoked in the critical path, and even if they were, this patch only moves the procedural functions into utility classes.

ok nevermind then.

Patch looks great, just one thing and its ready to go

+++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
@@ -0,0 +1,55 @@
+    $this->assertEquals(Bytes::toInt($size), $expected_int);

+++ b/core/tests/Drupal/Tests/Component/Utility/EnvironmentTest.php
@@ -0,0 +1,85 @@
+    $this->assertEquals($return, $expected, $message);

$expected is the first argument

http://phpunit.de/manual/3.7/en/writing-tests-for-phpunit.html#writing-t...

Fixed assertEquals() argument order + favor self-descriptive phpunit code instead of needless phpDoc.

Oh yes, +1000 for killing our custom messages, thanks!

Sorry, We dont just say "self documenting".

We still need @params.

this doesn't fix those @params on the test,
but it does add back the docs on the deprecated function. (those will be removed when we remove the function)

+++ b/core/lib/Drupal/Component/Utility/Bytes.php
@@ -0,0 +1,47 @@
+   * @param mixed $size
+   *   An integer or string size expressed as a number of bytes with optional SI
+   *   or IEC binary unit prefix (e.g. 2, 3K, 5MB, 10G, 6GiB, 8 bytes, 9mbytes).

it says here mixed, could be an int or a string. The tests though, only test a string input with units. Fix in a followup?

adds the docs.

thanks

draft change notice: https://drupal.org/node/2253127

If a class contains Unit in its name, remove it when converting to PHPUnit..there is no point keeping it.

hmm? the tests now are named BytesTest and EnviromentTest. I dont see any Unit in there

how about reading what the issue is stating and not what the patch is doing?

summary says:

Task to convert

Bootstrap/GetFilenameUnitTest.php
Bootstrap/OverrideServerVariablesUnitTest.php
Bootstrap/MiscUnitTest.php
Bootstrap/ResettableStaticUnitTest.php

to phpunit.

and also read the parent issue which is stating more clearly on what and how to do it.

The issue summary (and title) needs to be updated. Also I suppose @catch might insist on getting an answer to #55 before considering to commit this.

RE: #132, znerol is right, the summary is in accurate - not the patch.

RE #55/#133, The regular classloader will already be loaded by then anyway (loaded in index.php) and again in drupal_classloader(). The APC class loader is then just wrapping this?

@damiankloip, that's right, but it means that if classes are requested before the APC classloader is available, we're hitting the file system every request.

@catch, ah - I see your concern now. Well I think this will still happen (and is currently) as Settings will be initialized before drupal_classloader() is invoked?

Yes I was thinking of a completely different issue here, this patch isn't a problem either way.

After applying the patch grep drupal_check_memory_limit turns up three invocations:

core/includes/bootstrap.inc:function drupal_check_memory_limit($required, $memory_limit = NULL) {
core/modules/color/color.module:    if (!drupal_check_memory_limit($usage + $required, $memory_limit)) {
core/modules/simpletest/simpletest.install:  if (!drupal_check_memory_limit(SIMPLETEST_MINIMUM_PHP_MEMORY_LIMIT, $memory_limit)) {
core/modules/system/system.install:  if (!drupal_check_memory_limit(DRUPAL_MINIMUM_PHP_MEMORY_LIMIT, $memory_limit)) {

Also I see one invocation of parse_size:

core/includes/common.inc:function parse_size($size) {
core/modules/editor/lib/Drupal/editor/Form/EditorImageDialog.php:    $max_filesize = min(parse_size($editor->image_upload['max_size']), file_upload_max_size());

Those invocations should be replaced by the new component-methods.

A quick git log -S through the drush source-tree tells me that none of those functions were ever used in drush. Therefore we even may remove the deprecated function definitions as part of this patch. Though I do not know whether D8 contrib would be affected by that, so we also might want to leave the deprecated functions in for a moment and only remove them before release.

I think we need to leave the legacy procedural wrappers in place, to be removed later on.

Created #2253915: [policy, no patch] Fix coding standards for PHPUnit tests

If anyone is going to reroll this, please make sure to replace the invocations of the deprecated functions (as described in #140).

+++ b/core/modules/system/system.install
@@ -6,6 +6,7 @@
+use Drupal\Component\Utility\Enviroment;

Typo in use statement. Same for color.module and simpletest.install

oops

Great!

+++ b/core/includes/common.inc
@@ -631,17 +632,12 @@ function format_plural($count, $singular, $plural, array $args = array(), array
+ * @deprecated 8.0

@deprecated usage is incorrect.

fixed

Committed eed52b4 and pushed to 8.x. Thanks!