smarty-php
Bug fix for underscore in template name
This addresses bug #578 and #490, by correcting the {$smarty.template} variable when using template names containing the "_" character along with a custom resource.
This also extends the maximum template name length from 25 to 255 when using a custom resource.
Hi @EDCScott . I've reviewed your changes and I see no problem in allowing for an underscore the regex. The changing of the max length might be a problem, though. The template name is combined with prefixes and postfixes into a cache file name, and the total length of the cache file name probably should exceed 255 chars. So allowing 255 chars for the template name alone might cause problems there.
@wisskid I agree. The 255 length was chosen as it's the maximum filename length on many platforms (https://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits), including NTFS and various versions of ext. If we're going to be adding something to that file name, then we should abbreviate it.
Is there a maximum length on the prefix or postfix, or are they user-definable? If they have a limit, we should probably make the maximum length here be (255 - (max prefix) - (max postfix)). If there is no maximum length, then that's probably another bug in it's own right. Perhaps we limit prefix and postfix to 64 characters, and then limit the template name itself to 127 characters?
Or if you want to make things really robust (and complicated), the limits are technically 255 BYTES. I'm not sure if Smarty has made an effort to account for multi-byte filenames.
The lower effort changes would probably be just to adjust the 255 to 127 and call it a day. If smarty isn't providing proper error handlers for overlength files, then it's not really a different developer experience if you overflow the maximum filename length, than if it's unsuspectedly trimming your filename length internally.
Your thoughts?
Since
- cache ID is user-defined (read: arbitrary length),
- cache location is user-defined (read: can be anything, not just filesystem),
the entire discussion about lengths of the names is questionable.
But I see a different problem with this PR: it lacking tests.
looks fine, maybe repo maintainers can just edit the character limit to their liking and merge?
