Deprecate format_size() and use Drupal\Core\StringTranslation\ByteSizeMarkup instead

Looks code changes are ready, what's left to polish except change record?

...hiding files as MR has latest changes to review

@andypost

what's left to polish except change record?

There is one review comment, can you pls provide your's feedback on it ?

Oleh Vehera
Missing documentation for arguments. What is the type of $size argument? Which values are allowed?

Bhanu951
I think $size can be either float or NULL ?

One more discussion we need to make is where the class should be placed as commented in # 237 📌 Deprecate format_size() and use Drupal\Core\StringTranslation\ByteSizeMarkup instead Fixed

From @alexpott :

One thing I ponder is whether Drupal\Core\StringTranslation is the correct place for this. Maybe Drupal\Core\Utility is a better location because the translation aspect is really a secondary effect - the major part of this class and reason for using it is to convert a large number of bytes into something more human friendly.

Before updating the issue summary and change record with the latest changes I think we should sort this out. The more I think about the more I'm convinced that this belongs outside the translation system. The fact this thing produces translatable strings is an implementation detail. The important this is about converting a large number of bytes into a readable string. Utility is not a bad place for this - we've got a hodge-podge of things in there like TableSort - which is not that dissimilar - it's a helper for rendering sortable tables with only static methods.

Added some comments to the MR. More importantly we need to update the title as it no longer reflects the implementation.

Added code suggestions. Please review

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

kim.pepper → made their first commit to this issue’s fork.

Needs an IS update as we are now calling \Drupal\Core\StringTranslation\ByteSizeMarkup::create() instead of TranslationInterface::formatByteSize()

Updated title, IS and CR. Removed tags.

Some open threads in the MR.

Addressed most of the feedback. Still have to look at while \Drupal\Tests\file\Functional\FileFieldWidgetTest::testMaximumUploadFileSizeValidation() is failing because the value isn't being trimmed.

Removing credit from myself as all I did was a rebase to run the tests.

@kim.pepper I ran the test locally without the space around 5.1 megabytes and they pass. Are they just failing here?

And is that a stopper for this?

Looks like those tests are passing now? Not sure why they were failing before.

Going to go ahead and mark but if that one change could be reverted back (if it matters at all)

Left a review on the MR

I think it ready as all feedback addressed

Meantime the main worries are about strong type requirement for function create(float|int $size,...) but
As All tests passing we can get more feedback (and address it) withing 10.2 cycle so contrib and custom can fix it properly

Checked uploadprogress code and it returns values as strings (probably will need type-cast) but can't test if it fails or throws deprecation, see https://github.com/php/pecl-php-uploadprogress/blob/master/uploadprogres...

In related there's APCu replacement 📌 Add support for built-in PHP session upload progress Needs work
Moreover uploadprogress working only with Apache2 server and we have no testing env to be sure

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased on 11.x. Back to RTBC

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Merge with 11.x. Back to rtbc.

This poor issue. Approaching its 10th birthday, and bumping in and out of RTBC for 5 years.

I pruned 135 empty-ish comments (status and metadata changes with no substantive info, test fail comments, etc.) and it still barely renders due to GitLab JS crossposts.

It's also been tagged for framework manager review for 3 years. A couple framework managers have been reviewing it, but not given signoff that I can see. I will reach out to other committers.

I hadn't reviewed this since before it moved to a value object/ByteSizeMarkup c. 2021/2022, it looks so much better using that.

I did think ByteSizeMarkup sounds like a cheeky name for some kind of HTML preprocessor - make your markup bite size! Not an actual problem.

There was one remaining question from @alexpott here about whether it should be in the translation or utility namespace. On the one hand yes it's not actually making things translatable (except in the sense it produces a translatable string), but it does seem extremely closely related to TranslatableMarkup and PluralTranslatableMarkup etc. so this feels fine to me. There haven't been any strong opinions since the question was asked, so I think we should go ahead here, seems 50/50 to me at worst. MarkupInterface and friends are in Drupal\Component\Render so that's definitely not the right place, but had to check.

Committed/pushed to 11.x, thanks!

🎉

Automatically closed - issue fixed for 2 weeks with no activity.