Pkp-lib: Upgrade Smarty to v3

[asmecher]

[asmecher]

@NateWr, one of the changes required in preparation for the upgrade to Smarty3 is to stop using our own custom |assign modifier, e.g.:

{translate|assign:"myVariableName key="some.locale.key.here"}

Instead, this needs to be wrapped in the standard {capture} function, e.g.:

{capture assign="myVariableName"}{translate key="some.locale.key.here"}{/capture}

This syntax is a little clunkier, but it's standard, and because of some nasty Smarty internals, it's a necessary change IMO.

This will definitely affect some of your theme templates -- I've already prepared a PR for the Bootstrap theme.

I haven't decided yet when to merge the Smarty3 upgrade, but converting |assign calls to {capture} works with Smarty2 and would probably be a good thing to do ASAP.

[asmecher]

Pulls:

• https://github.com/pkp/pkp-lib/pull/3243
• https://github.com/pkp/ojs/pull/1782
• https://github.com/pkp/omp/pull/489

[asmecher]

@NateWr, would you be able to look at the pulls in the above comment and the additional pull and comment (and additional necessary work in other themes/copies of templates)?

I'm leaning towards merging this ASAP, if you think that's wise. So far it's run quite stably in my testing/fixing, with the exception of the convention changes that were needed per the PRs.

[NateWr]

Looks good. Did you come up with a regex for those capture changes? The bootstrap theme changes look good and I can prepare something similar for the new theme.

Looks like there's a few test issues to work out but otherwise looks good. I clicked around a bit and didn't notice any issues.

[asmecher]

Thanks, @NateWr, I've corrected the issues you identified, re-based, and pushed everything up. Hopefully that'll clear up the tests too.

I don't have a regexp -- I recorded a vim macro to help, and if you like, can use it to clean up any batch of templates you'd like to point me at. (I can open a PR as I did with the manuscript theme.)

Will you be ready for me to merge this a little later this week? (So as to avoid throwing you back in rebase hell.)

[NateWr]

I've only got one left (https://github.com/pkp/pkp-lib/issues/2872) but there could be some conflicts there. The OJS tests are passing so I'm just preparing OMP now. If I don't have it merged by end of day then just go ahead. I've only got the one big one left.

[asmecher]

@NateWr, I finally got this fixed to work with your template precedence changes (don't ask). This involved cleaning up template resources in a way that breaks backwards compatibility.

In OJS 2.x, we had app, core, and file (stock Smarty) template resources. It wasn't clear how these interacted -- the default was file, which was configured to have templates/ and lib/pkp/templates directories; then app and core sometimes covered those same directories a different way.

What I've gone with is a less inconsistent approach:

• core: Always the lib/pkp form of a template (as used e.g. here as a kind of "parent class")
• app: This is the new default resource. It looks first in the app templates directory, then in lib/pkp/templates. This allows the app to selectively override shared templates (as per the above example).
• Plugin template resources, one per plugin that uses templates. Plugins need to refer to these using getTemplateResource. (This required change is described below e.g. for NativeImportExportPlugin.)

This means some reworking in plugin code:

• The getTemplatePath() function should always return a path, or null if the plugin doesn't use templates. This determines whether or not the template resource gets registered at all.
• Template should always be located in the plugin's template path, which is templates unless overridden. This will take some rearrangement of existing plugins but it's good practice.
• Changes will be required where plugins reference templates; see e.g. NativeImportExportPlugin.

With this, all template loads go through PKPTemplateResource, which calls the TemplateResource::getFilename hook permitting overrides.

Questions:

• Do the coding changes justify breaking backward compatibility?
• Does this still meet your needs for overriding templates e.g. with themes? I think this should still work (the hook conventions haven't changed) but I haven't tested it.
• What are the implications for plugins that worked with resource names per your latest changes? I haven't really pulled on this thead yet, but I suspect those changes might not be needed any more.

[asmecher]

The most recent PRs are still:

• https://github.com/pkp/pkp-lib/pull/3243
• https://github.com/pkp/ojs/pull/1782
• https://github.com/pkp/omp/pull/489

...and never mind that the tests fail -- our checkout scripts currently won't handle plugin checkouts from dev forks. They work locally.

[NateWr]

Do the coding changes justify breaking backward compatibility?

I think it's ok. If you're nervous, we could delay until 3.2 so it's in a more major update.

Does this still meet your needs for overriding templates e.g. with themes? I think this should still work (the hook conventions haven't changed) but I haven't tested it.

I'll need to test it, but it looks like the PLUGIN_TEMPLATE_RESOURCE_PREFIX is still in place, so the hook should still work. One thing to check, which I forgot in the original PR, is to make sure that a child theme overriding a plugin will pre-empt a parent theme overriding a plugin.

What are the implications for plugins that worked with resource names per your latest changes? I haven't really pulled on this thead yet, but I suspect those changes might not be needed any more.

There were three parts that needed changing for each plugin (example):

1. The resource was registered.
2. getTemplatePath was registered to return a resource name.
3. Calls to the plugin templates were updated with getTemplatePath or getTemplateResource (there is some inconsistency here.

The goal of step 2 was to increase backwards compatibility. As you can see in the example commit, by changing getTemplatePath to return a resource name prefix, I was able to preserve some backwards compatibility. A call to $this->getTemplatePath . 'mytemplate.tpl would still work.

My sense so far is that this trick could be used to preserve backwards compatibility using your system as well. But I'm not sure about that. And you may decide it's best to enforce a single pattern.

If it is possible to override getTemplatePath to preserve backwards compatibilty, we could do that for 3.1.1, but still refactor all of our plugins to use your new getTemplateResource approach (which I favor). And then when we release 3.1.1 we can indicate this is a temporary measure, and in 3.2 we could pull that out so that getTemplatePath no longer works the old way.

A couple more questions:

1. Is $thing = doing anything here?
2. You said "all template loads go through PKPTemplateResource". Is that all plugin templates or all core and app ones as well? I think it's core and app as well, just checking.

[asmecher]

@NateWr, I've refreshed this post-3.1.1 for merging into the master branch. The pulls are still as above, though updated:

• https://github.com/pkp/pkp-lib/pull/3243
• https://github.com/pkp/ojs/pull/1782
• https://github.com/pkp/omp/pull/489

Ramifications for plugins:

• Templates should be kept consistently in a templates/ subdirectory. No need to override getTemplatePath or similar; those overrides have been removed and templates have been moved where necessary.
• No need to call _registerTemplateResource in plugins. This is done where needed by the Plugin parent class automatically.

The biggest change required is that plugins use $plugin->getTemplateResource('templateName.tpl') rather than $plugin->getTemplatePath() . 'templateName.tpl'). I haven't committed these changes because they're all in plugin code and I can neither test them with Travis (they work locally) nor imagine creating that many branches/PRs :grimacing:

Mind taking another look? The details are essentially the same. I've taken the liberty of merging the necessary plugin versions into their respective master branches, creating ojs-stable-3_1_1 and omp-stable-3_1_1 branches for compatibility. (The Travis scripts aren't currently capable of checking out plugin code from developer forks.)

Perks:

• The old Smarty was starting to collect warnings; the new one does away with many of those.
• Plugins get more consistent and have a little less scaffolding to bother with.
• The Composer dependency is current and the Smarty version is specified generally so it'll keep getting compatible updates.

[NateWr]

This sounds awesome! I'll get started today or tomorrow.

[NateWr]

I've completed the code review. A lot of tedious work done, nice! I just ran into the one major issue with theme templates.

I just ran into a couple more things that needed to be updated:

• The announcement feed block needed to be moved. PR to your branch: https://github.com/asmecher/ojs/pull/5
• The browseBySection plugin will need to be updated. PR here for when you merge this: https://github.com/pkp/browseBySection/pull/8

[asmecher]

@NateWr, https://github.com/asmecher/pkp-lib/commit/6f4efcbf436da1a6f5da572dbdb93ef82b439d05 should fix template overrides -- mind giving it a try? Which plugin are you working with for testing?

[NateWr]

Hmm, maybe I'm missing a step to get this working for me? I have:

• Pulled the PRs for OJS and pkp-lib. They appear to have all commits.
• Run composer update in lib/pkp.
• Checked out the i3242_smartyv3 branch of the browseBySection plugin.
• Activated bootstrap3 theme.

When I load the frontend, the site loads the default templates and none of the ones from the theme. Any theme that doesn't use the default templates seems to have this problem.

[asmecher]

@NateWr, I finally had a chance to look into this further. https://github.com/asmecher/pkp-lib/commit/3494c197e9840503869965a1a2f86f59d0a00c87 should get theme template overrides working again -- please have a look when you can!

[NateWr]

It's still not working for me. You can use the Critical Times theme to test out the plugin template overrides:

1. Check out the ojs3.2 branch of the Critical Times theme: https://github.com/NateWr/criticalTimes/tree/ojs3.2
2. Check out the i3242_smartyv3 branch of the Browse By Section plugin: https://github.com/pkp/browseBySection/tree/i3242_smartyv3
3. Go to Settings > Journal > Sections and make one section browseable.
4. You can then view it at <baseUrl>/section/view/<section-path>.

It should look like this:

Not like this:

I also encountered a fatal error with the usageStats plugin, which looks like it's trying to load the wrong template file path:

[Mon Jun 04 11:01:03.781052 2018] [:error] [pid 25332] [client 127.0.0.1:41188] PHP Fatal error:  Uncaught  --> Smarty: Unable to load template 'app:lib/pkp/plugins/generic/usageStats/templatestemplates/outputFrontend.tpl' in 'app:frontend/objects/article_details.tpl' <-- 

[asmecher]

Thanks, @NateWr, I hadn't checked that case. This should fix it:
https://github.com/asmecher/pkp-lib/commit/84b58e93745b85b07ded13bc72891ee9da9ea0c6

Essentially the hook callback needs to handle tweaks for lib/pkp/ and plugins/ bases. I tried to clean this up by going with the template's name according to the app, rather than the name it gets resolved to on the filesystem, but I think that approach will end up with nasty collisions among frequently-used template names (e.g. settings.tpl for plugins).

[asmecher]

Merged & closed!

[asmecher]

(Thanks for a very helpful couple of review rounds, @NateWr.)