Skip to content

Conversation

@daniser
Copy link

@daniser daniser commented Jun 11, 2025

This PR renames Twig runtime extension from MenuRuntimeExtension.php to MenuRuntime.php to follow naming convention.
This allows Twig to know when to recompile template(s) if extension code has changed.
This should not break BC because it is internal class not used anywhere else directly (correct me if I'm wrong).

This PR provides custom MenuExtension::getLastModified method implementation. This allows Twig to know when to recompile template(s) if extension code has changed.

Copy link
Collaborator

@stof stof left a comment

Choose a reason for hiding this comment

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

MenuRuntimeExtension was not marked as @internal and projects using the library have to handle instantiating it, so this PR is a BC break.

Please provide a custom implementation of getLastModified instead to achieve it without BC break.

{
return max(
(int) filemtime(__FILE__),
(int) filemtime((string) (new \ReflectionClass(MenuRuntimeExtension::class))->getFileName()),
Copy link
Author

Choose a reason for hiding this comment

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

Casts added to keep Phpstan happy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could simplify this by not using reflection but using __DIR__ . '/MenuRuntimeExtension.php') instead.

Copy link
Collaborator

@garak garak left a comment

Choose a reason for hiding this comment

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

Can we add a test?

@daniser daniser requested review from garak and stof June 11, 2025 15:15
@stof stof merged commit b528141 into KnpLabs:master Jun 13, 2025
11 checks passed
@daniser daniser deleted the fix/extension-last-modified branch June 13, 2025 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants