File field's Maximum upload size always passes validation

This adds a test.

Weird. I didn't notice the t() function when testing locally these worked for some reason.

I'm guessing this won't be allowed, but Views does it, and it illustrates the fix. I could have sworn there was an issue out there somewhere to add \Drupal::t() for use in static methods like this, but I couldn't find it.

Needs re-roll.

Manually testing because It seems that the "It will always validate because Bytes::toInt() always returns a number." part had been fixed in #3142934: Replace \Drupal\Component\Utility\Bytes::toInt() with \Drupal\Component\Utility\Bytes::toNumber() due to return type.

Ok, my bad I was not testing the right thing.

I was testing the upload in a field. Not the settings form of the field...

Confirming that there is still the bug.

MR created, I have removed an unused statement in the test file. And kept the "@name" instead of changing into "!name".

I don't know if credits will be applied properly as selectin "author" and "by" does not change the commit command and I didn't commit with my drupal.org user account email address.

Fail related to #2458387: Remove Utility\String class. Fixing.

Not sure what we want to do with the commented out test cases

Yes, me too, that's why I need some feedbacks :). Replied in the MR.

For the record, when uncommeting thelines, this is the warning message:

Warning: A non-numeric value encountered in Drupal\Component\Utility\Bytes::toNumber() (line 55 of core/lib/Drupal/Component/Utility/Bytes.php).
Drupal\Component\Utility\Bytes::toNumber('') (Line: 249)
Drupal\file\Plugin\Field\FieldType\FileItem::validateMaxFilesize(Array, Object, Array)
call_user_func_array(Array, Array) (Line: 280)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 236)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 236)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'field_config_edit_form') (Line: 118)
Drupal\Core\Form\FormValidator->validateForm('field_config_edit_form', Array, Object) (Line: 589)
Drupal\Core\Form\FormBuilder->processForm('field_config_edit_form', Array, Object) (Line: 321)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 573)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 158)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 706)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
require('/project/app/index.php') (Line: 4)

Added some feedback to the MR

Thanks @Lendude for the review.

Test cases added.

Good for me

Just doing a little tag tidying. Nice work everyone!!

Do we need to check that current values are valid? I think it is okay... atm invalid values like 'five' will be determined to be 0 which means that it will be ignored in file_validate_size(). The user will then be prompted to fix this if they save the field again.

Maybe we should consider adding a Bytes::validate() method to validate a string representation and use that here? For example '1five' is still considered valid!

>>> \Drupal\Component\Utility\Bytes::toNumber('1five');
=> 1.1529215046068E+18

And would result in a v. large maximum filesize.

If we added a generic string validation method on the utility class then anywhere else that accepts a byte string could use it.

Hello,

validate method added to the Bytes utility class as asked in comment 34.

I kept the test for the validateMaxFilesize method.

I made an allowed suffixes constant to avoid side effects and not catched cases with regex. If there is a better approach, please suggest.

Thanks for the review.

Checking.

Tests are back to green.

Thanks for the review.

Tests added.

Ready fo review again.

NW for the review points in the MR.

Also, is it worth a functional test showing end-to-end that a bogus value for this limit can be saved and then ignored by uploading a real file? I'd feel a bit safer RTBC'ing this if we weren't only testing the lowest level parts of this, but actually seeing the real bug we're fixing as a test-only fail that becomes green with the rest of the fix applied...

Hello,

Thanks @dww for the review!

Review points taken into account except one https://www.drupal.org/project/drupal/issues/2359675#mr127-note20090 for which I am waiting more people opinion.

About comment #2359675-42: File field's Maximum upload size always passes validation, I am not sure to understand what you mean because the bug is on the field config form, not on an entity form. So I don't think this is needed to go to the point to test a file upload.

Also with this patch it is now not possible to configure a bad value for the maximum upload size, so how did you imagine such test?

The proposed MR seems ok to me !

I'll work on it.
Set back to nw because of #46.

I didn't create a trait as @larowlan pointed bacause if we trim the input, the list must be edited so they get different between each other.
I didn't addressed what @dww pointed because as the lists are different I'm not sure what the best approach is.

Maybe we can reduce the list in FileItemTest just to the tests cases that are different from list in Bytes.php?

I addressed what @larowlan and @dww pointed.
Please review!

Cheers, Paulo.

I have pointed out to Slack that the issue version is 10.0.x-dev where comment 50 assigned it to version 9.3.x-dev (https://drupal.slack.com/archives/C51GNJG91/p1619869940156900)

Thanks @paulocs for the update.

I will complete a little bit the functional test.

Functional test completed.

Needs review!

For me it looks good!

Hello,

Just poking the issue, I don't think there is anything left to be done :)

Hi @Grimreaper, @paulocs this looks good to me, I have just one comment about an extra test case.

Please feel to ping me for another look when this is ready.

Lee

Saving issue credits

(whoops, cross-post with larowlan).

Looks good to me as well.

I had to think for a bit about toNumber() being able to translate units without needing the list of allowed values, vs validation needing an explicit list, but I think this is fine . toNumber() is doing some trickery with string ordering and powers, which will allow some 'invalid' values (i.e. it would accept killerbytes because it's only looking for 'k'), but there's no reason to make that more restrictive, or to make the validation less restrictive, because they're doing different things.

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!

Re-opening for discussion - but we should either roll back or open a follow-up rather than commit two patches.

Hello,

Thanks @larowlan for the review.

Ok to write a new test in a follow-up issue. Can someone just give some advice on what to write exactly please?

@catch thanks for the commits!

Hello Grimreaper,
I think we should add '300 0' to providerTestValidate.

I'll open a follow-up issue to add the test.

Follow-up issue: #3218711: Add a test case for when Maximum upload size is '300 0'

Thanks for opening the follow-up, marking this fixed again.

@paulocs Thanks for the follow-up issue.

Ok, clear now, I didn't realize it was the "$string = '300 0';" the use case of the online example, sorry.

Marked this eleven year old issue as duplicate #870576: Insufficient validation of the max upload file size field.

@catch

Is it just an oversite that credit was not assigned for all the effort put into to https://www.drupal.org/project/drupal/issues/870576 ?

It is heartedly discouraging to put significant effort into an issue simply to have it all thrown away by a duplicate issue.

@bevan.wishart@gmail.com yeh that was definitely an oversight mate - copying over the credit from the other issue.

Feel free to ask if this happens again, we're all human