Problem/Motivation

When twig.config:auto_reload is true, the component's templates are not refreshed, they only refreshed after the change to the YML file.
This is also weird because the parsed YML file is cached separately anyway.

This issue lies in \Drupal\Core\Template\Loader\ComponentLoader::isFresh. It is checking the YML file, and if YML is not updated, it will never check the template itself.

Steps to reproduce

Set twig.config:auto_reload to true
Create component
Render component
Change the component's template
The component still renders the old contents while the usual templates are refreshed.

Proposed resolution

Always check if the template file is fresh. Not sure the YML file needs to be checked at all since its contents will be cached anyway.

Remaining tasks

-

User interface changes

-

API changes

-

Data model changes

-

Release notes snippet

-

Issue fork drupal-3456676

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

ReINFaTe created an issue. See original summary.

reinfate’s picture

Status: Active » Needs review
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Nice catch!

Feel this is something we should probably have test coverage for.

reinfate’s picture

Status: Needs work » Needs review

Added the test coverage for the isFresh function in ComponentLoader

I've removed the yml file change check, it is not needed, since the component schema will be refreshed only after the discovery cache clear anyway.

The test failures seem to be not related to this issue. I've rerun them multiple times on 11.x and 11.1.x branches, and each time different test was failed.

grimreaper’s picture

Issue tags: -Needs tests

Hi,

Thanks for the bug report and the fix, I also witnessed this behavior to be in Twig debug mode and not have component refreshed after change in Twig.

Tested ok.

I just put one question on MR, so not RTBC for that.

grimreaper’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think if we're enabling twig.config:auto_reload i would expect changes to the yml or the template to result in changes automatically. Made a suggestion to make it work this way.

reinfate’s picture

Status: Needs work » Needs review

I don't see why we need to check the YML file.
The class is responsible specifically for the component's twig file.
Even if we assume that the YML discovery cache is disabled, the YML file itself doesn't affect the twig compilation in any way.
Unless the changes are in the twig file itself it shouldn't be recompiled therefore it is fresh.

pdureau’s picture

Assigned: Unassigned » pdureau
grimreaper’s picture

Assigned: pdureau » grimreaper

Hi,

Even if we could disable YML discovery cache with $settings['cache']['bins']['discovery'] = 'cache.backend.null';, it will disable all plugin discovery cache which could slow down development environment.

So even if I would say we should not check for YML change, I agree that it could be handy to have that check.

I don't see why we need to check the YML file.

I am not sure but with UI Patterns 2 features not yet in Core like with data normalization, you may change some prop types and it can impact the template results?

I will update the MR regarding @alexpott proposed changes, so maintainers can easily decide if they want it or not.

grimreaper’s picture

Assigned: grimreaper » Unassigned

MR updated.

grimreaper’s picture

Unrelated fails in the pipeline. I rebased and repushed to see if that's better.

pdureau’s picture

Assigned: Unassigned » pdureau
alexpott’s picture

Status: Needs review » Reviewed & tested by the community

The updates made by @grimreaper look great.

pdureau’s picture

Great, I will commit.

  • pdureau committed de5530c6 on 11.x
    Issue #3456676 by reinfate, grimreaper, pdureau, alexpott: Single...

  • pdureau committed 4c016c7d on 11.2.x
    Issue #3456676 by reinfate, grimreaper, pdureau, alexpott: Single...
pdureau’s picture

Assigned: pdureau » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed de5530c6 and pushed to 11.x. Thanks!

Committed 4c016c7 and pushed to 11.2.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.