Sitelet https://github.com/KnpLabs/KnpMenu/pull/405
Skip to content

Fix twig runtime extension last modification time detection - #405

Merged
stof merged 5 commits into
KnpLabs:masterfrom
daniser:fix/extension-last-modified
Jun 13, 2025
Merged

stof merged 5 commits into
KnpLabs:masterfrom
daniser:fix/extension-last-modified

Conversation

@daniser

@daniser daniser commented Jun 11, 2025 •

Copy link
Copy Markdown

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.

@stof stof left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread src/Knp/Menu/Twig/MenuExtension.php Outdated
{
return max(
(int) filemtime(__FILE__),
(int) filemtime((string) (new \ReflectionClass(MenuRuntimeExtension::class))->getFileName()),

Copy link
Copy Markdown
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
Copy Markdown
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.

@garak garak left a comment

Copy link
Copy Markdown
Collaborator

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
@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