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

Resolve bundle deprecations #317

Merged
merged 5 commits into from
Oct 9, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 5 additions & 16 deletions src/Event/SitemapAddUrlEvent.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

namespace Presta\SitemapBundle\Event;

use LogicException;
use Presta\SitemapBundle\Routing\RouteOptionParser;
use Presta\SitemapBundle\Sitemap\Url\Url;
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
Expand All @@ -28,12 +27,6 @@
*/
class SitemapAddUrlEvent extends Event
{
/**
* @Event("Presta\SitemapBundle\Event\SitemapAddUrlEvent")
* @deprecated since presta/sitemap-bundle 3.3, use `SitemapAddUrlEvent::class` instead.
*/
public const NAME = 'presta_sitemap.add_url';

/**
* @var bool
*/
Expand All @@ -55,16 +48,16 @@ class SitemapAddUrlEvent extends Event
private $options;

/**
* @var UrlGeneratorInterface|null
* @var UrlGeneratorInterface
*/
protected $urlGenerator;

/**
* @param string $route
* @param RouteOptions $options
* @param UrlGeneratorInterface|null $urlGenerator
* @param string $route
* @param RouteOptions $options
* @param UrlGeneratorInterface $urlGenerator
*/
public function __construct(string $route, array $options, UrlGeneratorInterface $urlGenerator = null)
public function __construct(string $route, array $options, UrlGeneratorInterface $urlGenerator)
{
$this->route = $route;
$this->options = $options;
Expand Down Expand Up @@ -139,10 +132,6 @@ public function getOptions(): array

public function getUrlGenerator(): UrlGeneratorInterface
{
if (!$this->urlGenerator) {
throw new LogicException('UrlGenerator was not set.');
}

return $this->urlGenerator;
}
}
21 changes: 5 additions & 16 deletions src/Event/SitemapPopulateEvent.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

namespace Presta\SitemapBundle\Event;

use LogicException;
use Presta\SitemapBundle\Service\UrlContainerInterface;
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
use Symfony\Contracts\EventDispatcher\Event;
Expand All @@ -24,12 +23,6 @@
*/
class SitemapPopulateEvent extends Event
{
/**
* @Event("Presta\SitemapBundle\Event\SitemapPopulateEvent")
* @deprecated since presta/sitemap-bundle 3.3, use `SitemapPopulateEvent::class` instead.
*/
public const ON_SITEMAP_POPULATE = 'presta_sitemap.populate';

/**
* @var UrlContainerInterface
*/
Expand All @@ -42,19 +35,19 @@ class SitemapPopulateEvent extends Event
protected $section;

/**
* @var UrlGeneratorInterface|null
* @var UrlGeneratorInterface
*/
protected $urlGenerator;

/**
* @param UrlContainerInterface $urlContainer
* @param string|null $section
* @param UrlGeneratorInterface|null $urlGenerator
* @param UrlContainerInterface $urlContainer
* @param string|null $section
* @param UrlGeneratorInterface $urlGenerator
*/
public function __construct(
UrlContainerInterface $urlContainer,
string $section = null,
UrlGeneratorInterface $urlGenerator = null
UrlGeneratorInterface $urlGenerator
Copy link
Member

Choose a reason for hiding this comment

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

Woops, we also missed this one 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the catch

) {
$this->urlContainer = $urlContainer;
$this->section = $section;
Expand All @@ -81,10 +74,6 @@ public function getSection(): ?string

public function getUrlGenerator(): UrlGeneratorInterface
{
if (!$this->urlGenerator) {
throw new LogicException('UrlGenerator was not set.');
}

return $this->urlGenerator;
}
}
4 changes: 2 additions & 2 deletions src/EventListener/RouteAnnotationEventListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
use Symfony\Component\Routing\RouterInterface;

/**
* Listen to "presta_sitemap.populate" event.
* Listen to {@see SitemapPopulateEvent} event.
* Populate sitemap with configured static routes.
*
* @phpstan-import-type RouteOptions from RouteOptionParser
Expand Down Expand Up @@ -84,7 +84,7 @@ private function addUrlsFromRoutes(UrlContainerInterface $container, ?string $se
}

$event = new SitemapAddUrlEvent($name, $options, $this->router);
$this->dispatcher->dispatch($event, SitemapAddUrlEvent::NAME);
$this->dispatcher->dispatch($event);

if (!$event->shouldBeRegistered()) {
continue;
Expand Down
2 changes: 1 addition & 1 deletion src/EventListener/StaticRoutesAlternateEventListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;

/**
* Listen to "presta_sitemap.add_url" event.
* Listen to {@see SitemapAddUrlEvent} event.
* Decorate translatable Url with multi-lang alternatives.
* Support both Symfony translated routes & JMSI18nRoutingBundle.
*
Expand Down
20 changes: 0 additions & 20 deletions src/PrestaSitemapBundle.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@

namespace Presta\SitemapBundle;

use Presta\SitemapBundle\DependencyInjection\Compiler\EventAliasMappingPass;
use Presta\SitemapBundle\Event\SitemapAddUrlEvent;
use Presta\SitemapBundle\Event\SitemapPopulateEvent;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\EventDispatcher\DependencyInjection\AddEventAliasesPass;
use Symfony\Component\HttpKernel\Bundle\Bundle;

/**
Expand All @@ -25,21 +20,6 @@
*/
class PrestaSitemapBundle extends Bundle
{
/**
* @inheritdoc
*
* @return void
*/
public function build(ContainerBuilder $container)
{
parent::build($container);

$container->addCompilerPass(new AddEventAliasesPass([
SitemapAddUrlEvent::class => SitemapAddUrlEvent::NAME,
SitemapPopulateEvent::class => SitemapPopulateEvent::ON_SITEMAP_POPULATE,
]));
}

/**
* @inheritDoc
*/
Expand Down
18 changes: 3 additions & 15 deletions src/Service/AbstractGenerator.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ abstract class AbstractGenerator implements UrlContainerInterface
protected $itemsBySet;

/**
* @var UrlGeneratorInterface|null
* @var UrlGeneratorInterface
*/
protected $urlGenerator;

Expand All @@ -62,23 +62,11 @@ abstract class AbstractGenerator implements UrlContainerInterface
*/
private $defaults;

/**
* @param EventDispatcherInterface $dispatcher
* @param int|null $itemsBySet
* @param UrlGeneratorInterface|null $urlGenerator
*/
public function __construct(
EventDispatcherInterface $dispatcher,
int $itemsBySet = null,
UrlGeneratorInterface $urlGenerator = null
UrlGeneratorInterface $urlGenerator
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ you forgot to reorder arguments here

) {
if (!$urlGenerator) {
@trigger_error(
'Not injecting the $urlGenerator is deprecated and will be required in 4.0.',
\E_USER_DEPRECATED
);
}

$this->dispatcher = $dispatcher;
// We add one to LIMIT_ITEMS because it was used as an index, not a quantity
$this->itemsBySet = ($itemsBySet === null) ? Sitemapindex::LIMIT_ITEMS + 1 : $itemsBySet;
Expand Down Expand Up @@ -169,7 +157,7 @@ protected function populate(string $section = null): void
{
$event = new SitemapPopulateEvent($this, $section, $this->urlGenerator);

$this->dispatcher->dispatch($event, SitemapPopulateEvent::ON_SITEMAP_POPULATE);
$this->dispatcher->dispatch($event);
}

/**
Expand Down
48 changes: 18 additions & 30 deletions tests/Unit/EventListener/RouteAnnotationEventListenerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,7 @@ class RouteAnnotationEventListenerTest extends TestCase
*/
public function testPopulateSitemap(?string $section, array $routes, array $urls): void
{
$urlContainer = new InMemoryUrlContainer();
$event = new SitemapPopulateEvent($urlContainer, $section);
$dispatcher = new EventDispatcher();
$this->dispatch($dispatcher, $event, $routes);
$urlContainer = $this->dispatch($section, $routes);

// ensure that all expected section were created but not more than expected
self::assertEquals(\array_keys($urls), $urlContainer->getSections());
Expand Down Expand Up @@ -63,36 +60,18 @@ public function testPopulateSitemap(?string $section, array $routes, array $urls
*/
public function testEventListenerCanPreventUrlFromBeingAddedToSitemap(?string $section, array $routes): void
{
$dispatcher = new EventDispatcher();
$dispatcher->addListener(
SitemapAddUrlEvent::NAME,
function (SitemapAddUrlEvent $event): void {
$event->preventRegistration();
}
);

$urlContainer = new InMemoryUrlContainer();
$event = new SitemapPopulateEvent($urlContainer, $section);

$this->dispatch($dispatcher, $event, $routes);
$urlContainer = $this->dispatch($section, $routes, function (SitemapAddUrlEvent $event): void {
$event->preventRegistration();
});

self::assertEmpty($urlContainer->getSections());
}

public function testEventListenerCanSetUrl(): void
{
$dispatcher = new EventDispatcher();
$dispatcher->addListener(
SitemapAddUrlEvent::NAME,
function (SitemapAddUrlEvent $event): void {
$event->setUrl(new UrlConcrete('http://localhost/redirect'));
}
);

$urlContainer = new InMemoryUrlContainer();
$event = new SitemapPopulateEvent($urlContainer, null);

$this->dispatch($dispatcher, $event, [['home', '/', true]]);
$urlContainer = $this->dispatch(null, [['home', '/', true]], function (SitemapAddUrlEvent $event): void {
$event->setUrl(new UrlConcrete('http://localhost/redirect'));
});

$urlset = $urlContainer->getUrlset('default');
self::assertCount(1, $urlset);
Expand Down Expand Up @@ -132,8 +111,13 @@ public function routes(): \Generator
];
}

private function dispatch(EventDispatcher $dispatcher, SitemapPopulateEvent $event, array $routes): void
private function dispatch(?string $section, array $routes, ?\Closure $listener = null): InMemoryUrlContainer
{
$dispatcher = new EventDispatcher();
if ($listener !== null) {
$dispatcher->addListener(SitemapAddUrlEvent::class, $listener);
}

$router = new Router(
new ClosureLoader(),
static function () use ($routes): RouteCollection {
Expand All @@ -147,9 +131,13 @@ static function () use ($routes): RouteCollection {
['resource_type' => 'closure']
);

$urlContainer = new InMemoryUrlContainer();
$listener = new RouteAnnotationEventListener($router, $dispatcher, 'default');
$dispatcher->addListener(SitemapPopulateEvent::class, [$listener, 'registerRouteAnnotation']);
$dispatcher->dispatch($event, SitemapPopulateEvent::class);
$event = new SitemapPopulateEvent($urlContainer, $section, $router);
$dispatcher->dispatch($event);

return $urlContainer;
}

private function findUrl(array $urlset, string $loc): ?UrlConcrete
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ private function dispatch(array $listenerOptions, string $route, array $options
$dispatcher = new EventDispatcher();
$dispatcher->addListener(SitemapAddUrlEvent::class, [$listener, 'addAlternate']);

$event = new SitemapAddUrlEvent($route, $options);
$dispatcher->dispatch($event, SitemapAddUrlEvent::class);
$event = new SitemapAddUrlEvent($route, $options, $this->router);
$dispatcher->dispatch($event);

return $event;
}
Expand Down
16 changes: 8 additions & 8 deletions tests/Unit/Service/DumperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,10 @@ public function testFromScratch(?string $section, bool $gzip): void
$hasIndex = $hasDefaultSection || $hasBlogSection;

if ($hasDefaultSection) {
$this->eventDispatcher->addListener(SitemapPopulateEvent::ON_SITEMAP_POPULATE, self::defaultListener());
$this->eventDispatcher->addListener(SitemapPopulateEvent::class, self::defaultListener());
}
if ($hasBlogSection) {
$this->eventDispatcher->addListener(SitemapPopulateEvent::ON_SITEMAP_POPULATE, self::blogListener());
$this->eventDispatcher->addListener(SitemapPopulateEvent::class, self::blogListener());
}

self::assertEmpty(\glob(self::DUMP_DIR . '/*'), 'Sitemap is empty before test');
Expand All @@ -104,8 +104,8 @@ public function fromScratch(): \Generator
*/
public function testIncremental(bool $gzip): void
{
$this->eventDispatcher->addListener(SitemapPopulateEvent::ON_SITEMAP_POPULATE, self::defaultListener());
$this->eventDispatcher->addListener(SitemapPopulateEvent::ON_SITEMAP_POPULATE, self::blogListener());
$this->eventDispatcher->addListener(SitemapPopulateEvent::class, self::defaultListener());
$this->eventDispatcher->addListener(SitemapPopulateEvent::class, self::blogListener());

self::assertEmpty(\glob(self::DUMP_DIR . '/*'), 'Sitemap is empty before test');

Expand All @@ -126,7 +126,7 @@ public function incremental(): \Generator

public function testDirCreated(): void
{
$this->eventDispatcher->addListener(SitemapPopulateEvent::ON_SITEMAP_POPULATE, self::defaultListener());
$this->eventDispatcher->addListener(SitemapPopulateEvent::class, self::defaultListener());

self::removeDir();

Expand All @@ -141,7 +141,7 @@ public function testDirCreated(): void
public function testExistingInvalidSitemap(string $index): void
{
$this->expectException(\InvalidArgumentException::class);
$this->eventDispatcher->addListener(SitemapPopulateEvent::ON_SITEMAP_POPULATE, self::defaultListener());
$this->eventDispatcher->addListener(SitemapPopulateEvent::class, self::defaultListener());

\file_put_contents(self::DUMP_DIR . '/sitemap.xml', $index);
$this->dumper->dump(self::DUMP_DIR, 'https://acme.org', 'default');
Expand All @@ -154,7 +154,7 @@ public function testRouterInjectedIntoEvent(): void
$eventRouter = $event->getUrlGenerator();
};

$this->eventDispatcher->addListener(SitemapPopulateEvent::ON_SITEMAP_POPULATE, $listener);
$this->eventDispatcher->addListener(SitemapPopulateEvent::class, $listener);

$this->dumper->dump(self::DUMP_DIR, 'https://acme.org', 'default');

Expand All @@ -165,7 +165,7 @@ public function testErrorInListener(): void
{
$this->expectException(\Exception::class);
$this->eventDispatcher->addListener(
SitemapPopulateEvent::ON_SITEMAP_POPULATE,
SitemapPopulateEvent::class,
self::errorListener(new Exception('Throw on Unit Test'))
);

Expand Down
4 changes: 2 additions & 2 deletions tests/Unit/Service/GeneratorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public function testFetch(): void
self::assertEquals($event->getSection(), 'foo');
$triggered = true;
};
$this->eventDispatcher->addListener(SitemapPopulateEvent::ON_SITEMAP_POPULATE, $listener);
$this->eventDispatcher->addListener(SitemapPopulateEvent::class, $listener);

$generator->fetch('foo');
self::assertTrue($triggered, 'Event listener was triggered');
Expand All @@ -73,7 +73,7 @@ public function testRouterInjectedIntoEvent(): void
$eventRouter = $event->getUrlGenerator();
};

$this->eventDispatcher->addListener(SitemapPopulateEvent::ON_SITEMAP_POPULATE, $listener);
$this->eventDispatcher->addListener(SitemapPopulateEvent::class, $listener);

$this->generator()->fetch('foo');

Expand Down