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

Feature request 1312: Added configuration option for setting the timezone of the current database connection #1495

Merged
merged 1 commit into from Feb 10, 2015

Conversation

dbroeks
Copy link
Contributor

@dbroeks dbroeks commented Feb 9, 2015

@ibennetch
Copy link
Member

This seems reasonable to me but I hope some others developers comment as well.

Could you also add this directive to doc/config.rst ?

@dbroeks
Copy link
Contributor Author

dbroeks commented Feb 9, 2015

Sure, I will do this tomorrow.

* time zone of your database server.
*
* Useful when your database server uses a time zone which is different from the
* time zone you want to use in PhpMyAdmin.
Copy link
Member

Choose a reason for hiding this comment

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

Lines 500 and 501 probably can be removed from libraries/config.default.php and moved to doc/config.rst. We don't need full documentation here; just enough to give hints. Users can always refer to the full documentation if they don't understand a directive.

My suggestion is that here we say "Sets the time zone used by phpMyAdmin. Possible values are explained at http://dev.mysql.com/doc/refman/5.7/en/time-zone-support.html" -- and move the rest of what you wrote to config.rst.

@lem9
Copy link
Contributor

lem9 commented Feb 9, 2015

The patch seems reasonable to me too. I appreciate the fact that if left empty, the directive will do nothing (we don't want unnecessary queries sent to the MySQL server).

);

$GLOBALS['error_handler']->addError(
__($error_message_tz),
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure about this, but don't think this will work properly for translating. Normally you'd do something like sprintf(__('Unable to use...')). That way, gettext can properly format the plural forms and digits.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 57.87% when pulling 858d225 on d-bro:timezone into eca87c1 on phpmyadmin:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 57.87% when pulling 858d225 on d-bro:timezone into eca87c1 on phpmyadmin:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 57.85% when pulling 858d225 on d-bro:timezone into eca87c1 on phpmyadmin:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 57.87% when pulling 858d225 on d-bro:timezone into eca87c1 on phpmyadmin:master.

@lem9
Copy link
Contributor

lem9 commented Feb 10, 2015

Thanks. Could you clean up your commits, producing just one?
See https://wiki.phpmyadmin.net/pma/Git#Changing_history

…zone of the current database connection.

Signed-off-by: Dennis Broeks <dennis@uitzendbureau.nl>
@lem9
Copy link
Contributor

lem9 commented Feb 10, 2015

Thanks.

lem9 added a commit that referenced this pull request Feb 10, 2015
Feature request 1312: Added configuration option for setting the timezone of the current database connection
@lem9 lem9 merged commit 80da5e5 into phpmyadmin:master Feb 10, 2015
@lem9
Copy link
Contributor

lem9 commented Feb 10, 2015

See also 26f7584

@dbroeks dbroeks deleted the timezone branch February 12, 2015 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants