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

BUGFIX: ensure template file exists #9488

Closed
wants to merge 1 commit into from

Conversation

mattclegg
Copy link
Contributor

@mattclegg mattclegg commented Apr 22, 2020

When a .ss file is removed from the theme and the cache is not cleared then I receive the error; filemtime(): stat failed for MyTemplate.ss ref SSViewer.php on line 636.

Does it make sense to check if the file exists before running filemtime to avoid the problem again?

Any feedback welcome.

Copy link
Contributor

@dhensby dhensby left a comment

Choose a reason for hiding this comment

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

Sure, something like this makes sense, but it also seems there needs to be a more thorough handling of missing template files.

Fixing this particular notice would then just mean the template rendered unexpectedly from the cache, right?

This change needs to more thoroughly handle this error case and we probably needs some test coverage as well as agreed handling for this. Should we have a helpful error, for example, like "template not found" or should this kick off a flush of the template cache?

@mattclegg
Copy link
Contributor Author

Yeah, this is just hiding the problem really but personally I'd rather they see stale file cache when a user is accessing the website than they see a PHP Warning.

@dhensby
Copy link
Contributor

dhensby commented Apr 22, 2020

No one will see a php warning because warnings aren't displayed in prod

@mattclegg
Copy link
Contributor Author

That's true. Do you think it would be better to raise "template not found" as a warning when filemtime fails?

@dhensby
Copy link
Contributor

dhensby commented Apr 22, 2020

I think that makes sense - we could report a warning if the template doesn't exist and we can give a more helpful warning too: Could not find template "%s"; you may need to flush the cache

@emteknetnz
Copy link
Member

This pull request hasn't had any activity for a while. Are you going to be doing further work on it, or would you prefer to close it now?

@emteknetnz
Copy link
Member

It seems like there's going to be no further activity on this pull request, so we've closed it for now. Please open a new pull-request if you want to re-approach this work, or for anything else you think could be fixed or improved.

@emteknetnz emteknetnz closed this Sep 21, 2020
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.

None yet

3 participants