Problem/Motivation

In #3055193: [Symfony 5] The "Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesser" class is deprecated since Symfony 4.3, use "Symfony\Component\Mime\MimeTypes" instead. we introduced a new MimeTypePass compiler pass to deal with BC issues. However, the code has two undefined variables:

      if (isset($priority_pos)) {
        $arguments[$priority_pos] = $priority;
      }
      if (isset($id_pos)) {
        $arguments[$id_pos] = $id;
      }

$priority_pos and $id_pos are never defined so these arguments will never be set. Further, it looks like $priority is picked up from the service tags but then never set correctly either.

Steps to reproduce

Open MimeTypePass.php in PhpStorm, it highlights undefined variables in red.

Proposed resolution

Figure out what to do with $priority and fix the code.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

longwave created an issue. See original summary.

longwave’s picture

Status: Active » Needs review
StatusFileSize
new1.01 KB

We know that the signature of addGuesser/addMimeTypeGuesser will always be ($guesser, $priority) so we just need to ensure we pass $priority as the second argument.

klausi’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +PHPStan

I think that change makes sense!

It looks like we are not passing the priority at all right now and they all get added with priority 0. Since we sort by priority further above I guess this works by accident. Can we add a priority test in MimeTypePassTest to check that the correct priority is passed?

Phpstan detected these undefined variables as well in #3190393: Start running PHPStan on Drupal core (level 0 - part 2).

longwave’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new2.59 KB

Added priorities in the test and asserted they are passed in as arguments. I tried to see if I could make this cleaner by mocking the guesser service, but couldn't figure out how to get a mock into the container while still having the setup methods called.

klausi’s picture

Status: Needs review » Needs work

Looks good, thanks!

You introduced some unused use statements, that's why the tests are failing. Can you fix those?

ankithashetty’s picture

Status: Needs work » Needs review
StatusFileSize
new2.12 KB
new822 bytes

Fixed custom command errors in the #4 patch as mentioned by #5. Thank you!

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

catch’s picture

Would be useful to have a test-only patch here to see how the failure looks.

longwave’s picture

StatusFileSize
new1.11 KB
new2.12 KB

The last submitted patch, 9: 3186009-6-test-only.patch, failed testing. View results

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 3186009-6.patch, failed testing. View results

longwave’s picture

Status: Needs work » Reviewed & tested by the community

Random fail.

  • catch committed 63416c9 on 9.2.x
    Issue #3186009 by longwave, ankithashetty, klausi: MimeTypePass has...

  • catch committed 58542b0 on 9.1.x
    Issue #3186009 by longwave, ankithashetty, klausi: MimeTypePass has...
catch’s picture

Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

Status: Fixed » Closed (fixed)

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