Insufficient validation of the max upload file size field

My personal thought is that,

1. values which can't be parsed into a quantity of bytes, e.g. "banana", should be a validation error
2. values greater than php.ini post_max_size/upload_max_size (file_upload_max_size()) should be saved, as they are currently, but are not in D6
3. if the Drupal max size is ever greater than the php.ini max size, either because a greater value was entered in Drupal, or php.ini max size was reduced, or the site was migrated to an environment with different php.ini settings, then the administrator should be warned (either by a warning on the file field edit page, or a "hook check"?)

My reasoning is that values which can't be parsed into a quantity of bytes are bad data. Values greater than php.ini max size may not be bad data - like when I enter "30M" although my php.ini max size is only 8M, I may actually want to enable files as large as 30M to be uploaded. In this case "30M" is good data and I need to be warned to fix my php.ini.

Refusing to save values greater than the php.ini max size is just a usability frustration? Like if Drupal is smarter than me and thinks I made a mistake, it should be helpful and suggest how I might resolve inconsistent Drupal and php.ini configuration, vs. tying my hands.

A different issue is that the php.ini max size could be mentioned more prominently than in the max size input description, because it affects the file field regardless of whether you configure a Drupal max size.

Maybe the max size could be mentioned in a heading or summary? near the top of the file field edit page?

Not for field_ui.

However, to ensure that the field only accepts positive integers, file.module could use field_ui's _element_validate_integer_positive() helper function.

thanks, yched. can field_ui be disabled? or is it guaranteed to always be present?

re #4 Field UI can be disabled - but then the form to change the file field definition is not available anymore and nothing can be changed.
Fields and instances definitions can still be updated through API functions, but no validation of the settings happens there (like is often the case in drupal...)

Moving to 8.x - to be backported.

Adding Needs tests. The steps to reproduce should be converted to a test case. Note that the image field has the same problem.

The options are:

1. Add validation to the textfield ensuring the limit isn't greater than max_post_size, not negative and not invalid.
2. Use the new #type => 'number' element. The user picture settings use this. That would be:
   

   $textfield = array(
     '#type' => 'number',
     '#min' => 1,
     '#max' => $max_post_size,
     '#field_suffix' => 'KB',
   );
   

   However, the number element type only works with pure numbers. So we'd have to decide for KB, for example.

To move along getting tests for this scenario, small help is that here is a link to an existing test that already does much of what would be
necessary. This tests that drupal observes and generatest the correct error when uploading a file that's too large.
https://git.drupalcode.org/project/drupal/-/blob/9.0.x/core/modules/file...

I think we can add a number field AND a drop down to select the unit KB, MB.
Like so:

As soon as I start looking at testing, I start asking the questions.

We've talked about validation here, however form validation won't cut it. There are plenty of possible scenarios where this value can be set
through other means. A dev setting it directly via a drush cset reverting configuration etc.

The system kind of handles that now, but the question of how much information to feedback is on the table. If you're importing configuration,
or setting it via code. Perhaps a warning should at least write to the the drupal log.

The form can validate too and present the user with a more friendly message.

The description of the creation field is OK, I think could do with a little improvement for those that don't understand what the php.ini max_upload_size is and the implications. We could dumb that description down and still get the message across.

Case in point, in the file module's own tests, there's the file creation trait.

  public function createFileField($name, $entity_type, $bundle, $storage_settings = [], $field_settings = [], $widget_settings = []) {
    $field_storage = FieldStorageConfig::create([
      'entity_type' => $entity_type,
      'field_name' => $name,
      'type' => 'file',
      'settings' => $storage_settings,
      'cardinality' => !empty($storage_settings['cardinality']) ? $storage_settings['cardinality'] : 1,
    ]);
    $field_storage->save();

    $this->attachFileField($name, $entity_type, $bundle, $field_settings, $widget_settings);
    return $field_storage;
  }

Calling that in a functional test will bypass form validation and result in a false positive test.
Of course we could use the browserTestBase to edit the field configuration form directly.
That might be what needs to happen to test the form validation.

A patch that provides a basic demonstration of what I think we want to do here.

This is a webdriver test, so it's bulky for what it does and could be done as a kernel test at least.
But I think it's useful for clarity on direction to take this.. and get it tested.

remove duplicate comment

Side note, that patch is supposed to fail as it's testing for the desired outcome, not the outcome it actually gets.

I whipped up this simple kernel test

  /**
   * Tests that a sane Max file size is entered.
   */
  public function testValidateMaxfilesize() {
    $form_state = (new FormState()); //->setValues([]);
    FileItem::validateMaxFilesize(['#value' => "100 MB"], $form_state); // Pass
    FileItem::validateMaxFilesize(['#value' => "bananas"], $form_state); // Fail
  }

Which suggests that the validation isn't being called in form submission. Because "bananas"
fails the test with A non-numeric value encountered

Existing function currently:

  /**
   * Form API callback.
   *
   * Ensures that a size has been entered and that it can be parsed by
   * \Drupal\Component\Utility\Bytes::toInt().
   *
   * This function is assigned as an #element_validate callback in
   * fieldSettingsForm().
   */
  public static function validateMaxFilesize($element, FormStateInterface $form_state) {
    if (!empty($element['#value']) && !is_numeric(Bytes::toInt($element['#value']))) {
      $form_state->setError($element, t('The "@name" option must contain a valid value. You may either leave the text field empty or enter a string like "512" (bytes), "80 KB" (kilobytes) or "50 MB" (megabytes).', ['@name' => $element['title']]));
    }
  }

And it is at least set as an #element_validate callback as suggested by the doc comment.

    $element['max_filesize'] = [
      '#type' => 'textfield',
      '#title' => t('Maximum upload size'),
      '#default_value' => $settings['max_filesize'],
      '#description' => t('Enter a value like "512" (bytes), "80 KB" (kilobytes) or "50 MB" (megabytes) in order to restrict the allowed file size. If left empty the file sizes will be limited only by PHP\'s maximum post and file upload sizes (current limit <strong>%limit</strong>).', ['%limit' => format_size(Environment::getUploadMaxSize())]),
      '#size' => 10,
      '#element_validate' => [[get_class($this), 'validateMaxFilesize']],
      '#weight' => 5,
    ];

I'll get that test into a patch, probably tomorrow. Then some investigation is necessary into why the form allows "bananas" as valid input.

Patch ahoy.

Quick little kernel patch .. can replace the full web driver test from previous patch.

1. +++ b/modules/file/tests/src/Kernel/FileItemFieldSettingsFormTest.php
   @@ -0,0 +1,25 @@
   +class FileItemFieldSettingsFormTest extends KernelTestBase {
   

   I wonder if a Unit test is enough for this. Is there specific need for a Kernel?

2. +++ b/modules/file/tests/src/Kernel/FileItemFieldSettingsFormTest.php
   @@ -0,0 +1,25 @@
   +    FileItem::validateMaxFilesize(['#value' => "100 MB"], $form_state); // Pass
   +    FileItem::validateMaxFilesize(['#value' => "bananas"], $form_state); // Fail
   

   Usually we would see some asserts. I think for the 100 mb one, we should assertEmpty on $form_state->getErrors(). And then on fail, assertNotEmpty.

3. +++ b/modules/file/tests/src/Kernel/FileItemFieldSettingsFormTest.php
   @@ -0,0 +1,25 @@
   +    $form_state = (new FormState());
   

   The following is sufficient:

   $form_state = new FormState();

Thanks. Yeah this is not intended as a final path. For now the test just demonstrates the bug.

The JavaScript tests demonstrates that no validation occurs. The kernel test demonstrates that there is a validation function.. but the form does not validate.

That basic test could be a unit test. I'd prefer a test somewhere between these two. I feel a good test needs to submit the form

Updating issue summary.

New patch!!!

This one has replaced the Kernel test with a unit test. And also provides enhanced validation.

I haven't implemented anything about validating against php.ini as yet.

bad patch path. new patch added

Aaah.. New patch.. added some more test conditions for negative numbers.. and the fixed the code to validate those too.

The validating against the php.ini max size really needs some consideration.
Each file field gets it's own max upload value, so in the scenarion that php lowered it's value, you'd only be wanred if you edited a field
(and you'd have to adjust the others)..

I don't think it should at all enforce that value to be lower that php's max.. but some feedback to make a user aware of the situation.

Plus there are other modules that upload files too. I like the cron idea in that you could then mark it as an issue on the status page of drupal..
I wonder about performance issues.

Looks as though a few other tests fail now.. Because the form fails validation those tests I imagine.

I love patches <insert sarcasm emoji here> ..

This patch will fix the first of those broken tests, and hopefully the others. My previous patch was dissallowing empty values as input.

Expect there will be a suggestion to move that data to a data provider.. hmm.

1. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -246,9 +307,30 @@ class FileItem extends EntityReferenceItem {
   +    $size = self::stripSpacesFromString($element['#value']);
   +
   +    $not_start_with_number = self::stringNotStartWithNumber($size);
   +    $has_numbers_after_alpha_chars = self::hasNumbersFollowingAlphaCharInString($size);
   ...
   +        !self::toIntCanParseString($size)
   

   Are all these necessary? Can we just parse bytes and see what happens? I'd like to see a test of what happens in that case.

2. +++ b/core/modules/file/tests/src/Unit/FileItemFieldSettingsFormTest.php
   @@ -0,0 +1,65 @@
   +    $inputs = [
   +      '' => FALSE,
   +      '0 bytes' => FALSE,
   +      '0 K' => FALSE,
   +      '100bytes' => FALSE,
   

   Include edge cases where the string starts or ends with a space.

3. +++ b/core/modules/file/tests/src/Unit/FileItemFieldSettingsFormTest.php
   @@ -0,0 +1,65 @@
   +    $inputs = [
   +      '' => FALSE,
   +      '0 bytes' => FALSE,
   +      '0 K' => FALSE,
   +      '100bytes' => FALSE,
   

   Should be a data provider.

I've notice that test is a false positive too.

I need to add test for 'formError()->shouldNotBeCalled()' I must have lost it along the way.

Thanks.. agreed I'm proceeding the data provider. from #45.3

I've added some edge cases as per $45.2 locally which showed up some issues (yay tests :) )
I'm having to make some decisions about what data to allow and not.. but suggestions are welcome upon review.

As for #45.1, I'm not quite sure I understand what you mean.
And to cover some more of these edge cases, I'm about to add more checks in that same location.

And here we go for another round of what will my patch look like today.

#45.2 I have added more edge case test data - and adjusted code to work with those edge cases. Nice catch @heddn

#45.3 I have moved it to a data provider.

You will see I've added yet another check that the data is alphnumeric.

My gut is telling me this is deserving of splitting the storageSettings and fileSettings forms into their own classes.
And this max upload filesize though small, still worthy of extraction.

Had a crack at adding a max filesize units select box with bytes, kilobytes, megabytes, gigabytes, this means an additional field, and probably wrapping the size and units fields in a fieldset. That means merging them and validating the result, and I believe overriding the save method of FieldConfigEditForm.php, with a hook or replacing the form, or making that form handle this specific case. All of which gets complicated in an attempt to simplify things. Better eyes than mine may see a simpler solution.

The idea of splitting the fileSettings and storageSettings out involves refactoring FieldConfigEditForm too, so that worth it's own issue.

Energy spent moving the max_filesize validation to it's own class or service is where the value is.

And so we have it. Validation mechanics for max file upload field now in it's own helper class. It is much cleaner this way.

I'm also now considering stepping out to a wider view and approaching this from the idea of testing the FieldConfigEditForm
not just this validation part. As it stands now this unit test test the validator directly, as it did before but is was less obvious
when the validator was in the FileItem class.

I'm pondering creating a test that implements formInterface and submitting sample values to the form from there.

Really interested in some input. I've been lucky to have heaps of capacity recently to put time on this, but that well will run dry.

+++ b/core/modules/file/tests/src/Unit/FileItemFieldSettingsFormTest.php
@@ -0,0 +1,93 @@
+use Drupal\Core\StringTranslation\TranslatableMarkup;
...
+   *

One nitpick, Unused use statement.

Chunked file uploading is a topic you might be interested in -- how to proper validating max-size of files uploaded via chunking. So adding two related issues.

Continue with #54

1. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItemMaxFileSizeValidator.php
   @@ -0,0 +1,125 @@
   +    if (
   +      !empty($size)
   +      &&
   +      (
   +        $is_not_alphanumeric
   +        ||
   +        $not_start_with_number
   +        ||
   +        $has_numbers_after_alpha_chars
   +        ||
   +        !self::toIntCanParseString($size)
   +      )
   +    ) {
   

   Prefer the following

       if (!empty($size)) {
         if($is_not_alphanumeric ||
           $not_start_with_number ||
           $has_numbers_after_alpha_chars ||
           !self::toIntCanParseString($size)) {
           $translatable_markup = new TranslatableMarkup('The "@name" option must contain a valid value. You may either leave the text field empty or enter a string like "512" (bytes), "80 KB" (kilobytes) or "50 MB" (megabytes).', ['@name' => $element['#title']]);
           $form_state->setError($element, $translatable_markup);
         }
       }
   

   Or at least, moving operations to the previous line. An example from core/lib/Drupal/Component/Gettext/PoStreamReader.php

           if (($this->context != 'MSGID') &&
               ($this->context != 'MSGCTXT') &&
               ($this->context != 'MSGID_PLURAL') &&
               ($this->context != 'MSGSTR_ARR')) {
   

2. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItemMaxFileSizeValidator.php
   @@ -0,0 +1,125 @@
   +    return !preg_match('/^[0-9]/', $string);
   

   [0-9] could be replaced by \d

Think more about #55.1

+++ b/core/modules/file/src/Plugin/Field/FieldType/FileItemMaxFileSizeValidator.php
@@ -0,0 +1,125 @@
+    $size = self::stripSpacesFromString($element['#value']);
...
+    $not_start_with_number = self::stringNotStartWithNumber($size);
+    $has_numbers_after_alpha_chars = self::hasNumbersFollowingAlphaCharInString($size);
+    $is_not_alphanumeric = self::stringIsNotAlphanumeric($size);
...
+    if (
+      !empty($size)

if should be called right after the $size = ..., so that, if the $size is empty, the 3 lines of code between $size = ... and if won't be executed. Or, no matter the $size is empty or not, those 3 lines got executed.

So proposing the following.

 public static function validateMaxFilesize($element, FormStateInterface $form_state) {
    $size = self::stripSpacesFromString($element['#value']);
    if (!empty($size)) {
      if(self::stringIsNotAlphanumeric($size) ||
        self::stringNotStartWithNumber($size) ||
        self::hasNumbersFollowingAlphaCharInString($size) ||
        !self::toIntCanParseString($size)) {
        $translatable_markup = new TranslatableMarkup('The "@name" option must contain a valid value. You may either leave the text field empty or enter a string like "512" (bytes), "80 KB" (kilobytes) or "50 MB" (megabytes).', ['@name' => $element['#title']]);
        $form_state->setError($element, $translatable_markup);
      }
    }
  }

Addressing #54, #55 and #56. Changing the target version to 9.1.x.

"around around around it goes, where it stops nobody knows"

@jungle thanks for those additions. Triggered some further refactoring for readability.

Late in the game to say it now but this validator is looking much less like a 'max filesize upload' setting validator, and more of a generic input validator for the Bytes::toInt() utility. Which means this validator doesn't belong in it's current directory/namespace but rather `Drupal/Component/Utility` or roll it into the `Drupal/Component/Bytes` class itself.

Remembering that the root of this issue is with the form relying on Bytes::toInt() to self validate but toInt() (almost?) always returns a number, and only false in edge cases. So it makes sense that we have ended up with a toInt() validator. (although this validator currently strictly accepts only strings, toInt() accepts integers too. It makes sense to add a public 'validate' input to `Bytes`.. Or modify toInt to return saner output, but that becomes a compatability issue.

Self review, doh.

+++ b/core/modules/file/src/Plugin/Field/FieldType/FileItemMaxFileSizeValidator.php
@@ -22,21 +31,39 @@ class FileItemMaxFileSizeValidator {
+    // Spaces are irrelevant to us. Bytes::toInt will process with or without
+    // them. Make our lives easier by just doing away with them.
+    self::$size = self::stripSpacesFromString($element['#value']);
+
+    // Having removed whitespace, if our string is now empty then the user
+    // either entered nothing or a string of whitespace. Accept both cases as
+    // valid.
+    if (empty(self::$size)) {

Rename from stripSpacesFromString to stripWhitespaceFromString

`tests/Drupal/Tests/Component/Utility/BytesTest.php`

  public function testToInt($size, $expected_int) {
    $this->assertEquals($expected_int, Bytes::toInt($size));
  }

  public function providerTestToInt() {
    return [
      ['1', 1],
      ['1 byte', 1],
      ['1 KB'  , Bytes::KILOBYTE],
      ['1 MB'  , pow(Bytes::KILOBYTE, 2)],
      ['1 GB'  , pow(Bytes::KILOBYTE, 3)],
      ['1 TB'  , pow(Bytes::KILOBYTE, 4)],
      ['1 PB'  , pow(Bytes::KILOBYTE, 5)],
      ['1 EB'  , pow(Bytes::KILOBYTE, 6)],
      ['1 ZB'  , pow(Bytes::KILOBYTE, 7)],
      ['1 YB'  , pow(Bytes::KILOBYTE, 8)],
      ['23476892 bytes', 23476892],
      // 76 MB.
      ['76MRandomStringThatShouldBeIgnoredByParseSize.', 79691776],
      // 76.24 GB (with typo).
      ['76.24 Giggabyte', 81862076662],
    ];
  

This Bytes class is interesting. I think this test coverage is a little light. And it's uber flexible rather than strict, the `76MRandomStringThatShouldBeIgnoredByParseSize.` is of note.

Ok. So I don't want to put this form validation into Bytes. It's not appropriate because it's much more strict.
We can leave it here associated with FileItems.

* I started looking at moving this over to a kernel test and test the actual form submission.

* I then discovered `modules/field/tests/src/Functional/FormTest.php` inheriting from BrowserTestBase, which does generic testing for form confg and storage. Is also flagged to be refactored: #3077193: FormTest should be converted (as much as possible) to a kernel test

* There is also `web/core/modules/file/tests/file_test/src/Form/FileTestForm.php` this tests the File form itself, but not the config or storage forms.

I'm resistent to making this conversion now for two reasons.

1) To decide if to create a separate test for config, storage with unique names or should they form part of FormTest class.

2) Given there is a plan to refactor FormTest, I'm hesistant to refactor this into it now. When this unit tests gives the benefits we need
and we can make the required improvements the future after the FormTest conversion. #3077193: FormTest should be converted (as much as possible) to a kernel test

3) Perhaps this can stand alone as it's own Kernel test.

4) It's a valuable solution as it stands now, with the added validator and associated test.
* Let the FormTest refactor issue progress on it's own.
* Add a separate issue for "test coverage for File configration and storage settings forms".
* As there is another ticket also for chunked uploads, we can leave that in it's own ticket and close this off too.

NW for #59 and the following

1. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItemMaxFileSizeValidator.php
   @@ -0,0 +1,127 @@
   +   * @var string $size
   

   ”Do not append variable name "$size" to the type declaration in a member variable comment“ 1 code standard violation to fix.

2. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItemMaxFileSizeValidator.php
   @@ -0,0 +1,127 @@
   +  public static function validateMaxFilesize($element, FormStateInterface $form_state) {
   

   Missing @params

working on it

@jungle Please review this patch All the changes #62 has been implemented.
Thanks
Pavnish

1. >NW for #59 and the following

   Thanks @pavnish. Missed addressing #59

2. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItemMaxFileSizeValidator.php
   @@ -28,8 +28,12 @@
       * This function is assigned as an #element_validate callback in
       * fieldSettingsForm().
   +   * @param array $element
   

   One empty line before @param is expected. Furthermore, the expected comment format is:

   [Summary]
   
   [Descritpion]
   
   @param
   

   So

       * This function is assigned as an #element_validate callback in
       * fieldSettingsForm().

   should be re-written to one-line or

   [Summary]
   
   [Descritpion]
   

@jungle Thanks for pointing addressed #59 .
Please review this patch.

@jungle Thanks for pointing #59.
Please ignore #66
The #59 and #65 has been implemented
Please review this patch.

This is looking good.

1. I'm not sure core/modules/file/src/Plugin/Field/FieldType/FileItemMaxFileSizeValidator is the correct namespace for this new class. That's a plugin folder, where we typically store plugins. I see the merit in splitting this out into it's own class - but given it still relies on FormStateInterface as an argument, it's not completely generic. So I think perhaps we should move it back to where it was - unless we want to make it completely generic as a class in Drupal\Component\FileSystem, which would then require it to simply return a boolean if it were valid, and we'd need to use the existing validation method to call that, and set the error. Thoughts?

2. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItemMaxFileSizeValidator.php
   @@ -0,0 +1,132 @@
   +  private static $size;
   ...
   +    self::$size = self::stripWhitespaceFromString($element['#value']);
   ...
   +    if (empty(self::$size)) {
   ...
   +    return !preg_match('/^\d/', self::$size);
   ...
   +    return (bool) preg_match('/[a-zA-Z]+(\d+)/', self::$size);
   ...
   +    return (bool) !preg_match('/^[a-zA-Z0-9]+$/', self::$size);
   ...
   +      Bytes::toInt(self::$size);
   

   I don't think we need to use a static property here. We can just use a regular variable and pass it along as an argument to the other methods.

3. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItemMaxFileSizeValidator.php
   @@ -0,0 +1,132 @@
   +      $valid = FALSE;
   

   we could return FALSE here and that would avoid the need for the $value local variable

4. +++ b/core/modules/file/tests/src/Unit/FileItemFieldSettingsFormTest.php
   @@ -0,0 +1,92 @@
   +      ['0 bytes', FALSE],
   +      ['123456.03 ', TRUE],
   

   This is some awesome coverage!

   Would it be possible to give each of these cases a name.

   One of the unfortunate problems with test-providers that aren't named is that PHPUnit will say 'test case 3' failed, which is great if you have 3 cases. But with ~30 odd here, 'Test case 16 failed' means you have to count down to find which one that is.

   If we give each of these a name (by making the array associative, with the names as keys) then it will say something like 'Test case "words with spaces" failed' which is much easier to debug.

This patch addresses #68-3 and #68-4
Nice spotting on the unnecssary $valid var.

Coming up with descriptive names for each input in the data provider seemed tedious and unnecessary.
Instead transform the array to also use the input string as a label.
Sample failure output:

1) Drupal\Tests\file\Unit\FileItemFieldSettingsFormTest::testValidateMaxfilesize with data set "Sample input: 'bananas'" ('bananas', false

Textdox format

 Validate maxfilesize with data set "Sample input: '0 bytes'"
 ✔ Validate maxfilesize with data set "Sample input: '123456.03 '"
 ✔ Validate maxfilesize with data set "Sample input: '   '"
 ✔ Validate maxfilesize with data set "Sample input: ''"
 ✔ Validate maxfilesize with data set "Sample input: '0 K'"
 ✔ Validate maxfilesize with data set "Sample input: '100bytes'"
 ✔ Validate maxfilesize with data set "Sample input: '10000'"
 ✔ Validate maxfilesize with data set "Sample input: '100 bytes'"
 ✔ Validate maxfilesize with data set "Sample input: '999989 bytes'"
 ✔ Validate maxfilesize with data set "Sample input: '999989bytes'"
 ✔ Validate maxfilesize with data set "Sample input: '2'"
 ✔ Validate maxfilesize with data set "Sample input: '3K'"
 ✔ Validate maxfilesize with data set "Sample input: '5MB'"
 ✔ Validate maxfilesize with data set "Sample input: '10G'"
 ✔ Validate maxfilesize with data set "Sample input: '6GiB'"
 ✔ Validate maxfilesize with data set "Sample input: '8bytes'"
 ✔ Validate maxfilesize with data set "Sample input: '9mbytes'"
 ✔ Validate maxfilesize with data set "Sample input: '1 0 0 0 0 m b y tes'"
 ✔ Validate maxfilesize with data set "Sample input: 'nonumbers'"
 ✔ Validate maxfilesize with data set "Sample input: 'bread'"

Reference to:

#68-1 I will ponder that further.

#68-2 The patch was passing the value around, and I was finding it becomming unwieldy where I was documenting the same variable description over and over.. The main reason I shifted it up to a property.. document it just once and improve readibility. What is the benefit to just passing a variable around versus just setting a property?

What is the benefit to just passing a variable around versus just setting a property? static properties are a bit of an anti-pattern. Static methods shouldn't have state (because they're static) but we're simulating state with a static property. I don't, might be just me but it feels icky and I don't think its something we regularly do in core, other than for e.g. singletons like Settings. Happy to put that to others for their input.

Okay. Definitely needs review now.

I attempted to implement something along the lines of #68-1

Which as it unfolded ended up being the same point I'd thought up earlier:
#58

Late in the game to say it now but this validator is looking much less like a 'max filesize upload' setting validator, and more of a generic input validator for the Bytes::toInt() utility. Which means this validator doesn't belong in it's current directory/namespace but rather `Drupal/Component/Utility` or roll it into the `Drupal/Component/Bytes` class itself.

The test is now incorrectly namespaced but I have no clue as to where it should go.

I see some doc comments are incorrec now.. but I must put this down and do other things :D

Blurgh, well that's really broken..

Patch's test succeeds in my local. BytesValidator not found.. hmm.

I was curious about the error so had a look, hope this is helpful and doesn't double up on work. BytesValidator was in the /lib not /core/lib.

I know how I did that. Rookie error.

Thanks @quietone for fixing it :fingers_crossed: waiting for the tests.

I'm sold on removing that static property.

I would prefer to remove the static from the methods too and instantiate the class rather than calling it statically or passing variables through a chain. Now that we call the validator class from the validation Method in fileItem rather than as an #element_validator we can.

Have literally bean beating around the bush. And having a ball. Back in front of the computer now..

Refactored this back to not having that property as state, while keeping the static class.

Kicking off testbot.

Last patch failed to apply , follow a new patch.

Fixed lint fails of patch #81.

1. +++ b/core/lib/Drupal/Component/Utility/BytesValidator.php
   @@ -0,0 +1,150 @@
   +   *   or IEC binary unit prefix (e.g. 2, 3K, 5MB, 10G, 6GiB, 8 bytes, 9mbytes).
   

   8 bytes is the odd one out here with the space. Is that intentional?

2. +++ b/core/lib/Drupal/Component/Utility/BytesValidator.php
   @@ -0,0 +1,150 @@
   +    return trim(preg_replace('/\s+/', '', $byte_size_string));
   

   What does the trim do here that the preg_replace doesn't?

3. +++ b/core/lib/Drupal/Component/Utility/BytesValidator.php
   @@ -0,0 +1,150 @@
   +    return (bool) preg_match('/[a-zA-Z]+(\d+)/', $byte_size_string);
   

   are the back-reference brackets () required here?

4. +++ b/core/lib/Drupal/Component/Utility/BytesValidator.php
   @@ -0,0 +1,150 @@
   +    catch (\Exception $e) {
   

   Bytes:toInt doesn't throw any Exceptions - should this be \Throwable or \Error instead?

5. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -197,11 +199,11 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
   -   * ensures that the file directory path is not included at the beginning of the
   -   * value.
   +   * ensures that the file directory path is not included at the beginning of
   +   * the value.
   
   @@ -220,7 +222,8 @@ public static function validateDirectory($element, FormStateInterface $form_stat
   -   * as a space-separated list for compatibility with file_validate_extensions().
   +   * as a space-separated list for compatibility with
   +   * file_validate_extensions().
   

   these changes are out of scope here

6. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -239,15 +242,17 @@ public static function validateExtensions($element, FormStateInterface $form_sta
   +    $bytes_validator = new BytesValidator();
   +    if ($bytes_validator->validateByteSizeString($element['#value'])) {
   

   the method is static, so we don't need the new call here, just BytesValidator::validateByteSizeString

7. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -260,10 +265,9 @@ public static function validateMaxFilesize($element, FormStateInterface $form_st
   -   *
   ...
   -  public function getUploadLocation($data = []) {
   +  public function getUploadLocation(array $data = []) {
   
   @@ -281,7 +285,7 @@ public function getUploadLocation($data = []) {
   -  protected static function doGetUploadLocation(array $settings, $data = []) {
   +  protected static function doGetUploadLocation(array $settings, array $data = []) {
   

   these changes are out of scope too

8. +++ b/core/modules/file/tests/src/Unit/BytesValidatorTest.php
   @@ -0,0 +1,85 @@
   +    $data = [
   

   love the work here, great coverage

9. +++ /dev/null
   @@ -1,62 +0,0 @@
   -namespace Drupal\Tests\Component\Utility;
   

   we need to put this file back, I assume it was a bad re-roll that removed it

I see there a some conflict markers in my patch..some.how?!?!

This latest patch has pulled in some of the wrong code.

Thanks for the review

Tried to address @larowlan's feedback in #84:

1. Having not written this code, I wasn't sure what the intention was. So I left it as-is.
2. Removed the call to trim().
3. Not for any reason I can see. Removed.
4. I'm not sure what the reasoning is there, since as far as I can tell, nothing in Bytes::toInt() throws anything -- at least not explicitly. To cast the widest net possible, changed this to \Throwable.
5. Reverted.
6. Fixed.
7. Reverted.
8. My heart skipped a beat too!
9. Restored, I think.

I'm not sure if this still needs the "Needs tests" tag, so I'm leaving it for now.

This has just been fixed in #2359675: File field's Maximum upload size always passes validation.