Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix service definitions for mime and extension guessers #1379

Merged
merged 2 commits into from
Oct 6, 2021
Merged

Fix service definitions for mime and extension guessers #1379

merged 2 commits into from
Oct 6, 2021

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Jun 8, 2021

Q A
Branch? 2.0
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets Fixes #1287
License MIT
Doc PR N/A

In #1217 several deprecations from symfony/http-foundation for Symfony 4.3 were addressed, but the related service configurations weren't updated, leaving the liip_imagine.mime_type_guesser and liip_imagine.extension_guesser services in a broken state when symfony/http-foundation:^5.0 is used.

This PR aims to fix the service definitions by conditionally changing those two services to be aliases of the mime_types service the FrameworkBundle provides if a version of the FrameworkBundle that offers that service is installed. This should retain B/C for Symfony 4.2 and earlier by using the existing definition which uses the classes from symfony/http-foundation while offering forward compatibility by mapping the services to the replacement API for the deprecated classes. When support for Symfony 4.2 and earlier are removed, this compiler pass can be removed and the two services converted into aliases unconditionally.

*
* @internal
*/
final class MaybeSetMimeServicesAsAliasesCompilerPass extends AbstractCompilerPass
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purposefully final and internal to make it clear this is a B/C layer that can (and should) go away when older Symfony version support is dropped.

@lsmith77
Copy link
Contributor

@dbu / @Tobion

Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR.

i am a bit confused why you say we can remove the compiler pass when we drop support for symfony <= 4.2. if we then only revert this change, won't things be broken again? or do you mean that after that, we can just remove the service definitions for liip_imagine.mime_type_guesser and liip_imagine.extension_guesser alltogether?

{
if ($container->hasDefinition('mime_types')) {
$container->removeDefinition('liip_imagine.mime_type_guesser');
$container->removeDefinition('liip_imagine.extension_guesser');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be a BC break if people customized one of these services?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but as is these definitions already are invalid when symfony/http-foundation:>=5.0 is installed as the Symfony\Component\HttpFoundation\File\MimeType namespace just does not exist anymore. So, there are 3 realistic (IMO) options:

  1. This PR, which has a potential B/C break in changing a service definition to a service alias (and, I guess the same B/C concerns as the next item...)

  2. Change the service definitions to use the right classes conditionally directly in the container extension class (which, depending on the B/C policy could also be a break because it will change the classes provided by these services and cause existing typehints or instanceof checks to fail if they aren't expecting the counterparts from the symfony/mime component)

  3. Introduce new services that always point to the new classes and deprecate the existing services, which adds a lot of maintenance burden in having the bundle try to resolve to the correct services based on the active Symfony version, and if anyone is trying to use these services in their own applications they have to go through the effort of finding the right service for their Symfony version

IMO, I'd say this PR is the "best" option moving forward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the clarification. yeah, lets do it this way. i will add a note in UPGRADE.md

@dbu dbu added the State: Need To Investigate 🔎 This item requires additional investigation to determine a resolution. label Oct 5, 2021
@mbabker
Copy link
Contributor Author

mbabker commented Oct 5, 2021

i am a bit confused why you say we can remove the compiler pass when we drop support for symfony <= 4.2. if we then only revert this change, won't things be broken again? or do you mean that after that, we can just remove the service definitions for liip_imagine.mime_type_guesser and liip_imagine.extension_guesser alltogether?

The service definitions would be updated to the below when Symfony 4.3 is the minimum supported version:

<service id="liip_imagine.mime_type_guesser" alias="mime_types" />
<service id="liip_imagine.extension_guesser" alias="mime_types" />

(Or, those two service IDs could just be completely deprecated and the core mime_types service be used directly going forward)

The compiler pass is needed to handle cross-version compatibility while Symfony 3.4 is still supported. Essentially, once you get to a newer minimum Symfony version, there's no need for the compiler pass because you can set it directly in the XML and not need any conditional changes later.

@dbu dbu added this to the 2.7 milestone Oct 6, 2021
@dbu dbu merged commit 2af37d4 into liip:2.x Oct 6, 2021
dbu added a commit that referenced this pull request Oct 6, 2021
@mbabker mbabker deleted the bugfix-1287 branch October 6, 2021 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
State: Need To Investigate 🔎 This item requires additional investigation to determine a resolution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incompatibility with Symfony 5: Usage of class that does not exist anymore
3 participants