Problem/Motivation

Drupal core 7.79 adds php 8 compatibility however file_entity is not currently passing tests.

Steps to reproduce

Install Drupal core 7.79 and file_entity 7.x-3.x dev and run automated tests on file_entity.

Proposed resolution

Create a patch or merge request

Remaining tasks

Make patch or merge request and review

User interface changes

N/A

CommentFileSizeAuthor
#26 file_entity-3208277-26.patch1.25 KBjoseph.olstad
#28 file_entity-3208277-28.patch1.24 KBjoseph.olstad
#10 file_entity-n3208277-9.patch1.21 KBDamienMcKenna
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joseph.olstad created an issue. See original summary.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Title: PHP 8 compatibility » PHP 8 compatibility for file_entity
Parent issue: » #3208029: PHP 8.0 compatibility for Media
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs work » Active
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

The testbot shows a number of errors: https://www.drupal.org/pift-ci-job/2019361

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Issue tags: +PHP 8.0, +DrupalFest2021
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Assigned: Unassigned » taran2L-1

I added @Taran2L as a maintainer so that I could assign him to this issue to get his attention.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Assigned: taran2L-1 » Unassigned

Unassigned Taran2L , feel free to assign yourself if you have a moment.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Parent issue: #3208029: PHP 8.0 compatibility for Media »
Related issues: +#3208029: PHP 8.0 compatibility for Media
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented 
/var/www/html/modules/contrib/file_entity/tests/src/Kernel/FileEntityNormalizerTest.php:133
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

ERRORS!

Line 133 of FileEntityNormalizerTest.php

Line 703 of TestResult.php

then something weird here:

02:47:08 sudo chown -R 1001 /var/lib/mysql
02:47:08 /bin/bash: sudo: command not found

the sudo command not found is likely a testrunner issue

Try to focus on the FileEntityNormalizerTest.php first though.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Active » Needs review
FileSize
file_entity-n3208277-9.patch1.21 KB

This fixes one of the bugs, let's see if it fixes others too.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Related issues: +#3189937: PHP 8: Required parameter after optional deprecation notice

One of the errors comes from Views: #3189937: PHP 8: Required parameter after optional deprecation notice

I guess we need to tag a new release of Views for PHP 8 compatibility.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

wow great work!

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

I committed that change,
keeping this open until the remaining tests are fixed , whether it's upstream or here.

Thanks DamienMcKenna!

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

retriggered tests since views release was published
https://www.drupal.org/pift-ci-job/2182265

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

@DamienMcKenna, the views fix helped , now there's only one test fail left instead of 2

https://www.drupal.org/pift-ci-job/2182265

drupal_send_headers

exception: [Warning] Line 1505 of includes/bootstrap.inc:
Header may not contain more than a single header, new line detected

File entity access 229 passes, 0 fails, and 6 exceptions

Not sure why FileEntityAccessTestCase would cause a new line in the header only when running php 8.0.

?

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs review » Needs work

This isn't failing for me locally. Grr..

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

That one test works correctly for me locally.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Related issues: +#3221001: Plan for release of 7.x-1.10

@DamienMcKenna, I get the feeling that the test fail will be resolved once the rules module is tagged and released and once the entity api is tagged and released, both dev branches for these two modules have the fixes for php 8.0

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Related issues: +#3210622: [D7] PHP 8.0 introduced breaking change to call_user_func_array()

there's a chain of events that have to happen for our automated tests to pass, rules needs a new 7.x-2.x tagged release and entity api needs to tag and release, in that order, then we can retrigger our tests here, and then tag and relese both file_entity 7.x-3.x and cherry-pick whatever is necessary for 7.x-2.x to pass also.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

there's a chain of events that have to happen for us to resolve file_entity and media module php 8.0 automated test passing as media relies on file_entity, which in turn automated tests depend on the entity api which in turn relies on rules for it's automated test coverage dependency.

so , in this order:

  1. Tag and release rules 7.x-2.x branch for new tag #3210622: [D7] PHP 8.0 introduced breaking change to call_user_func_array()
  2. confirm that the new rules release fixes automated testing then Tag and release entity api 7.x-1.x branch for new tag for 7.x-1.10 #3221001: Plan for release of 7.x-1.10
  3. Confirm that the new entity api build fixes file_entity automated testing (test dependency) and then tag 7.x-2.x/7.x-3.x branch for new file_entity release #3208277: PHP 8 compatibility for file_entity
  4. confirm that the new file_entity release fixes media php 8.0 automated test coverage , review if anything needs adjusting in the media module #3208029: PHP 8.0 compatibility for Media
  5. Follow up with the core issue for php 8.0, if everything looks good, mark this issue as fixed: #3145797: [META] Make Drupal 7 core compatible with PHP 8.0
  6. Then continue working on making drupal core php 8.1 compatible #3224299: [META] Make Drupal 7 core compatible with PHP 8.1
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

UPDATE:

media is all green now, it was a core fix that did it

queuing file_entity tests now

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

ah, another update, file_entity tests, still one pesky fail, not sure what the cause is at this time, error comes from core but ?
https://www.drupal.org/pift-ci-job/2231985

 testFileEntityPageAccess
	✗	
drupal_send_headers

exception: [Warning] Line 1505 of includes/bootstrap.inc:
Header may not contain more than a single header, new line detected
mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid as a volunteer commented

I don't think this is a problem that's specific to a PHP version; I get the same exceptions testing 7.x-2.x on PHP 7.4 and 8.0

Looks to me like the problem is the Content-Disposition header in file_entity_download_page():

  $headers = array(
    'Content-Type' => mime_header_encode($file->filemime),
    'Content-Disposition' => 'attachment; filename="' . mime_header_encode(basename($file->uri)) . '"',
    'Content-Length' => $file->filesize,

Core's mime_header_encode() returns output split into chunks separated by newlines:

https://git.drupalcode.org/project/drupal/-/blob/7.x/includes/unicode.in...

So this test is generating headers like this:

Content-Type: application/octet-stream
Content-Disposition: attachment; filename="=?UTF-8?B?0KTQsNC50Lsg0LTQu9GPINGC0LXRgdGC0LjRgNC+0LLQsNC90LjRjyBwTmJ3OVA=?=
 =?UTF-8?B?YmM=?="
Content-Length: 87

The newline in the value of the header is what's causing the exception.

So perhaps you need to use a different function to encode the filename which is not going to split it into chunks?

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
FileSize
file_entity-3208277-26.patch1.25 KB

Thanks @mcdruid,

Here is a patch based on what is learned in #25

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Needs work » Needs review
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
FileSize
file_entity-3208277-28.patch1.24 KB

this one instead

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

Credit goes to @mcdruid !

Thanks so much @mcdruid!

Great work

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Status: Needs review » Fixed
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

https://www.drupal.org/project/file_entity/releases/7.x-3.0-beta19

https://www.drupal.org/project/file_entity/releases/7.x-2.33

Thanks everyone!

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented

Great to have the tests passing, but won't the fixed code still mangle long filenames as each encoded chunk has a prefix and suffix added as well as the newline you're now removing:

    while ($len > 0) {
      $chunk = drupal_truncate_bytes($string, $chunk_size);
      $output .= ' =?UTF-8?B?' . base64_encode($chunk) . "?=\n";

I've not tested this - perhaps browsers will understand the prefix/suffix and filenames will be decoded correctly, but I suspect that might not be the case.

Is there a reason you can't just base64_encode the filename?

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

I'll keep a close eye on the file_entity queue, with that said, test coverage is pretty strong so I am thinking we're good as-is.

Status: Fixed » Closed (fixed)

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