search

esotalk / esoTalk

Fat-free forum software.
GNU General Public License v2.0
1.47k stars 239 forks source link

make sure overriden definitions stay overriden #413

Closed mroi closed 9 years ago

mroi commented 9 years ago

When a plugin, or – in my case – custom.php changes a definition by calling ET::define("foo", "bar", true), this new definition is placed in $runtimeDefinitions and then becomes active, because $override is true.

However, when the language changes by way of loadLanguage() the definitions are replayed from $runtimeDefinitions without override and thus do not necessarily become effective. This patch rearranges the logic such that $runtimeDefinitions only captures those definitions that actually become effective when being set for the first time. Then, when a new language is loaded, those definitions are replayed with override set to true.

I am not quite sure about this patch and whether it will break anything else. I am using it for a while in a lightly used esoTalk installation and it fixes a problem for me: I customize some language definitions in custom.php, but those become ineffective when a notification mail is being sent, because there, a loadLanguage() occurs.

jgknight commented 9 years ago

Can you explain the use case of this more? Perhaps I'm not that familiar with how this portion of esoTalk works. But if some text needs to be translated it should be within "$languagePath/definitions.".sanitizeFileName($plugin).".php" is this fix so that a plugin can define something via ET::define() that's not explicitly in the language file?

I guess I'm confused because you could also create a custom language file and then just override the specific definition and let it fall back to the default for everything else.

tobyzerner commented 9 years ago

I think @mroi has explained the problem quite well. Putting definitions in custom.php is a valid use-case, if for example you wanted to customise a definition quickly just for your forum without overwriting core language files (addons/languages/English is overwritten with core updates).

The diff seems like it'll be OK. I'm going to merge it and if any problems crop up we'll deal with them later.

mroi commented 9 years ago

Thanks for merging. I will watch out whether this breaks anything here and will send patches if it does.