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
Comment #2
longwaveWe know that the signature of
addGuesser/addMimeTypeGuesserwill always be($guesser, $priority)so we just need to ensure we pass$priorityas the second argument.Comment #3
klausiI 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).
Comment #4
longwaveAdded 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.
Comment #5
klausiLooks good, thanks!
You introduced some unused use statements, that's why the tests are failing. Can you fix those?
Comment #6
ankithashettyFixed custom command errors in the #4 patch as mentioned by #5. Thank you!
Comment #7
klausiLooks good, thanks!
Comment #8
catchWould be useful to have a test-only patch here to see how the failure looks.
Comment #9
longwaveComment #12
longwaveRandom fail.
Comment #15
catchCommitted/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!