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

Error handler should only allow notices etc in smarty templates, not beyond #840

Open
wants to merge 3 commits into
base: support/4.3
Choose a base branch
from

Conversation

rudiedirkx
Copy link

Only handle errors in the smarty compiled dir. Not errors that happen during smarty but somewhere else.

@rudiedirkx
Copy link
Author

See #839

@lemon-juice
Copy link

This has always been an important missing feature in Smarty error handler. rudiedirkx's solution looks good, I hope it gets merged into Smarty!

@rudiedirkx
Copy link
Author

@wisskid Did you catch this somewhere else by now? I haven't been paying attention to Smarty merges. My method might not be acceptable, but Smarty has to check against $smarty->getCompileDir() probably.

@wisskid
Copy link
Contributor

wisskid commented Oct 19, 2023

@rudiedirkx I hadn't looked into this yet. Seems like a great improvement. At some point, I would like to get rid of the error handler altogether, but this seems like a great first step. I'll have to adapt it to the new v5 release, but this one will do just fine for v4. I'll merge it asap.

@wisskid wisskid changed the base branch from master to support/4.3 October 19, 2023 20:43
@rudiedirkx
Copy link
Author

The base branch change did something weird to the diff. I resolved a merge conflict, but it's still weird. The error handler has changed in the meantime probably. All I did was add

if (strpos($errfile, $this->smarty->getCompileDir()) === 0) {

around all of the return conditions in handleError().


Smarty still needs its own error handler, doesn't it? You don't want an uncaught error to trace to a compiled Smarty template. Or maybe you do... I don't know. Twig catches all errors during a template and rethrows it with original template info. That's good for debugging.

@rudiedirkx
Copy link
Author

I can't change the base branch of my fork, so I can't fix this.

@wisskid
Copy link
Contributor

wisskid commented Nov 6, 2023

@rudiedirkx how about uncompiled templates such as 'eval:' templates or stream plugins?

@rudiedirkx
Copy link
Author

No idea. I've never used those. What are $errfile and $this->smarty->getCompileDir() then?

@wisskid
Copy link
Contributor

wisskid commented Nov 6, 2023

The compile dir is probably still the (default) compile dir, but $errfile will be the phone script calling the template. Which will not be in the compile dir.

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.

None yet

3 participants