Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"ibexa/rest": "~6.0.x-dev",
"php-http/curl-client": "^2.1",
"psr/http-client": "^1.0",
"psr/log": "^3.0",
"symfony/config": "^7.4",
"symfony/dependency-injection": "^7.4",
"symfony/event-dispatcher": "^7.4",
Expand Down
24 changes: 0 additions & 24 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -6,30 +6,6 @@ parameters:
count: 1
path: spec/EventSubscriber/HiddenLocationExceptionSubscriberSpec.php

-
message: '#^Call to an undefined method Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\ContentInfo\:\:willReturn\(\)\.$#'
identifier: method.notFound
count: 1
path: spec/EventSubscriber/HiddenLocationExceptionSubscriberSpec.php

-
message: '#^Call to an undefined method Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\Location\:\:shouldHaveBeenCalled\(\)\.$#'
identifier: method.notFound
count: 1
path: spec/EventSubscriber/HiddenLocationExceptionSubscriberSpec.php

-
message: '#^Call to an undefined method Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\Location\:\:willReturn\(\)\.$#'
identifier: method.notFound
count: 1
path: spec/EventSubscriber/HiddenLocationExceptionSubscriberSpec.php

-
message: '#^Call to an undefined method Ibexa\\Core\\MVC\\Exception\\HiddenLocationException\:\:getWrappedObject\(\)\.$#'
identifier: method.notFound
count: 1
path: spec/EventSubscriber/HiddenLocationExceptionSubscriberSpec.php

-
message: '#^Call to an undefined method Symfony\\Component\\HttpFoundation\\Request\:\:getWrappedObject\(\)\.$#'
identifier: method.notFound
Expand Down
76 changes: 47 additions & 29 deletions spec/EventSubscriber/HiddenLocationExceptionSubscriberSpec.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,71 +7,89 @@

namespace spec\Ibexa\HttpCache\EventSubscriber;

use Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo;
use Ibexa\Contracts\Core\Repository\Values\Content\Location;
use FOS\HttpCache\ResponseTagger as FosResponseTagger;
use Ibexa\Core\MVC\Exception\HiddenLocationException;
use Ibexa\Core\Repository\Values\Content\Location;
use Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo;
use Ibexa\HttpCache\ResponseTagger\Value\ContentInfoTagger;
use Ibexa\HttpCache\ResponseTagger\Value\LocationTagger;
use PhpSpec\Exception\Example\FailureException;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Event\ExceptionEvent;
use Symfony\Component\HttpKernel\HttpKernelInterface;

class HiddenLocationExceptionSubscriberSpec extends ObjectBehavior
{
public function let(
LocationTagger $locationTagger,
ContentInfoTagger $contentInfoTagger
): void {
$this->beConstructedWith($locationTagger, $contentInfoTagger);
}

public function it_tags_on_hidden_location_exception(
HttpKernelInterface $kernel,
Request $request,
LocationTagger $locationTagger,
ContentInfoTagger $contentInfoTagger,
Location $location,
ContentInfo $contentInfo,
HiddenLocationException $exception
Request $request
): void {
$exception->getLocation()->willReturn($location);
$location->getContentInfo()->willReturn($contentInfo);
$locationTagger->tag($location)->willReturn($locationTagger);
$contentInfoTagger->tag($contentInfo)->willReturn($contentInfoTagger);
$locationResponseTagger = new FosResponseTagger();
$contentInfoResponseTagger = new FosResponseTagger();
$this->beConstructedWith(
new LocationTagger($locationResponseTagger),
new ContentInfoTagger($contentInfoResponseTagger)
);

$contentInfo = new ContentInfo([
'id' => 321,
'contentTypeId' => 987,
'mainLocationId' => null,
]);
$location = new Location([
'id' => 123,
'parentLocationId' => 2,
'pathString' => '/1/2/123/',
'contentInfo' => $contentInfo,
]);
$exception = new HiddenLocationException($location);

$event = new ExceptionEvent(
$kernel->getWrappedObject(),
$request->getWrappedObject(),
HttpKernelInterface::MAIN_REQUEST,
$exception->getWrappedObject()
$exception
);

$this->tagHiddenLocationExceptionResponse($event);

$exception->getLocation()->shouldHaveBeenCalled();
$locationTagger->tag(Argument::type(Location::class))->shouldHaveBeenCalled();
$contentInfoTagger->tag(Argument::type(ContentInfo::class))->shouldHaveBeenCalled();
if (!$locationResponseTagger->hasTags()) {
throw new FailureException('LocationTagger did not add any tags.');
}

if (!$contentInfoResponseTagger->hasTags()) {
throw new FailureException('ContentInfoTagger did not add any tags.');
}
}

public function it_does_not_tag_on_other_exceptions(
HttpKernelInterface $kernel,
Request $request,
LocationTagger $locationTagger,
ContentInfoTagger $contentInfoTagger,
\Exception $exception
): void {
$locationResponseTagger = new FosResponseTagger();
$contentInfoResponseTagger = new FosResponseTagger();
$this->beConstructedWith(
new LocationTagger($locationResponseTagger),
new ContentInfoTagger($contentInfoResponseTagger)
);

$event = new ExceptionEvent(
$kernel->getWrappedObject(),
$request->getWrappedObject(),
HttpKernelInterface::MAIN_REQUEST,
$exception->getWrappedObject()
);

$locationTagger->tag(Argument::type(Location::class))->shouldNotBeCalled();
$contentInfoTagger->tag(Argument::type(ContentInfo::class))->shouldNotBeCalled();

$this->tagHiddenLocationExceptionResponse($event);

if ($locationResponseTagger->hasTags()) {
throw new FailureException('LocationTagger should not add tags for unrelated exceptions.');
}

if ($contentInfoResponseTagger->hasTags()) {
throw new FailureException('ContentInfoTagger should not add tags for unrelated exceptions.');
}
}
}
2 changes: 2 additions & 0 deletions src/bundle/Resources/config/view_cache.yml
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ services:
# Taggers are added by a compiler pass
arguments:
- []
- '@logger'
- '%kernel.debug%'

# Alias for ResponseTagger interface => DispatcherTagger
Ibexa\Contracts\HttpCache\ResponseTagger\ResponseTagger: '@Ibexa\HttpCache\ResponseTagger\Delegator\DispatcherTagger'
Expand Down
2 changes: 2 additions & 0 deletions src/contracts/ResponseTagger/ResponseTagger.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
*/
interface ResponseTagger
{
public function supports(mixed $value): bool;

/**
* Extracts tags from a value.
*
Expand Down
3 changes: 1 addition & 2 deletions src/lib/ResponseTagger/Delegator/ContentValueViewTagger.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@
use Ibexa\Contracts\Core\Repository\Values\Content\Content;
use Ibexa\Contracts\HttpCache\ResponseTagger\ResponseTagger;
use Ibexa\Core\MVC\Symfony\View\ContentValueView;
use Ibexa\HttpCache\ResponseTagger\Value\AbstractValueTagger;

class ContentValueViewTagger extends AbstractValueTagger
final readonly class ContentValueViewTagger implements ResponseTagger
{
public function __construct(private readonly ResponseTagger $contentInfoTagger)
{
Expand Down
44 changes: 32 additions & 12 deletions src/lib/ResponseTagger/Delegator/DispatcherTagger.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
namespace Ibexa\HttpCache\ResponseTagger\Delegator;

use Ibexa\Contracts\HttpCache\ResponseTagger\ResponseTagger;
use Psr\Log\LoggerInterface;
use Psr\Log\NullLogger;

/**
* Dispatches a value to all registered ResponseTaggers.
Expand All @@ -17,27 +19,31 @@
/**
* @param iterable<\Ibexa\Contracts\HttpCache\ResponseTagger\ResponseTagger> $taggers
*/
public function __construct(private iterable $taggers = [])
public function __construct(
private iterable $taggers = [],
private LoggerInterface $logger = new NullLogger(),
private bool $debug = false,
) {
}

public function supports(mixed $value): bool
{
return true;
}

public function tag(mixed $value): void
{
$handled = false;
foreach ($this->taggers as $tagger) {
if (method_exists($tagger, 'supports')) {
if ($tagger->supports($value)) {
$tagger->tag($value);
}
} else {
trigger_deprecation(
'ibexa/http-cache',
'5.0.7',
'%s does not implement supports(). This will be required in 6.0, supports() will be a part of ResponseTagger interface',
get_debug_type($tagger),
);
if ($tagger->supports($value)) {

@mnocon mnocon Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Going back the initial idea that pushed us towards these changes - maybe we throw an exception* if there's no tagger supporting given value? As this must be a mistake.

The origial report said that if you pass a Content object (instead of ContentInfo) to be tagged, nothing happens and you get no information that something is wrong.

*or log a warning, or log a warning in dev mode, or something else

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I went with your "log a warning" idea, but split it by environment:

debug mode (dev/test) → throw an InvalidArgumentException, so the mistake surfaces immediately, including in the test suite
prod → log a warning instead

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me at least.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great to me, thank you! 🙇

$tagger->tag($value);
$handled = true;
}
}

if (!$handled) {
$this->handleUnsupportedValue($value);
}
}

public function __toString(): string
Expand All @@ -52,4 +58,18 @@ public function __toString(): string

return sprintf('Available response taggers are: %s', $taggers);
}

private function handleUnsupportedValue(mixed $value): void
{
$message = sprintf(
'No response tagger supports value of type "%s"; no cache tags were added.',
get_debug_type($value),
);

if ($this->debug) {
throw new \InvalidArgumentException($message);
}

$this->logger->warning($message, ['taggers' => (string)$this]);
}
}
3 changes: 1 addition & 2 deletions src/lib/ResponseTagger/Delegator/LocationValueViewTagger.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@
use Ibexa\Contracts\Core\Repository\Values\Content\Location;
use Ibexa\Contracts\HttpCache\ResponseTagger\ResponseTagger;
use Ibexa\Core\MVC\Symfony\View\LocationValueView;
use Ibexa\HttpCache\ResponseTagger\Value\AbstractValueTagger;

class LocationValueViewTagger extends AbstractValueTagger
final readonly class LocationValueViewTagger implements ResponseTagger
{
public function __construct(private readonly ResponseTagger $locationTagger)
{
Expand Down
21 changes: 0 additions & 21 deletions src/lib/ResponseTagger/Value/AbstractValueTagger.php

This file was deleted.

11 changes: 7 additions & 4 deletions src/lib/ResponseTagger/Value/ContentInfoTagger.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,17 @@

namespace Ibexa\HttpCache\ResponseTagger\Value;

use FOS\HttpCache\ResponseTagger as FosResponseTagger;
use Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo;
use Ibexa\Contracts\HttpCache\Handler\ContentTagInterface;
use Ibexa\Contracts\HttpCache\ResponseTagger\ResponseTagger;

/**
* @final
*/
class ContentInfoTagger extends AbstractValueTagger
final readonly class ContentInfoTagger implements ResponseTagger
{
public function __construct(private readonly FosResponseTagger $responseTagger)
{
}

public function supports(mixed $value): bool
{
return $value instanceof ContentInfo;
Expand Down
11 changes: 7 additions & 4 deletions src/lib/ResponseTagger/Value/LocationTagger.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,17 @@

namespace Ibexa\HttpCache\ResponseTagger\Value;

use FOS\HttpCache\ResponseTagger as FosResponseTagger;
use Ibexa\Contracts\Core\Repository\Values\Content\Location;
use Ibexa\Contracts\HttpCache\Handler\ContentTagInterface;
use Ibexa\Contracts\HttpCache\ResponseTagger\ResponseTagger;

/**
* @final
*/
class LocationTagger extends AbstractValueTagger
final readonly class LocationTagger implements ResponseTagger
{
public function __construct(private readonly FosResponseTagger $responseTagger)
{
}

public function supports(mixed $value): bool
{
return $value instanceof Location;
Expand Down
Loading
Loading