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

Validate shortcode controllername service configurations (Case 173693) #40

Merged
merged 14 commits into from
Oct 30, 2024
Merged
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
composer.lock
vendor/
phpunit.xml
.phpunit.cache/
.phpunit.result.cache
.php-cs-fixer.cache
tests/Fixtures/cache/
29 changes: 23 additions & 6 deletions src/Handler/EmbeddedShortcodeHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Webfactory\ShortcodeBundle\Handler;

use InvalidArgumentException;
use Psr\Log\LoggerInterface;
use Psr\Log\NullLogger;
use Symfony\Component\HttpFoundation\RequestStack;
Expand Down Expand Up @@ -33,17 +34,15 @@ class EmbeddedShortcodeHandler
/** @var RequestStack */
private $requestStack;

/**
* @param string $controllerName
* @param string $renderer
*/
public function __construct(
FragmentHandler $fragmentHandler,
$controllerName,
$renderer,
string $controllerName,
string $renderer,
RequestStack $requestStack,
?LoggerInterface $logger = null
) {
$this->validateControllerName($controllerName);

$this->fragmentHandler = $fragmentHandler;
$this->controllerName = $controllerName;
$this->renderer = $renderer;
Expand Down Expand Up @@ -90,4 +89,22 @@ public function getControllerName(): string
{
return $this->controllerName;
}

private function validateControllerName(string $controllerName): void
{
if (class_exists($controllerName)) {
// Check with method_exists instead of is_callable, because is_callable would need an object instance to
// positively test an invokable classes
if (method_exists($controllerName, '__invoke')) {
return;
}

throw new InvalidArgumentException('The configured controller "'.$controllerName.'" does not refer a method. Although a class "'.$controllerName.'" exists, but has no __invoke method.');
}

$callableFragments = explode('::', $controllerName);
if (!\is_array($callableFragments) || 2 !== \count($callableFragments) || !method_exists($callableFragments[0], $callableFragments[1])) {
throw new InvalidArgumentException('The controller method: "'.$controllerName.'" does not exist.');
}
}
}
13 changes: 13 additions & 0 deletions tests/Fixtures/Controller/InvokableShortcodeTestController.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php

namespace Webfactory\ShortcodeBundle\Tests\Fixtures\Controller;

use Symfony\Component\HttpFoundation\Response;

final class InvokableShortcodeTestController
{
public function __invoke(): Response
{
return new Response('invokable-controller-response');
}
}
2 changes: 1 addition & 1 deletion tests/Fixtures/config/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ webfactory_shortcode:
test-config-esi:
controller: 'Webfactory\ShortcodeBundle\Tests\Fixtures\Controller\ShortcodeTestController::test'
method: esi
test-config-invokable: 'Webfactory\ShortcodeBundle\Tests\Fixtures\Controller\InvokableShortcodeTestController'
test-shortcode-guide:
controller: 'Webfactory\ShortcodeBundle\Tests\Fixtures\Controller\ShortcodeTestController::test'
description: "Description for the 'test-shortcode-guide' shortcode"
example: "test-shortcode-guide test=true"
test-config-invalid-controller: 'Foo\Bar::baz'

services:
Webfactory\ShortcodeBundle\Tests\Fixtures\Controller\:
Expand Down
32 changes: 32 additions & 0 deletions tests/Functional/EmbeddedShortcodeHandlerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,14 @@
namespace Webfactory\ShortcodeBundle\Tests\Functional;

use Generator;
use InvalidArgumentException;
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpKernel\Fragment\FragmentHandler;
use Webfactory\ShortcodeBundle\Handler\EmbeddedShortcodeHandler;
use Webfactory\ShortcodeBundle\Test\EndToEndTestHelper;
use Webfactory\ShortcodeBundle\Tests\Fixtures\Controller\ShortcodeTestController;

/**
* Test shortcode processing using EmbeddedShortcodeHandler and a fixture ShortodeTestController,
Expand Down Expand Up @@ -64,6 +68,34 @@ public static function provideEsiShortcodes(): Generator
yield 'ESI-based shortcode defined in service configuration' => ['test-service-esi'];
}

/** @test */
public function invokable_controller_can_be_used(): void
{
self::assertSame('invokable-controller-response', $this->processShortcodes('<p>[test-config-invokable]</p>'));
}

/**
* @test
*
* @dataProvider provideControllerNames
*/
public function throws_exception_on_invalid_controller_names(string $controllerName): void
{
$this->expectException(InvalidArgumentException::class);

new EmbeddedShortcodeHandler($this->createMock(FragmentHandler::class), $controllerName, 'inline', $this->createMock(RequestStack::class));
}

public static function provideControllerNames(): Generator
{
yield 'Empty string' => [''];
yield 'Not existing controller' => ['Foo\Bar::baz'];
yield 'Missing method name' => [ShortcodeTestController::class];
yield 'Not existing method' => [ShortcodeTestController::class.'_notExistingMethod'];
yield 'Missing class' => ['ThisClassDoesNotExist'];
yield 'Valid reference followed by a second scope resolution operator' => [ShortcodeTestController::class.'::test::'];
}

private function processShortcodes(string $content, ?Request $request = null): string
{
self::bootKernel();
Expand Down
10 changes: 0 additions & 10 deletions tests/Functional/ShortcodeDefinitionTestHelperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

namespace Webfactory\ShortcodeBundle\Tests\Functional;

use InvalidArgumentException;
use RuntimeException;
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
use Thunder\Shortcode\Handler\PlaceholderHandler;
Expand Down Expand Up @@ -31,15 +30,6 @@ public function throws_exception_for_handlers_that_do_not_use_controllers(): voi
$this->helper->resolveShortcodeController('placeholder'); // uses the \Thunder\Shortcode\Handler\PlaceholderHandler handler class directly
}

/**
* @test
*/
public function throws_exception_for_shortcode_with_unresolvable_controller(): void
{
self::expectException(InvalidArgumentException::class);
$this->helper->resolveShortcodeController('test-config-invalid-controller');
}
Comment on lines -34 to -41
Copy link
Member Author

@FabianSchmick FabianSchmick Oct 15, 2024

Choose a reason for hiding this comment

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

With this Test, the execution in an PHP8.1 and composer update --prefer-lowest environment failed, because the config for the shortcode named "test-config-invalid-controller" was loaded every time. Like in this run

In this PR this should also be captured by the new test EmbeddedShortcodeHandlerTest::throws_exception_on_invalid_controller_names()


/**
* @test
*/
Expand Down