Opensearch improvements #840
Conversation
- Regroup XML generation at end of script - Add XML header
Previously, we were using an hardcoded image file that was not matching the new MantisBT icon. This references the standard MantisBT favicon instead, and allows administrators to customize their Browser Search with their own icon. Fixes #11964
Prior to this, the prefix for OpenSearch entries generated by browser_search_plugin.php was hardcoded to 'MantisBT'. This defines a new '$g_search_title' config option (which defaults to the value of $g_window_title) to be used as prefix, allowing - administrators to customize the name of their browser search entries - users to have distinct search engines for multiple MantisBT instances $g_search_title should be shorter than 8 chars to be compliant with the maximum size of 16 chars for the ShortName element as per OpenSearch specification [1] Fixes #11964 [1] http://www.opensearch.org/Specifications/OpenSearch/1.1
The $g_search_title prefix is inserted into the language strings to produce the final text used both to advertise and register the search engine in the user's browser. Issue #11964
| * @see $g_window_title | ||
| * @global string $g_search_title | ||
| */ | ||
| $g_search_title = '%window_title%'; |
There was a problem hiding this comment.
Please mark as a public config.
There was a problem hiding this comment.
A bit off topic.
I don't like to introduce one more of this '%var%' stuff.
I would like to discuss removal of recursion in defining configuration variables.
I don't see that much value in it and would expect a better performance if config_eval would be removed and
config_get_global and config_get would be simplified.
There was a problem hiding this comment.
I don't see that much value in it
It makes configuration easier for admins, by avoiding the need to duplicate declaration of several related options. Consider the case of paths, or $g_cookie_prefix for example. Without the '%%' feature, the admin would have to manually set each of the cookies names in their config_inc.php.
You will probably and rightly argue that we do not need to make the cookies names configurable. Of course by refactoring the code (in the above case, getting rid of these config options), we can implement alternative that works without the recursion.
The questions are, whether this is worth the effort and what kind of performance improvement we can obtain by removing this feature.
expect a better performance if config_eval would be removed and config_get_global and config_get would be simplified.
If this is important to you, I would suggest to open an issue in the tracker, and evaluate how much we'd gain with your proposed approach.
There was a problem hiding this comment.
evaluate how much we'd gain with your proposed approach.
Quick and dirty removal of config_eval
https://github.com/atrol/mantisbt/tree/config_eval_remove
There is a bit less memory usage (removed the caching of the evaled options) and there is about 7% better performance in 2nd benchmark, see results below.
Not outstanding, but not that bad for such a small part of the code.
Do you think it's worth to think more about it (obsolete cookie seetings, ...) and target it for a version > 2.0?
My View 2nd call
Original master-1.3.x
Page execution time: 0.3540 seconds
Memory usage: 10,411 KiB
Removed config_eval
Page execution time: 0.3499 seconds
Memory usage: 10,388 KiB
View Issues 2nd call
Original master-1.3.x
Page execution time: 0.2916 seconds
Memory usage: 10,514 KiB
Removed config_eval
Page execution time: 0.2705 seconds
Memory usage: 10,441 KiB
There was a problem hiding this comment.
about 7% better performance in 2nd benchmark
7% is not bad indeed, I did not expect that much actually. But keep in mind that in absolute terms, it's still less than 0.02 s gain.
I'm still concerned about the loss of "just in time" evaluation of configs however, especially for paths that are quite likely to have been changed by admins - we need to be very careful not to cause regressions. Consider this example:
config_defaults_inc.php
$g_path = (dynamically built from $_SERVER) // e.g. http://internal-hostname.example.com/mantisbt/
$g_icon_path = $g_path . 'images/';
config_inc.php
$g_path = 'http://example.com/mantisbt/'; # required e.g. due to use of reverse proxy
In this case, $g_icon_path == 'http://internal-hostname.example.com/mantisbt/images/, which is invalid in the reverse-proxy scenario; this would force the admin to manually define all variables referencing $g_path (formerly %path% in their config_inc.php (and knowing that they have to do so). Not very user-friendly, and likely to cause support issues when upgrading.
Do you think it's worth to think more about it (obsolete cookie seetings, ...)
Yes definitely. And get others' opinions too.
target it for a version > 2.0?
For sure a 2.x target, if we decide to move forward.
|
👍 once comments are applied. |
1 similar comment
|
👍 once comments are applied. |
As per vboctor's suggestion in PR review comments.
As per vboctor's suggestion in PR review comments.
Done
Fixed (also for favicon_image). |
- Use standard favicon instead of hardcoded PNG for Mantis search engines - Define new config for search engine name prefix (defaults to $g_window_title) - Localize search engine ShortNames and Descriptions Fixes #11964, Pull request #840
This PR follows-up and improves on #73, fixes #11964