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 Symfony 6.2 deprecation #303

Merged
merged 2 commits into from
Apr 24, 2023
Merged

Fix Symfony 6.2 deprecation #303

merged 2 commits into from
Apr 24, 2023

Conversation

franmomu
Copy link
Contributor

@franmomu franmomu commented Jan 9, 2023

When using the bundle with Symfony 6.2 there is this deprecation warning:

The "Presta\SitemapBundle\Messenger\DumpSitemapMessageHandler" class implements "Symfony\Component\Messenger\Handler\MessageHandlerInterface" that is deprecated since Symfony 6.2, use the {@see AsMessageHandler} attribute instead.

AFAIK the MessageHandlerInterface is just a marker to tell Symfony to autoconfigure the class, but since it's already defined and configured in

<service id="presta_sitemap.messenger.message_handler" class="Presta\SitemapBundle\Messenger\DumpSitemapMessageHandler">
<argument type="service" id="router" />
<argument type="service" id="presta_sitemap.dumper" />
<argument>%presta_sitemap.dump_directory%</argument>
<tag name="messenger.message_handler" />
</service>

I think the interface is not necessary.

Technically this is a BC break, but I guess this class is not meant to be extended by the user.

Update: Following #303 (comment), it is resolved by adding handles="Presta\SitemapBundle\Messenger\DumpSitemapMessage", there is no need to remove the interface for now.

@norkunas
Copy link
Contributor

Maybe we should add handles="Presta\SitemapBundle\Messenger\DumpSitemapMessage" as Symfony does in it's core configurations?

@norkunas
Copy link
Contributor

norkunas commented Jan 17, 2023

Edit: oh because ecd355f was not released yet..

Also I saw another two deprecation logs but they says the same:

The "Symfony\Component\Console\Command\Command::$defaultName" property is considered final. You should not override it in "Presta\SitemapBundle\Command\DumpSitemapsCommand".

Since symfony/console 6.1: Relying on the static property "$defaultName" for setting a command name is deprecated. Add the "Symfony\Component\Console\Attribute\AsCommand" attribute to the "Presta\SitemapBundle\Command\DumpSitemapsCommand" class instead.

@franmomu
Copy link
Contributor Author

Maybe we should add handles="Presta\SitemapBundle\Messenger\DumpSitemapMessage" as Symfony does in it's core configurations?

Apparently it's not necessary: https://symfony.com/doc/current/messenger.html#manually-configuring-handlers

<service id="App\MessageHandler\SmsNotificationHandler">
     <!-- handles is only needed if it can't be guessed by type-hint -->
     <tag name="messenger.message_handler"
          handles="App\Message\SmsNotification"/>
</service>

but if they have it I don't know why 🤷‍♂️

@norkunas
Copy link
Contributor

When it is declared Symfony does not need to guess what this handler handles, so imho from perf side it would be better even it's unnoticeable

@franmomu
Copy link
Contributor Author

When it is declared Symfony does not need to guess what this handler handles, so imho from perf side it would be better even it's unnoticeable

There is no need then to remove the interface 👍

@maho125

This comment was marked as duplicate.

@norkunas
Copy link
Contributor

Could this get merged? 😊

@yann-eugone
Copy link
Member

Will fix #307

@yann-eugone
Copy link
Member

I will take some time to see if something more is required for us to release it, but trying to do it ASAP

@yann-eugone yann-eugone merged commit 61539b0 into prestaconcept:3.x Apr 24, 2023
@yann-eugone
Copy link
Member

@norkunas
Copy link
Contributor

norkunas commented May 3, 2023

The deprecation about MessageHandlerInterface is still triggered. I guess we should conditionally create the class based on symfony version something like:

if (Kernel::VERSION_ID >= 60200) {
    class DumpSitemapMessageHandler {}
} else {
    class DumpSitemapMessageHandler implements MessageHandlerInterface {}
}

to avoid triggering it. or check if we need to implement it at all.

@yann-eugone
Copy link
Member

Here we go again with these ugly solutions ^^
I'm wondering whether it is actually BC ?
If we remove the interface, but it is still working on EVERY symfony version we support, it should be OK right ?
This code was never intended to be used directly, or extended, or...
I believe that if someone has some static analysis tools plugged on, relying on this interface, he should be pissed we removed the interface, but does that exists anyway ?
Do we have examples on how the community has handled these changes ?

@norkunas
Copy link
Contributor

norkunas commented May 3, 2023

I leaning to think that handles part in config is enough and should work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants