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

Bug fix for underscore in template name #581

Merged

Conversation

Scootin159
Copy link
Contributor

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.

@wisskid wisskid added the waiting Waiting for answer label Apr 13, 2020
@wisskid
Copy link
Contributor

wisskid commented Apr 13, 2020

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.

@Scootin159
Copy link
Contributor Author

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

@AnrDaemon
Copy link
Contributor

Since

  1. cache ID is user-defined (read: arbitrary length),
  2. 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.

@BarbzYHOOL
Copy link

looks fine, maybe repo maintainers can just edit the character limit to their liking and merge?

@wisskid wisskid removed the waiting Waiting for answer label Mar 21, 2021
@wisskid wisskid merged commit 4fc39d5 into smarty-php:master Sep 22, 2022
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.

4 participants