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

Use config API to access allow_browser_cache #1956

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

dregad
Copy link
Member

@dregad dregad commented Jan 7, 2024

Until now $g_allow_browser_cache config was handled in a non-standard way in in http_caching_headers(), i.e. by referencing it as a global variable instead of retrieving its value with config_get_global().

Moreover, the setting was commented out in config_default_inc.php (i.e. not defined) instead of being set to OFF.

This follows up on discussion in PR #1925.

Fixes #33482

Until now $g_allow_browser_cache config was handled in a non-standard
way in in http_caching_headers(), i.e. by referencing it as a global
variable instead of retrieving its value with config_get_global().

Moreover, the setting was commented out in config_default_inc.php
(i.e. not defined) instead of being set to OFF.

This follows up on discussion in PR
mantisbt#1925

Fixes #33482
@dregad dregad self-assigned this Jan 7, 2024
@dregad
Copy link
Member Author

dregad commented Jan 7, 2024

Please read https://mantisbt.org/bugs/view.php?id=33482#c68427 as part of review

Copy link
Member

@atrol atrol left a comment

Choose a reason for hiding this comment

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

I am fine with the changes.

As a side note: Using config_get_global is about 7 times (xdebug.mode = debug) / 11 times (without xdebug) slower than direct access of the variable.

@dregad
Copy link
Member Author

dregad commented Jan 9, 2024

Thanks for the feedback @atrol

Using config_get_global is about 7 times (xdebug.mode = debug) / 11 times (without xdebug) slower than direct access of the variable.

Which is to be expected of course, although I'm surprised it's actually faster with Xdebug than without...

I just followed @vboctor's note in #1925 (comment)

Update the code fetching them to use the standard config_get_global() method.

@atrol
Copy link
Member

atrol commented Jan 9, 2024

I'm surprised it's actually faster with Xdebug than without...

@dregad It's not faster with Xdebug.
Seems my formulation was not clear.

As a side note: Using config_get_global is about 7 times (xdebug.mode = debug) / 11 times (without xdebug) slower than direct access of the variable.

I compared two times config_get_global calls with direct access of the variable.
With Xdebug enabled, config_get_global is about 7 times slower than direct access of the variable, (e.g. 7 seconds vs. 1 second)
With Xdebug disabled, config_get_global is about 11 times slower than direct access of the variable. (e.g. 5.5 seconds vs. 0.5 seconds)

@dregad
Copy link
Member Author

dregad commented Jan 10, 2024

Thanks for the clarification.

@dregad dregad merged commit 2c09d01 into mantisbt:master Jan 15, 2024
1 check passed
@dregad dregad deleted the i33482-allow_browser_cache branch January 15, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants