Conversation
… present in the config file to allow for backward compatibility.
|
My build is failing, but I'm supposing this has something to do with #2627 since it's failing on |
|
couldnt one maybe instead of having the same time stuff written all over the place instead have one time utility function which globally does time and is just called from everywhere needed? would make it more maintainable as you dont have a metric ton of instances of the code you have to manually replace and stuff |
|
Might be cleaner yes @My1 . There's currently 4 parts of code in 3 files that I had to mainly edit. They generally require a timestamp, but they are formatting all in a different way. Eg. in So there's 3 parts were we need a timestamp and 1 part where we need the offset in hours. Now that I'm looking back at my code, I think that a time utility function would indeed be great since the whole if/else structure is basically duplicate. The utility function should have an epoch timestamp as input and output a 'timezoned timestamp' (based on the current if/else loop). Should I add this utility function to the Will start working on it already and put it in |
|
does this thing deal with local times in the db? lol for formatting one could maybe do a date style implementation, like for example also if done well one could set a default format somewhere to have it nice and clean everywhere |
|
Well, in the DB everything is saved in GMT time. In the code itself however, epoch timestamps are used. About that general date style implementation: this is probably a good idea when rewriting the core, but I think that will be a bit intrusive for now IMO. |
|
oh okay it just sounded a bit weird that the time offset was needed for something in the db when everything is GMT anyway and when in the code unixtime is used. maybe if the core gets a rewrite all datetime things could just be unix timestamp in db and not needing to hurl around with ANY formatting exceprt for display |
|
True! Currently working on a new patch, will push soon. Looks a lot cleaner with a single |
|
To summarise: time zone support was rewritten to use one out of the 2 new functions in |
cc498e2 to
c3fca0d
Compare
|
Interesting ! Much thanks for this. I'll have a closer look soon |
|
My pleasure, thanks! :) |
Pros & ConsLet me first state that I am 100% in favor of migrating YOURLS from offset based to Timezone based. If done right I see some advantages. If done wrong I see additional problems. That said, I think we should look at the Pros and Cons of this PR. Pros It takes a step toward using Timezones. @JeremyKeusters made a good first effort. Cons Added Constant Ozh has many times said we need fewer constants, not more. Because constants are problematic, and hard for plugins to change. Many YOURLS Admins have no more PHP knowledge than running a WordPress site that someone else set up. Asking them to go to a php.net site is not likely to get a well-formed answer in all cases. Not to mention typos and copy/paste errors made by noobs and PHP experts alike. This creates more issue reports. Solution: Store Timezone as a DB Option. No constant is needed. Unnecessary Constant I have used a part of YOURLS as a run one time only. This could be used as a conversion program without changing YOURLS performance. Solution: The same one time only script could be expanded for new sites into a 4-minute install! Code Bloat & Two Systems @My1 this would unify timezone stuff on the DB Option which YOURLS already handles. |
|
Thanks for the feedback!
If a user can create a MySQL login & a database and put those values in a config file together with the MySQL hostname, he/she will also be able to go to a simple website and look for their official timezone IMO? I think you're slightly underestimating the average user here. There's no way that you can set up YOURLS without having at least some technical knowledge/feeling. If really needed, we can add some extra explanation and/or an example:
This would require a whole re-do of the current config options (and put them all in the DB), you're not just going to put only the timezone option in the DB. You'll also need an admin interface to configure this and a full-fledged installer in which you can configure the setting. This would indeed be nice, but this is something for the long run, maybe for 2.0. It would be nice if Time Zone support could come in before 2.0, since it's an issue that's been open since 2013.
Again, you would need to create an admin interface to show this pulldown list.
Following your reasoning from above, you would need to move the offset too to the DB and all my remarks would be applicable again. How is an offset more noob friendly than a timezone? With offset, I'm pretty sure that most of 'your average users' need to look up first how many hours they are differing from GMT, maybe some even need to lookup what offset and what GMT means. Then the confusion comes when they read that they are for example +7 hours from GMT. "Should I write +7 or 7? Or should I write '7 hours'?" Also: imagine that they look up their GMT offset and see that it's GMT+2 in DST and GMT+1 in standard time? "What offset should I take?" |
|
Perhaps you should look at some of the long standing YOURLS issues. For
example, why do so many people still put a slash at the end of their URL
when YOURLS expressly tells you not to?
These show the kind of Admins you are actually dealing with.
I never suggested moving the offset to the DB. Before my back to back
Strokes I was working on moving these constants to the DB Options. One by
one. As I am recovering I'm looking forward to completing this.
If we choose to put the Timezone in the database, it becomes just a matter
of a noob choosing a timezone on the screen before them (like WordPress
does). Plus, anything a noob selects works, no possible failures. Typing a
constant allows fatal errors.
Ozh has stated a number of times that we should do more to make YOURLS more
noob friendly.
Most of your other statements are hard to understand.
*"If a user can create a MySQL login & a database and put those values in a
config file together with the MySQL hostname"*
Hosting usually gives DB info to you. I was working on an install just
exactly like the 5 minute WordPress install, except I think it will take 4
minutes!
Likewise, I have a working YOURLS that runs on a Linode and WordOps. Very
easy to install. WordOps loads, configures, and maintains Nginx, MariaDB,
etc. So noobs would never need to do anything in config.php
*"You'll also need an admin interface to configure this and a full-fledged
installer"*
*Admin Interface* already exists in YOURLS. But I would recommend putting
this in a Plugin as it only will be used by a limited amount of sites.
*Full-fledged installer* is part of the overall roadmap already. I have it
substantially written.
Adding constants to config.php will interfere with that and will need to be
removed.
Everything I have said in my previous Cons section could be fixed today, no
need to wait for 2.0, leaving a cleaner, better core.
Ok maybe it would take a few days, or a couple weeks, or even a couple
months!
*"you would need to move the offset too to the DB"*
Not true, I would delete the offset and only use a timezone, stored in the
DB Options. Using the DB Options in YOURLS is easy.
Did you look at how WordPress does this? How you set your Timezone in
WordPress?
Settings =》General
For me I would just select
Asia/Manila
That is the same as WordPress.
No need to know that I am UTC+08:00 without DST. Just need to select a
nearby city in my country with the same Timezone, noob friendly!
That is what I am recommending.
But hey, the Core Team makes the decision as to what PRs are accepted. I
think the Core Team does a fantastic job! I have a very high respect for
their decisions. I trust them to do what is best for YOURLS.
YOURLS is the Best!
…On Mon, Mar 30, 2020, 17:43 Jérémy K, ***@***.***> wrote:
Thanks for the feedback!
Many YOURLS Admins have no more PHP knowledge than running a WordPress
site that someone else set up. Asking them to go to a php.net site is not
likely to get a well-formed answer in all cases. Not to mention typos and
copy/paste errors made by noobs and PHP experts alike. This creates more
issue reports.
If a user can create a MySQL login & a database and put those values in a
config file together with the MySQL hostname, he/she will also be able to
go to a simple website and look for their official timezone IMO? I think
you're slightly underestimating the average user here. There's no way that
you can set up YOURLS without having at least *some* technical
knowledge/feeling. If really needed, we can add some extra explanation
and/or an example: eg. Europe/Brussels.
*Solution:* Store Timezone as a DB Option. No constant is needed.
This would require a whole re-do of the current config options (and put
them all in the DB), you're not just going to put only the timezone option
in the DB. You'll also need an admin interface to configure this and a
full-fledged installer in which you can configure the setting. This would
indeed be nice, but this is something for the long run, maybe for 2.0. It
would be nice if Time Zone support could come in before 2.0, since it's an
issue that's been open since 2013.
*Solution:* Use the PHP Timezone functions to generate a pulldown list to
select a Timezone (like WordPress does).
*Solution:* In addition, when PHP is updated, the Timezone list and
pulldown menu are also updated automatically.
Again, you would need to create an admin interface to show this pulldown
list.
*Solution:* The Offset is used correctly by a minority of sites. this
Timezone change code could be placed in a plugin (for the minority who want
it) or in the Admin section. This would also be noob friendly.
Following your reasoning from above, you would need to move the offset too
to the DB and all my remarks would be applicable again. How is an offset
more noob friendly than a timezone? With offset, I'm pretty sure that most
of 'your average users' need to look up first how many hours they are
differing from GMT, maybe some even need to lookup what offset and what GMT
means. Then the confusion comes when they read that they are for example +7
hours from GMT. *"Should I write +7 or 7? Or should I write '7 hours'?"*
Also: imagine that they look up their GMT offset and see that it's GMT+2 in
DST and GMT+1 in standard time? *"What offset should I take?"*
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2633 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHYYPQGB34N4LJ2EIJWFRVDRKBSSZANCNFSM4LVXTX4A>
.
|
|
@JeremyKeusters Hey Jeremy, They say a picture is worth a thousand words, so I had a few minutes today and I thought I would paint you one in PHP. The Timezone dropdown menu I'm going to show you took 5 minutes to write. It has 4 lines of code and 178 Characters (including spaces!). I want you to try to look at this through the eyes of a real noob. Method One: That has lots of ways for a real noob to mess up. Or This... Method Two: Impossible for a real noob (or anyone) to break YOURLS with this. Note: this pulldown is generated by PHP so it is always correct. Likewise, I have added no styles to make it look good, I'm sure we could do better! |
|
I have been looking at your PR code and I think it shows real promise. I have a few questions: 1. Refactoring to Seconds Your code takes the timezone offset, already in seconds, and converts it to hours. Wouldn't it be cleaner to do one conversion of 2. Avoid Adding Constants Why not combine them? For a Philippine example: Where it is used, test if it is an integer (hours offset) or text (timezone). If '0' or empty then set timezone to UTC. 3. This would indeed be nice!
It might surprise you to know that this was already written in 2018. the site, under test for about 18 months, moved and refactored each of the constants in config.php and put these in DB Options. It has full automatic conversion, therefore it is noob-friendly. Added features include Multisite (one set of files, multiple domains, multiple sets of keys), Full-Fledge Easy 4 Minute Installer, co-location of WordPress and YOURLS in the root directory of a single domain, without any restriction to either program (People say this cannot be done, but it runs just fine with only two copy/past lines of code!) The Problem was, the PR is Huge! After a long discussion with @ozh he advised me to break this into small PRs that focused on one thing at a time. I had just started that when I had a stroke, and a few months later another stroke. For the last year, I have had to focus on higher priorities like learning to talk, walk, and eat a completely different way. The point is, that code still exists, and I could extract the pieces for the timestamp project. So we could do timestamp first and "nice" could start in March or April 2020! Would you first look at 1. and 2. above. that would make 3. easier. |
|
I think I'll build on @JeremyKeusters patch, to leave in core functions like his Benefits:
|
|
that is a good way. offer a standardized way to get time and let plugins do the rest. btw if doing such a thing maybe add a way to change the timestamp format into the function as well so people can have time formats they want (by using the appropriate plugin). |
|
Great solution balancing both my PR as well as PopVeKind's remarks. Let me know if I can help with the plugin, I currently have time to work on stuff like this. We can as @My1 suggests make this bigger and change the |
true enough although some forums or other CMSes or whatever I saw offering 2 options, long and short date format (ignoring the fact that delta format [aka, "x time units ago"] also exists). and for example the idea would be that yourls would have one long and one short default time format like
if datestring is set, ignore below (like when a call to the timezone needs a specific time format is needed at some place for whatever reason), otherwise continue if no datestring is set let's just go for short as an unintentional long can break display, an unintentional short breaks less if the format is one of the constants and a display format for that has been set by plugin or config or whatever, use that, if no display format is set for the given type, use the defaults |
|
What I used for my dropdown. Not pretty but maybe it will help. |
|
Only the timezone will be stored in the DB. |
|
Thanks for the additional explanation @ozh !
I already use this, but thanks.
As @ozh already said, we are only storing the timezone in the DB. Offset will be completely deprecated (in the future). When not using the timezone plugin, it will simply use GMT time. |
|
@JeremyKeusters I propose the following code to display the dropdown: https://gist.github.com/ozh/9981e14eff488e6e071913b817a18fbc This needs to be improved : output should be translatable (ie |
|
@ozh That is an interesting pulldown list. I have suspected that UTC+08:00 was an undocumented but a valid timezone. yourls__('strings') is an interesting idea, perhaps PHP has a language option? yourls__('strings') could be worked into the code I presented above. Manual Timezones are not needed and cause more Issue reports. Although this pulldown is better than "type in your timezone"... Are Manual timezones needed or even desired? This dropdown creates an option for every imagined timezone that is 15 minutes apart. However, many or most of these are not used by anyone. WordPress uses a custom list of manual timezones and updates this in the WordPress code as needed. However, Is this desired? The first half of this dropdown presents the same information as the one I used (see the example page. Notice these are sorted by region. People complain about the WordPress dropdown because as you scroll through it, you lose track of what region you are in (the regions drop off as you scroll). Do we want to be just like that? The example page contains all the timezones that are used in the world, maintained by PHP using the official IANA list. Why are "Manual timezones" needed? They only confuse people (noobs) and they do not do DST/Summer Time. They have the same problem that offset has. The offset would accept a value of 5.75 (same as UTC+05:45) but No DST adjustment. Supporting unnecessary choices (Manual Timestamps) has only caused more WordPress questions, issues, and bug reports that needed to be answered. For example: Issue: "I found a bug in WordPress. I live in Los Angelus, California and correctly selected the manual timezone of UTC-08:00 on the dropdown list. However, my time is still wrong." Answer: "Don't use Manual Timestamps. Use Problem: Do what you like, I won't complain (any dropdown menu is far better than a constant). However... |
|
Selecting UTC and not having DST is the expected behavior. DST are regional choices. Selecting UTC is made available for specific cases when people want to pick a UTC offset but not comply to a timezone DST setting. This will be requested by people who care about time offset. |
|
@JeremyKeusters if I understand correctly, this PR will also need a change in every SQL query made where a timestamp is stored.
|
|
@ozh last time I checked, I noticed that the timestamps are already stored as UTC time? So no change is needed in the current SQL queries? The offset I was talking about is for a |
|
@JeremyKeusters totally possible. I'm a true SQL noob :) |
|
@ozh If you want me to leave the YOURLS community just ask and I'm gone. Otherwise please consider who, under what circumstance, would use
What Specific Case? PHP has included "ALL" offsets, those with and those without DST. I just wonder if anyone can give me a real-world example of why anyone would want to use this, within YOURLS? I'm not trying to be difficult, I'm just trying to understand. The DST is only applied to countries that have laws requiring it. That is why
For that one person doing something really weird, there are alternative timezones that can be selected from the PHP "ALL" list. It seems like a lot of extra work and code bloat, in all YOURLS installs, just to output something that will not be used and will confuse Noobs. Or UTC-Offsets could be put in a user-provided UTC-Offsets Plugin, for that one person doing something super weird. Have we changed from helping Noobs to confusing them? Just because we can make this list that won't actually be used, does not mean we should do it. Or is it just because you don't like my long explanations? I know ozh and I did not see eye to eye in the beginning, till I started understanding his reasoning. But is that a good reason to add code bloat, that likely never will be used, and will confuse noobs? All I ask is to be considered like everyone else. I ask anyone to explain why this core plugin code bloat, will actually (not hypothetically) be used by you in your YOURLS install? That seems a fair request for a real community. |
|
2nd Thought: May we leave this plugin as a contributed plugin? So this unnecessary and unused UTC-offset code bloat, as a core plugin, does not have to be downloaded and installed on every YOURLS install? |
|
well but one does have to note that finiding a named time zone that is with/without DST for any given offset can be a pain. so the ability to just specify a dumb offset may not be too dumb. |
Open to discuss this off site if you want, I'm at ozh at ozh dot org. Let's keep in mind that every comment here is mailed to 300+ people so we should keep it concise, on topic and constructive. |
Will try to implement this code! I like the fact that the time zones are grouped in the dropdown. I'm however not fond of the AM/PM time between brackets. Do we need the 'actual' time altogether? The time doesn't update 'live', so it takes the time that the user opened the page, which might be confusing? I would just leave it out, not sure what you/others think about this? I mean, I suppose that people know in which timezone they live based on their city? What we could do is show the time that will be used beneath the dropdown once the user has selected a time zone. Something like this: Without a time zone selected: With a time zone selected (Europe/Brussels in the example is GMT +2) |
|
I'm currently experimenting with the code you provided in combination with a display of the current time using the selected timezone. I'm however quite a noob in jQuery, and I'm not sure how to combine PHP and jQuery. I suppose that I need jQuery for updating the time as soon as the time zone is changed/selected in the dropdown (we want the time to be displayed live, not requiring the user to reload the page), but I'm not sure of the compatibility between the time zones in PHP and the time zones in jQuery/Javascript. Also, we can't use the UTC options with this current solution. |
|
personally I would prefer a normal settings form -> save approach, as you can actually see whether or not it properly saved and it not leaving you in a weird state I am not even aware how much this thing actually relies on js compared to PHP |
|
@JeremyKeusters a way to simplify the JS would be to generate the dropdown list with a |
|
A new push in which I implemented the new time zone dropdown that @ozh provided, but left out the 'live' time indication for now. Also implemented the 'wordpress' alike Date and Time format options. The date and time format options are saved in the database, but not used yet. Time zone is only used for the main page which lists all urls. Small screenshot for those who just want to have a quick idea without running the code: @ozh How do you see the further implementation of the date and time format options?
|
|
@JeremyKeusters Looking neat! Point 2 : in core we just need "simple" functions to output stuff, with filters on which plugins can hook, and that will not change current behaviors if there's no plugin. Something like function yourls_get_formatted_date($timestamp) {
$format = yourls_apply_filter( 'get_formatted_date_format', 'M d, Y' ); // tap into this with the plugin
return yourls_apply_filter( 'get_formatted_date', date( $format, $timestamp ) );
}Regarding default formatting option, the best would be to keep things unchanged for people who won't use the plugin. Seems to be |
|
@JeremyKeusters hello man, just checking in to see if everything is OK. Feel free to ask for help if needed ! |
|
@JeremyKeusters ping |
|
Everything okay, yes! :) I'm sorry for not replying, I'm currently working on a small project that shouldn't take too long. After that, I'm planning on spending some time on this again! |
|
Closing here, continuing in #2684 |

This is a pull request for an implementation of time zone support and should resolve #1344 .
A new config parameter was added to the
sample-config.phpfile calledYOURLS_TIMEZONE. When this value is left empty, the value ofYOURLS_HOURS_OFFSETis used, which allows for backward compatibility. This allows to keep usingYOURLS_HOURS_OFFSETand avoids the need for all users to add the new config parameterYOURLS_TIMEZONEto their config file.I first experimented with DateTime objects, but eventually went for a solution that calculates the offset of the timezone such that most of the original code can be kept, which reduces the risk of bugs.
Feedback and reviews are highly welcome, PHP is not my main language and I'm not sure either about the performance impact of my fix. Thanks!