Skip to content

Conversation

FabianSchmick
Copy link
Member

Resolves #35

?LoggerInterface $logger = null
LoggerInterface $logger = null
) {
$callableFragments = explode('::', $controllerName);
Copy link
Member Author

@FabianSchmick FabianSchmick Oct 14, 2024

Choose a reason for hiding this comment

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

I am not sure if I should also check for single : if I remember correctly, this was valid some time ago, too?

In the docs https://www.php.net/manual/en/language.oop5.paamayim-nekudotayim.php there is no : alternate

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK not in PHP, you might be thinking about Symfonys homebrew "bundle notation", which got deprecated in 4.1.

More importantly, I think we should allow invokable controllers as well. I'll add some changes shortly.

Comment on lines -34 to -41
/**
* @test
*/
public function throws_exception_for_shortcode_with_unresolvable_controller(): void
{
self::expectException(InvalidArgumentException::class);
$this->helper->resolveShortcodeController('test-config-invalid-controller');
}
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()

Copy link
Member

@MalteWunsch MalteWunsch left a comment

Choose a reason for hiding this comment

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

Thank you! Do you want to have a look at my additions?

From my point of view: good to merge and release!

@FabianSchmick FabianSchmick merged commit b763b8b into master Oct 30, 2024
@FabianSchmick FabianSchmick deleted the 173693_validate_shortcode_config branch October 30, 2024 10:40
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.

Validate shortcode service configurations (Case 173693)
2 participants