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

Update smarty to version 3.1.27 #12807

Merged
merged 2 commits into from Dec 7, 2015

Conversation

Projects
None yet
4 participants
@exside
Contributor

exside commented Dec 6, 2015

What does it do ?

Remove all files in core/model/smarty/ and add all files of the current smarty release 3.1.27.

Why is it needed ?

a) To keep up to date with major improvements in the library (speed etc.) and b) to avoid possible issues with no available PHP 7.

Related issue(s)/PR(s)

Just marginally #12797 as the PHP 7 Compatibility checker also lists one smarty class which could be an issue in PHP7 (unconfirmed)

Stuff to be aware of

On my git installation this switch to the current version works without any hickups, the problem for me is that I'm not very familiar with smarty and thus may not be able to test this properly...in my case the manager works exactly like before, cleared the cache folder manually and extensively browsed the manager, but yeah, that's kinda "hobby" testing so if anybody has some concerns about this update, please let me know how I can test this PR better!

exside added some commits Dec 6, 2015

@Mark-H Mark-H self-assigned this Dec 7, 2015

@Mark-H Mark-H added this to the v2.5.0-pl milestone Dec 7, 2015

@Mark-H Mark-H merged commit a607a45 into modxcms:2.x Dec 7, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Mark-H

This comment has been minimized.

Collaborator

Mark-H commented Dec 7, 2015

Thanks @exside - merged into 2.5.

@rtripault

This comment has been minimized.

Collaborator

rtripault commented Dec 8, 2015

Some Smarty method signature changed, making modSmarty override causing the following error : (ERROR @ /app/vendor/modx/revolution/core/model/modx/smarty/modsmarty.class.php : 37) PHP warning: Declaration of modSmarty::display($template, $cache_id = NULL, $compile_id = NULL, $parent = NULL) should be compatible with Smarty::display($template = NULL, $cache_id = NULL, $compile_id = NULL, $parent = NULL).

I believe adding a default null value to $template would do the trick while still being Backward Compatible (i might be wrong, but not seeing anyone extending modSmarty).

Also, it appears Smarty became more "strict" and now rises warnings when trying to access undefined variables/indexes : (WARN @ /app/vendor/modx/revolution/core/cache/mgr/smarty/app/65fb5a01a40da9eec417ba32a0f289537ef2107d_0.file.header.tpl.php : 81) PHP notice: Undefined index: a. To fix that, i guess using the |default modifier (http://www.smarty.net/docs/en/language.modifier.default.tpl) could to the trick (i remind doing that in PR #12063)

@exside

This comment has been minimized.

Contributor

exside commented Dec 8, 2015

@rtripault how do you get these and where (when accessing which pages in the manager)? should I set the default MODX log level to debug or warn to get them?

@rtripault

This comment has been minimized.

Collaborator

rtripault commented Dec 9, 2015

Right, my log level is at warn (3).
Those errors appears on every manager page (since some/most "issues" are inside the header.tpl), but i believe each Smarty template should be checked (well, especially those having variables in)

@exside

This comment has been minimized.

Contributor

exside commented Dec 9, 2015

it's strange, I set the log_level in the manager to 4 (e.g. show all / DEBUG) but I don't get anything =(, did you set the log level somewhere else?

@opengeek

This comment has been minimized.

Member

opengeek commented Jan 11, 2016

FWIW @exside this depends on your error_reporting level in PHP as well. If PHP is not configured to show E_NOTICE level errors (typical production PHP configuration), the log_level will make no difference.

@exside

This comment has been minimized.

Contributor

exside commented Mar 7, 2016

@opengeek: I put

error_reporting = E_ALL & ~E_NOTICE | E_STRICT

in my php ini, but didn't get the notices...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment