Skip to content

Improve handling of missing language strings#1696

Merged
dregad merged 4 commits intomantisbt:masterfrom
dregad:i27241-missing-lang-string
Sep 25, 2020
Merged

Improve handling of missing language strings#1696
dregad merged 4 commits intomantisbt:masterfrom
dregad:i27241-missing-lang-string

Conversation

@dregad
Copy link
Copy Markdown
Member

@dregad dregad commented Sep 5, 2020

Fixes #27241

atrol
atrol reviewed Sep 5, 2020
View reviewed changes
Comment thread core/error_api.php
@atrol
Copy link
Copy Markdown
Member

atrol commented Sep 5, 2020

I am fine with Print lang string if English localization missing

Special handling for missing lang string errors might not work (see my inline comment) and I am not sure if it's a good idea in general to have a special handling at this place.
It shouldn't be too hard for a developer to find the source code where lang_get is called, or to set $g_show_detailed_errors = ON; to see the call in stack trace.

@dregad
Copy link
Copy Markdown
Member Author

dregad commented Sep 5, 2020 •
edited
Loading

It shouldn't be too hard for a developer to find the source code where lang_get is called

True it's not very hard, but still extra effort to grep for the missing string, when we actually have this information and can easily provide it to them for immediate action.

set $g_show_detailed_errors = ON; to see the call in stack trace.

That does not work with our recommended dev config for $g_display_errors, as there is no stack trace displayed when errors are shown inline. The developer would need to set $g_display_errors[E_USER_WARNING] = DISPLAY_ERROR_HALT;

@atrol
Copy link
Copy Markdown
Member

atrol commented Sep 5, 2020

I thought I am using the recommended dev config, as I tried on localhost.

The developer would need to set $g_display_errors[E_USER_WARNING] = DISPLAY_ERROR_HALT;

When using localhost, we set $g_display_errors[E_USER_WARNING] = DISPLAY_ERROR_HALT;, so I got the trace.

Should we change what is documented as recommended dev config, or should we change the settings for localhost?

@dregad
Copy link
Copy Markdown
Member Author

dregad commented Sep 6, 2020

Should we change what is documented as recommended dev config, or should we change the settings for localhost

I never realized that we had a discrepancy between documentation and the actual defaulted localhost config. They should definitely be aligned, and they were when I originally introduced this feature (see #215).

It appears that when @vboctor enforced halting of scripts on errors (as part of #1240, commit c816588), he changed the defaulted localhost behavior by adding $g_display_errors[E_USER_WARNING] = DISPLAY_ERROR_HALT;, not sure where that came from.

IMO, DISPLAY_ERROR_INLINE is better for our internal warnings in development mode, as it is less intrusive; I would suggest to align the localhost settings to the documented values.

@dregad dregad force-pushed the i27241-missing-lang-string branch 2 times, most recently from 527366c to a212dc0 Compare September 20, 2020 11:15
@dregad
Copy link
Copy Markdown
Member Author

dregad commented Sep 20, 2020

I updated Admin Guide, PHPDoc for $g_display_errors and the default developers settings.

This should now be ready to merge.

atrol
atrol reviewed Sep 20, 2020
View reviewed changes
Comment thread config_defaults_inc.php
@dregad dregad force-pushed the i27241-missing-lang-string branch from a212dc0 to 55be3e0 Compare September 25, 2020 18:28
@dregad
Print lang string if English localization missing
6ef1734 
When an English language string is missing, MantisBT currently displays
an empty string; this is not very friendly to the user, who may be
missing important information (e.g. a button label).

If English string is missing, display the language string itself
(i.e. the $p_string parameter to lang_get()).

Fixes #27241
dregad added a commit to dregad/mantisbt that referenced this pull request Sep 25, 2020
@dregad
PR review fix
2838272 
mantisbt#1696 (comment)
@dregad dregad force-pushed the i27241-missing-lang-string branch from 55be3e0 to 2838272 Compare September 25, 2020 23:03
@dregad
Special handling for missing lang string errors
6000434 
If the error type is 300 (ERROR_LANG_STRING_NOT_FOUND), we retrieve the
details (file name and line number) of the parent error. Properly
handles the case when user's language is not English, by going through
the error stack trace until the first lang_get() call is found.

This allows printing information about the actual location of the
missing string, instead of a not-so-useful reference to lang_get().

Fixes #27241
@dregad
Adjust PHPDoc for error_handler()
e3e82b1 
Better document the fact that $p_error can be either int or string,
depending on whether a MantisBT internal or a PHP system error has been
thrown.

Follows discussion in https://github.com/mantisbt/mantisbt/pull/1696/files#r483995299
@dregad
Align display_errors docs and default dev settings
53fba47 
There were discrepancies between the PHPDoc block for $g_display_errors
in config_defaults_inc.php, the defaulted developer-specific settings
when the server is localhost, and the indications in the Admin Guide.

Fixes #27300
@dregad dregad force-pushed the i27241-missing-lang-string branch from 2838272 to 53fba47 Compare September 25, 2020 23:09
@dregad dregad mentioned this pull request Sep 25, 2020
Merged
@dregad dregad merged commit 53fba47 into mantisbt:master Sep 25, 2020
@dregad dregad deleted the i27241-missing-lang-string branch September 25, 2020 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atrol atrol atrol left review comments

Assignees

@dregad dregad

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dregad @atrol