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

Upgrade Smarty to 2.6.31 to solve issues on PHP7.2 #208

Merged
merged 1 commit into from
May 15, 2018

Conversation

seamuslee001
Copy link
Contributor

No description provided.

@@ -556,7 +551,7 @@ function _compile_tag($template_tag)

case 'php':
/* handle folded tags replaced by {php} */
list(, $block) = each($this->_folded_blocks);
$block = array_shift($this->_folded_blocks);
Copy link
Member

Choose a reason for hiding this comment

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

Most of the changes look like clear equivalents. The loop through $this->_folded_blocks is the only one which feels like it could potentially have side-effects (depending on how $this->_folded_blocks is used elsewhere), but I'd defer to upstream if they like it.

@totten
Copy link
Member

totten commented May 15, 2018

(CiviCRM Review Template WORD-1.1)

  • (r-explain) Pass: The purpose and substance of the upgrade seem narrow, so I don't think it needs much more detailed explanation.
  • (r-test) Pass
  • (r-code) Pass
  • (r-doc) Pass: Not needed
  • (r-maint) Pass: Same as before
  • (r-run) Pass: Spot-checked some pages which use Smarty in different ways (e.g. plain pages and AJAX snippets).
  • (r-user) Pass: No UX implication
  • (r-tech) Pass: No change in contract. Civi does have a couple customizations in Smarty that can rely on upstream semantics behaving just-so, but the ones I can recall all have unit-tests.

Thanks, @seamuslee001 !

@totten totten merged commit 93f590c into civicrm:master May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants