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

Removed hardcoded text from Upgrade.php #4470

Merged
merged 18 commits into from Mar 1, 2018

Conversation

Projects
None yet
7 participants
@frandominguez03
Member

frandominguez03 commented Jan 4, 2018

Fixes #4465

Did I forget about anyone?

frandominguez03 added some commits Jan 4, 2018

We like $txt strings!
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>
Now update Upgrade.php
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>
One missing
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>
Remove extra space
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>

@frandominguez03 frandominguez03 changed the title from Removed hardcoded text from Upgrade.txt to Removed hardcoded text from Upgrade.php Jan 4, 2018

frandominguez03 added some commits Jan 4, 2018

Typo
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>
Typo2
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>
@wintstar

This comment has been minimized.

Show comment
Hide comment
A couple more
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>

frandominguez03 added some commits Jan 4, 2018

Duplicated string
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>
Remove this xD
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>
@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Jan 4, 2018

Member

@wintstar Nice catch! Fixed now.

Member

frandominguez03 commented Jan 4, 2018

@wintstar Nice catch! Fixed now.

@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Jan 4, 2018

Member

There are a few $txt global missing too. Gonna fix them asap.

Member

frandominguez03 commented Jan 4, 2018

There are a few $txt global missing too. Gonna fix them asap.

@frandominguez03 frandominguez03 changed the title from Removed hardcoded text from Upgrade.php to [WIP] Removed hardcoded text from Upgrade.php Jan 4, 2018

@wintstar

This comment has been minimized.

Show comment
Hide comment
@wintstar

wintstar Jan 4, 2018

Fatal error: Uncaught Error: Call to undefined function load_lang_file() in C:\xampp\htdocs\smfbeta\upgrade.php:85 Stack trace: #0 {main} thrown in C:\xampp\htdocs\smfbeta\upgrade.php on line 85

function load languge_file is defined in other/install.php

function load_lang_file()

missing in install language file https://github.com/d3vcho/SMF2.1/blob/hardcoded_text/other/upgrade.php#L3656
$txt['upgrade_admin_login'],

wintstar commented Jan 4, 2018

Fatal error: Uncaught Error: Call to undefined function load_lang_file() in C:\xampp\htdocs\smfbeta\upgrade.php:85 Stack trace: #0 {main} thrown in C:\xampp\htdocs\smfbeta\upgrade.php on line 85

function load languge_file is defined in other/install.php

function load_lang_file()

missing in install language file https://github.com/d3vcho/SMF2.1/blob/hardcoded_text/other/upgrade.php#L3656
$txt['upgrade_admin_login'],

frandominguez03 added some commits Jan 4, 2018

More fixes
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>
One more
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>
@wintstar

This comment has been minimized.

Show comment
Hide comment
@wintstar

wintstar Jan 4, 2018

It is not better to the function function load_lang_file() moved from install.php to Load.php. So that this can also be included for the install.php and upgrade.php? There was a bug with the German umlauts yesterday, because the language file was not loaded correctly.

#4457
#4463

wintstar commented Jan 4, 2018

It is not better to the function function load_lang_file() moved from install.php to Load.php. So that this can also be included for the install.php and upgrade.php? There was a bug with the German umlauts yesterday, because the language file was not loaded correctly.

#4457
#4463

Looks like I've missed one more heh
Signed-off-by: Frankie "d3vcho" <d3vcho@mail.com>
@wintstar

This comment has been minimized.

Show comment
Hide comment
@wintstar

wintstar Jan 4, 2018

With your last fix in upgrade.php all language strings not correct loaded.

upgrade-8

However, if I copy the function load_lang_file() and paste it last in upgrade-helper,php all language and set function load_lang_file() in upgrade.php at line 85 the strings will be loaded.

wintstar commented Jan 4, 2018

With your last fix in upgrade.php all language strings not correct loaded.

upgrade-8

However, if I copy the function load_lang_file() and paste it last in upgrade-helper,php all language and set function load_lang_file() in upgrade.php at line 85 the strings will be loaded.

@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Jan 4, 2018

Member

Which means that the following lines aren't working...

// Load the language. if (file_exists($modSettings['theme_dir'] . '/languages/Install.' . $upcontext['language'] . '.php')) require_once($modSettings['theme_dir'] . '/languages/Install.' . $upcontext['language'] . '.php');

Member

frandominguez03 commented Jan 4, 2018

Which means that the following lines aren't working...

// Load the language. if (file_exists($modSettings['theme_dir'] . '/languages/Install.' . $upcontext['language'] . '.php')) require_once($modSettings['theme_dir'] . '/languages/Install.' . $upcontext['language'] . '.php');

@wintstar

This comment has been minimized.

Show comment
Hide comment
@wintstar

wintstar Jan 4, 2018

upgrade-9

Your first version of fix upgrade.php line 83

global $txt;

// Initialize everything and load the language files.
load_lang_file();

work with this
This funtion https://github.com/SimpleMachines/SMF2.1/blob/release-2.1/other/install.php#L260 have i copy to thend of file other/upgrade-helper.php.

and this work not correctly at line 83
// Load the language. if (file_exists($modSettings['theme_dir'] . '/languages/Install.' . $upcontext['language'] . '.php')) require_once($modSettings['theme_dir'] . '/languages/Install.' . $upcontext['language'] . '.php');

wintstar commented Jan 4, 2018

upgrade-9

Your first version of fix upgrade.php line 83

global $txt;

// Initialize everything and load the language files.
load_lang_file();

work with this
This funtion https://github.com/SimpleMachines/SMF2.1/blob/release-2.1/other/install.php#L260 have i copy to thend of file other/upgrade-helper.php.

and this work not correctly at line 83
// Load the language. if (file_exists($modSettings['theme_dir'] . '/languages/Install.' . $upcontext['language'] . '.php')) require_once($modSettings['theme_dir'] . '/languages/Install.' . $upcontext['language'] . '.php');

@wintstar

This comment has been minimized.

Show comment
Hide comment
@wintstar

wintstar Jan 4, 2018

Missing language string_ in other/upgrade.php for
Updating Your SMF Installation!
https://github.com/SimpleMachines/SMF2.1/pull/4470/files#diff-cccb32785017365b9e98cf35be9ed3dfL218

Warning! https://github.com/SimpleMachines/SMF2.1/pull/4470/files#diff-cccb32785017365b9e98cf35be9ed3dfR3752

Database Changes
https://github.com/SimpleMachines/SMF2.1/pull/4470/files#diff-cccb32785017365b9e98cf35be9ed3dfR1286

search in upgrade.php file at "page_title" They are not have language strings.

I mean it´s not optimal the html start tag in language string $txt['upgrade_stats_info'] and the endtag in upgrade.php. Translators might think that a closing tag is missing and add this tag.
https://github.com/SimpleMachines/SMF2.1/pull/4470/files#diff-c1b42c4dcb08415dfddc398e9c1f0557R289

string ist double $txt['upgrade_done2'] , i mean $txt['upgrade_done3'] it should be
https://github.com/SimpleMachines/SMF2.1/pull/4470/files#diff-cccb32785017365b9e98cf35be9ed3dfR4531

upgrade-11

unofficial updated Install.german.php to test

wintstar commented Jan 4, 2018

Missing language string_ in other/upgrade.php for
Updating Your SMF Installation!
https://github.com/SimpleMachines/SMF2.1/pull/4470/files#diff-cccb32785017365b9e98cf35be9ed3dfL218

Warning! https://github.com/SimpleMachines/SMF2.1/pull/4470/files#diff-cccb32785017365b9e98cf35be9ed3dfR3752

Database Changes
https://github.com/SimpleMachines/SMF2.1/pull/4470/files#diff-cccb32785017365b9e98cf35be9ed3dfR1286

search in upgrade.php file at "page_title" They are not have language strings.

I mean it´s not optimal the html start tag in language string $txt['upgrade_stats_info'] and the endtag in upgrade.php. Translators might think that a closing tag is missing and add this tag.
https://github.com/SimpleMachines/SMF2.1/pull/4470/files#diff-c1b42c4dcb08415dfddc398e9c1f0557R289

string ist double $txt['upgrade_done2'] , i mean $txt['upgrade_done3'] it should be
https://github.com/SimpleMachines/SMF2.1/pull/4470/files#diff-cccb32785017365b9e98cf35be9ed3dfR4531

upgrade-11

unofficial updated Install.german.php to test

@colinschoen colinschoen added this to the RC 1 milestone Jan 4, 2018

@Gwenwyfar Gwenwyfar added the Language label Jan 4, 2018

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Jan 5, 2018

Member

Haven't looked at it yet, but since you are moving things to txt files, you shouldn't use URLS or other variables such as that in those. You should use sprintf in the templates and have those pass the variables to the text strings: https://secure.php.net/sprintf

Member

jdarwood007 commented Jan 5, 2018

Haven't looked at it yet, but since you are moving things to txt files, you shouldn't use URLS or other variables such as that in those. You should use sprintf in the templates and have those pass the variables to the text strings: https://secure.php.net/sprintf

Another semicolon
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>
@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Jan 7, 2018

Member

@jdarwood007 There are some other strings used on install.php that doesn't use sprintf, for example: $txt['congratulations_help'] is used as <p>', $txt['congratulations_help'], '</p>';

Another example, same happens with $txt['install_settings_stats_info']. Should I try to convert those ones too?

Member

frandominguez03 commented Jan 7, 2018

@jdarwood007 There are some other strings used on install.php that doesn't use sprintf, for example: $txt['congratulations_help'] is used as <p>', $txt['congratulations_help'], '</p>';

Another example, same happens with $txt['install_settings_stats_info']. Should I try to convert those ones too?

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Jan 7, 2018

Member

I would do what is reasonable. URLs shouldn't be in the text strings as translators can and have made mistakes or changed them to a different url. If at all possible, the amount of "code" in language strings should be as little as possible. HTML isn't as bad, but if it can easily be left in templates, I would do so.

Member

jdarwood007 commented Jan 7, 2018

I would do what is reasonable. URLs shouldn't be in the text strings as translators can and have made mistakes or changed them to a different url. If at all possible, the amount of "code" in language strings should be as little as possible. HTML isn't as bad, but if it can easily be left in templates, I would do so.

@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Jan 7, 2018

Member

Alright, thanks for the advice SleePy!

Member

frandominguez03 commented Jan 7, 2018

Alright, thanks for the advice SleePy!

@albertlast albertlast referenced this pull request Jan 14, 2018

Open

RC1 Checklist #4485

10 of 13 tasks complete
Add load_lang_file() and fix a typo
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>
@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Jan 23, 2018

Member

Just added the load_lang_file function and edited a typo. Can't test this for the moment. I'll go with the txt issues in the next few days.

Member

frandominguez03 commented Jan 23, 2018

Just added the load_lang_file function and edited a typo. Can't test this for the moment. I'll go with the txt issues in the next few days.

Added sprintf(), completed load_lang_file() and some other stuff
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>
@frandominguez03

This comment has been minimized.

Show comment
Hide comment
@frandominguez03

frandominguez03 Feb 28, 2018

Member

Should be ready to go unless I missed something

Member

frandominguez03 commented Feb 28, 2018

Should be ready to go unless I missed something

@frandominguez03 frandominguez03 changed the title from [WIP] Removed hardcoded text from Upgrade.php to Removed hardcoded text from Upgrade.php Feb 28, 2018

@Sesquipedalian

Please sprintf() just the raw URL, not the whole HTML link.

Edited sprintf(). We want translators to do their job!
Signed-off-by: Frankie "d3vcho" <d3vcho@gmail.com>

@Sesquipedalian Sesquipedalian merged commit 384945c into SimpleMachines:release-2.1 Mar 1, 2018

2 checks passed

Scrutinizer 8 new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Gwenwyfar Gwenwyfar added the Upgrader label Mar 1, 2018

@frandominguez03 frandominguez03 deleted the frandominguez03:hardcoded_text branch Mar 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment