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

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

Updated attributions.

The last submitted patch, 32: 2928801-32.patch, failed testing. View results →

Updated description to incorporate rename.

xurizaemon → closed merge request !1196

I'm setting this back to Needs Review. It was set to "Needs work" by the patch in #32 triggering tests which failed, but the patch doesn't introduce any meaningful change that I can see, and the tests pass once more now that the MR is refreshed.

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!

Unsetting patch visibility as the MR is up to date once more.

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 📌 Deprecate format_size() and use Drupal\Core\StringTranslation\ByteSizeMarkup instead Fixed 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 📌 Drupal Usability Meeting 2024-01-12 Active . 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 📌 Discuss moving formatter from file module Active

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.

longwave → closed merge request !1197

@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.

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