Problem/Motivation

\Drupal\file\Entity\File::getSize() can cause deprecations on PHP 8.1 because we pass the NULL value into format_size() in template_preprocess_file_link()

Steps to reproduce

Run \Drupal\Tests\file\Kernel\FileItemTest

Proposed resolution

Either make \Drupal\file\Entity\File::getSize() conform to its interface - it says it returns a string or fix template_preprocess_file_link()

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3240220-2.patch658 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
3240220-2.patch658 bytes

Here's the fix from the meta.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work
Issue tags: +Needs change record

Can we update the docblock of the function format_size() that the first parameter must be a int or a float. Can we also do a check for that and throw a deprecation warning when this is not the case. See: https://www.php.net/manual/en/function.abs.php

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

@daffie it can be a string too... which it nearly always is... see https://3v4l.org/ejYM6

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

I think the question here is not about format_size() but about whether the fix should be in \Drupal\file\Entity\File::getSize() or template_preprocess_file_link()

daffie’s picture

daffie CreditAttribution: daffie commented

From core/includes/common.inc:

function format_size($size, $langcode = NULL) {
  $absolute_size = abs($size);

and PHP defines the functions abs() as abs(int|float $num): int|float. Therefor the variable $size must be an integer value or a float value. It is not saying anything about allowing string values.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

@daffie see https://3v4l.org/ejYM6 - not all PHP docs are correct.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record
Related issues: +#2157945: Deprecate format_size() and use Drupal\Core\StringTranslation\ByteSizeMarkup instead

I bet it because PHP using internal conversion from strings to numbers, I bet because https://wiki.php.net/rfc/string_to_number_comparison

No need to fix docs for function which is going to be deprecated (naming question) #2157945: Deprecate format_size() and use Drupal\Core\StringTranslation\ByteSizeMarkup instead

The issue fixes the problem as #7 points with BC so no CR needed

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed 01a737b and pushed to 9.3.x. Thanks!

  • larowlan committed 01a737b on 9.3.x 
    Issue #3240220 by alexpott: \Drupal\file\Entity\File::getSize() can...

Status: Fixed » Closed (fixed)

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