Remove hardcoded restriction of filesize formatter to fields named "filesize"

Looks simple enough. I agree that this is unneededly limited.

I can see that this could be useful for other cases, but I think it should be reviewed by the usability team. This exposes a new formatter option for *all* integer fields and for most, it won't make sense, especially not as "File size".

One option could be to change the label.

Knocking back to needs review for #9.

I'd also agree to remove the restriction, because not being able to use the formatter is for example a problem if you use the "Filesize" field mapping available with the "Image" media source. While you'd use a custom integer field named field_* for that, you won't be able to use the filesize formatter due to the naming restriction.

But I also see the point Berdir mentioned... perhaps some other label would really be more appropriate for this formatter.

For clarity:

- MR https://git.drupalcode.org/project/drupal/-/merge_requests/1196 proposes the bugfix only
- MR https://git.drupalcode.org/project/drupal/-/merge_requests/1197 proposes both the bugfix *and* re-labels the Formatter from "File size" to "Bytesize (human-readable)" based on @berdir's comment in #9

Changed formatter label to be consistent with others - capitalised and removed brackets:

- *   label = @Translation("byte size (human-readable)"),
+ *   label = @Translation("Byte size"),

I believe it's RTBC now.

patch copy of MR @ 79677d04511fa137838b99f358684cd8aefcceb2

Re-rolled patch #29 for drupal version 10.1.x-dev. Please Review.

Updated attributions.

Updated description to incorporate rename.

@Rinku, appreciate the nudge. I can't tell what prompted the re-roll - the patch in #29 applied identically for me when I tested it against 10.1.x. Was it not applying for you?

I'm setting this back to Needs Review. It was set to "Needs work" by the patch in #32 triggering tests which failed, but AFAICT the patch doesn't introduce any meaningful change from the previous patch.

The only difference I observe between patches in #29 and #32 is that Git's context lines change, with no change in the patch outcome.

@Rinku Jacob 13 if your patch was intended to change something but did not, or introduced some meaningful change that I'm missing, please respond to clarify so we don't lose something here. The issue & MR did need a bump so I'm grateful for that, just want to check if there's a detail missing here. Thanks!

If your intent was simply to trigger a test re-run vs 10.x, you'll be glad to know you can do that by clicking "Add test / retest" on the existing patch - please do that in future, as it saves effort from others in comparing changes between old and new patches.

Tests pass once more now that the MR is refreshed. Unsetting patch visibility as the MR is up to date.

I think we should deprecate this function vs just fully remove. Has it been checked if any contrib modules could be using it?

@smustgrave: can you explain why this method should be deprecated? It is defined in the base class and keeping it in this formatter class won't make any sense though.

Given that the presence/only functionality of the function is the bug described here (preventing the formatter being used on other fields unless the field name condition is met), I don't see how deprecating it improves things - would it not just lock the existing behaviour in for an additional release?

Removing the function seems simplest, but if you feel it's better this MR made the function a no-op in case something is using it, I am happy to update the patch accordingly.

Rebased and adding patch of MR at e2833e38 for those needing a stable patch to apply via composer patches.

Setting this back to needs review - I think the needs work comment has been addressed via the label change, deprecation makes no sense here.

What is probably worth reviewing is if the formatter itself should be deprecated in favour of a new formatter of id byte_size introduced somewhere other than the file module?

As mentioned in #9 this would now be applicable to all integer fields (hence the subsequent label change) - but since then #2157945: Deprecate format_size() and use Drupal\Core\StringTranslation\ByteSizeMarkup instead has been committed which as far as I can see means that with the proposed change here there is nothing specifically file related in this formatter anymore. Does it make sense that file module would provide this formatter?

Usability review

We discussed this issue at #3412373: Drupal Usability Meeting 2024-01-12. That issue has a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @benjifisher, @rkoller, @shaal, and @simohell.

It is certainly an improvement that the formatter no longer displays the ignored "Thousand marker" option. Looking at the patch, I am not sure why this changes.

It is hard to think of a way to describe the formatter. The current label "Byte size" is consistent with the internal name of the ByteSizeMarkup class, but we felt that it is not very clear in the context of the admin UI. We looked at https://en.wikipedia.org/wiki/File_size and https://en.wikipedia.org/wiki/Units_of_information for inspiration, and did not find anything helpful. (Neither "systematic multiples" nor "with binary prefix" seems clear.)

The best suggestion we could think of was "Bytes (KB, MB, ...)". We think that gives a clear indication of what to expect. I am setting the status to NW for this change.

I notice that Comment #26 suggests that parentheses (or brackets) in option labels should be avoided. But there are places in the admin UI where parentheses are used. For example, when I add a filter to a View, two of the options for the operator are "Is empty (NULL)" and "Is not empty (NOT NULL)". I also see the logic in "human-readable": it would make sense to anyone familiar with the ls -h command.

It is out of scope for this issue, but we felt that the formatter should offer more options. For example, there should be options for the decimal marker and whether to use "KB" or "KiB" or "kB". Maybe there should be an option to use powers of 10 (SI notation) instead of powers of 1024, or maybe that should be a separate formatter.

I see some discussion of moving the formatter out of the file module. Perhaps it makes sense to do that in a follow-up issue that also makes the formatter more configurable.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

Thanks @benjifisher for looking at this!

It is certainly an improvement that the formatter no longer displays the ignored "Thousand marker" option

I'm not sure I understand this - can you please clarify what the change is here you saw? It doesn't sound like something this patch changes but I may be missing something.

I have rebased the MR and applied the label change suggested from #44.

I have also created a draft change record https://www.drupal.org/node/3426241 since this is a UI change for site builders.

Back to needs review.

Opened a follow up for the discussion of moving the formatter #3427617: Discuss moving formatter from file module

Change looks good, and not sure removing the function requires test coverage.

Agree that naming this is awkward, but the suggested version does appear to make more sense when shown in a list of similar formatters.

Committed and pushed e97d6c18f4 to 11.x and 1bdd9d754b to 10.3.x. Thanks!

Also published the change record.

@ericgsmith:

It is almost 3 months since I looked at this issue. I think I tested manually, but I may have been relying on the issue summary when I mentioned the "Thousand marker" option.

The Views UI behaves inconsistently at this point. While it will preserve the value of Formatter selection, Formatter settings will not be preserved; if you change "Thousand marker" to "comma", it will be "- None -" when revisisted. (NB: Thousand marker is likely only visible if the underlying storage is numeric.)

I also wrote, "Looking at the patch, I am not sure why this changes." So probably I did test manually. Perhaps that changed because of some other issue, not this one.

If anyone really cares, the recording of the usability meeting is available: see the link in Comment #44.