Fix Symfony 6.2 deprecation

[franmomu]

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

https://github.com/prestaconcept/PrestaSitemapBundle/blob/61e6c07606e4a52dfd76a0b754b07e3cdd50e31a/config/messenger.xml#L7-L12

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 https://github.com/prestaconcept/PrestaSitemapBundle/pull/303#issuecomment-1384860212, it is resolved by adding handles="Presta\SitemapBundle\Messenger\DumpSitemapMessage", there is no need to remove the interface for now.

[norkunas]

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

[norkunas]

Edit: oh because ecd355fbfa0e2201ffa7625673ffcdffda336756 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]

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]

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]

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]

I got another deprecation with symfony v6.2.6:

[2023-02-17T23:28:29.241934+00:00] deprecation.INFO: User Deprecated: 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. {"exception":"[object] (ErrorException(code: 0): User Deprecated: 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. at /var/www/coupodo.com/html/vendor/symfony/console/Command/Command.php:85)"} []

Here is fix for it:

`<?php

/*

• This file is part of the PrestaSitemapBundle package.
• (c) PrestaConcept https://prestaconcept.net
• For the full copyright and license information, please view the LICENSE
• file that was distributed with this source code. */

namespace Presta\SitemapBundle\Command;

use Presta\SitemapBundle\Service\DumperInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\Routing\RouterInterface; use Symfony\Component\Console\Attribute\AsCommand;

/**

• Command to dump the sitemaps to provided directory */

  [AsCommand(name: 'presta:sitemaps:dump')]

  class DumpSitemapsCommand extends Command { /**

  • @var RouterInterface */ private $router;

  /**

  • @var DumperInterface */ private $dumper;

  /**

  • @var string */ private $defaultTarget;

  public function __construct(RouterInterface $router, DumperInterface $dumper, string $defaultTarget) { $this->router = $router; $this->dumper = $dumper; $this->defaultTarget = $defaultTarget;

  parent::__construct();

  }

  /**

  • @inheritdoc */ protected function configure(): void { $this ->setDescription('Dumps sitemaps to given location') ->addOption( 'section', null, InputOption::VALUE_REQUIRED, 'Name of sitemap section to dump, all sections are dumped by default' ) ->addOption( 'base-url', null, InputOption::VALUE_REQUIRED, 'Base url to use for absolute urls. Good example - http://acme.com/, bad example - acme.com.' . ' Defaults to router.request_context.host parameter' ) ->addOption( 'gzip', null, InputOption::VALUE_NONE, 'Gzip sitemap' ) ->addArgument( 'target', InputArgument::OPTIONAL, 'Location where to dump sitemaps. Generated urls will not be related to this folder.', $this->defaultTarget ); }

  /**

  • @inheritdoc */ protected function execute(InputInterface $input, OutputInterface $output): int { /* @var string $targetDir / $targetDir = $input->getArgument('target'); $targetDir = rtrim($targetDir, '/');

    /* @var string|null $baseUrl / $baseUrl = $input->getOption('base-url'); if ($baseUrl) { $baseUrl = rtrim($baseUrl, '/') . '/';

    //sanity check
    if (!parse_url($baseUrl, PHP_URL_HOST)) {
        throw new \InvalidArgumentException(
            'Invalid base url. Use fully qualified base url, e.g. http://acme.com/',
            -1
        );
    }
    
    // Set Router's host used for generating URLs from configuration param
    // There is no other way to manage domain in CLI
    $request = Request::create($baseUrl);
    $this->router->getContext()->fromRequest($request);

    } else { $baseUrl = $this->getBaseUrl(); }

    /* @var string|null $section / $section = $input->getOption('section'); if ($section) { $output->writeln( sprintf( "Dumping sitemaps section %s into %s directory", $section, $targetDir ) ); } else { $output->writeln( sprintf( "Dumping all sections of sitemaps into %s directory", $targetDir ) ); }

    $options = [ 'gzip' => (bool)$input->getOption('gzip'), ]; $filenames = $this->dumper->dump($targetDir, $baseUrl, $section, $options);

    if (!is_array($filenames)) { $output->writeln( "No URLs were added to sitemap by EventListeners" . " - this may happen when provided section is invalid" );

    return 1;

    }

    $output->writeln("Created/Updated the following sitemap files:"); foreach ($filenames as $filename) { $output->writeln(" $filename"); }

    return 0; }

  private function getBaseUrl(): string { $context = $this->router->getContext();

  if ('' === $host = $context->getHost()) {
      throw new \RuntimeException(
          'Router host must be configured to be able to dump the sitemap, please see documentation.'
      );
  }
  
  $scheme = $context->getScheme();
  $port = '';
  
  if ('http' === $scheme && 80 != $context->getHttpPort()) {
      $port = ':' . $context->getHttpPort();
  } elseif ('https' === $scheme && 443 != $context->getHttpsPort()) {
      $port = ':' . $context->getHttpsPort();
  }
  
  return rtrim($scheme . '://' . $host . $port, '/') . '/';

  } }`

[norkunas]

Could this get merged? :blush:

[yann-eugone]

Will fix https://github.com/prestaconcept/PrestaSitemapBundle/issues/307

[yann-eugone]

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]

Released as of https://github.com/prestaconcept/PrestaSitemapBundle/releases/tag/v3.3.1

[norkunas]

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]

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]

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