Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I hate having to look inside the function to see what the array that I have to pass in has to look like.
Also adds IMO useful links for people who are unsure what the first. resp. the second parameter has to look like.
Comment | File | Size | Author |
---|---|---|---|
#29 | 854396-29-date-docs.patch | 21.78 KB | tstoeckler |
#25 | 854396-24.patch | 22.04 KB | tstoeckler |
#21 | 854396-21.patch | 17.45 KB | jhodgdon |
#13 | documentation_system_date_stuff_2.patch | 19.65 KB | tstoeckler |
#5 | documentation_system_date_stuff.patch | 5.45 KB | tstoeckler |
Comments
Comment #1
tstoecklerWell, a patch would be good, eh...
Comment #2
tstoecklerFeels pretty silly to have to upload two patches for such a simple thing, but I thing we have a standard to document the data types.
Comment #3
tstoecklerIs there a prize for most worthless patch frenzy of the month?
Since we're already changing the function we should also add the third-person 's' to the function description.
Comment #4
jhodgdonThanks!
The latest patch needs a "the" in this line:
+ * A date format array containing following keys:
(the following keys)
Otherwise, I think the patch is good.
Comment #5
tstoecklerWell, it's hard to reroll such a patch without digging up other stuff. This fixes all documentation related issues in all of the date-related functions in system.module I found. Most of them are coding-style issues, but 2 functions still referenced the date type as 'normal', 'short', 'long' or 'custom', which is simply incorrect in D7.
Comment #6
jhodgdonActually, http://api.drupal.org/api/function/system_date_format_types/7 defines long, medium, and short types. So I think it is not a bad idea to leave these strings in the documentation?
Also, since you are fixing things up, let's get everything right. Here are some remaining problems after your patch:
a) Maybe we should be more specific about the "attributes" that system_get_date_types() returns, in its @return? All it says now is "an array of date types", which doesn't tell us much.
b) system_get_date_formats - one-line description still refers to the "format length", which is incorrect.
c) "id" is a psychological term (ego, superego, and id). The abbreviation for identifier should always be ID. Check your dictionary... so this needs fixing:
+ * Gets the format details for a particular id.
d) This could be expanded to say what the "details" are:
+ * An array of date format details.
e) This needs a verb (is):
* The format string, or NULL if no matching format found.
f)
Date type or date format type? I think there is a difference?
Comment #7
tstoecklerWill roll a new patch soon, just some replies for now:
Using 'short', 'medium' and 'long' in these functions works just like it did in D6. But if you add new date types (which you couldn't in D6) you don't input 'custom', but 'name-of-date-type'. That is why I think the previous documentation was misleading (especially for people who were accustomed to how it worked in D6). Maybe we can do both:
No there is none. The terminology is currently horribly inconsistent. The current usage is:
In the UI: "Date types"
function names: date_format_type
variables: both $date_format_type and $type (and I think also $format_type).
Maybe for D7 we can at least clean up the internal variable names. Although I don't know if changing the name (but not the usage) of a parameter counts as an API change. Also we would have to decide which is better (date types vs. date format types) and then change either the interface or the function naming in D8.
The rest of the comments will be incorporated in the next patch.
Comment #8
jhodgdonHow about saying something like "For example, the system module defines 'long', 'medium', and 'short'; other types can be defined in the user interface and by other modules."
Regarding the date types vs. date format types... There is a difference. A date format is something like what's in system_default_date_formats(). A date type is something like what's in system_date_format_types(). They can be associated with each other, but they are not the same thing at all. The terminology needs to be made consistent. There are two hooks too: hook_date_formats() and hook_date_format_types().
Comment #9
tstoecklerSorry, but you got that wrong. I'm only talking about 'date format types' here. If you go to the date and time page in Drupal you can click a link saying "Add date type" not "Add date format type". Also like I said, the variable naming is inconsistent for date format types (not date formats).
Comment #10
jhodgdonLooking just at the date-related functions in system.module, there are actually three different types of things that are sometimes called "date formats".
1) Results of hook_date_format_types -- key/label pairs, along with attributes like the module they came from. No PHP date formats are part of this structure, so I think the name "date format type" is really stupid. They are things like 'long' => t('Long'). Functions that deal with these:
- system_get_date_types()
- _system_date_format_types_build()
- hook_date_format_types() (and system_date_format_types(), which implements this hook)
- hook_date_format_types_alter()
- system_date_format_type_save()
2) PHP date format strings (with keys). Functions that deal with these:
- hook_date_formats() (and system_date_formats(), which implements this hook)
3) The associations between #2 items and #1 items. Functions that deal with these:
- system_get_date_formats()
- _system_date_formats_build()
- system_date_formats_rebuild()
- system_date_format_save()
- system_date_format_delete()
- system_get_date_format()
- system_date_format_locale()
- system_date_format_type_delete() [primarily for deleting a #1 item, but also deletes the associations]
We need a consistent nomenclature for these three types of items.
Comment #11
jhodgdonNot to mention that the function format_date in common.inc is still hard-wiring 'long', 'short', and 'medium'. Can that possibly be correct?
This looks like a real mess, and it is not only documentation.
Comment #12
jhodgdonSee also
#826486: format_date() does not respect admin-defined date formats
Comment #13
tstoeckler...Dear lord, I've never written a 20k doc patch...
I think this is as good as it gets for D7. This provides extensive return information and renames the internal variables consistently. I can hardly imagine this is counts as an API change. The proposed naming is:
$date_type (e.g. 'short') < $date_format (A format including locale and date_type information, etc.) < $format (A simple format string).
While doing this I noticed that there is another inconsistency, namely language vs. locale, but it's not really misleading and it's not that prominent anyway.
This patch also fixes one bug, but since I was renaming variables anyway it seemed silly to stop there (l. 3558, system.module):
For date formats that are declared by multiple modules _system_date_formats_build is currently merging the existing array with $format. But $format is undefined, it has to be $module_format as is very evident when looking at the function (http://api.drupal.org/api/function/_system_date_formats_build/7). I would roll a patch without that if necessary, but ...
This definitely needs a completely overhaul in Drupal 8, but with this patch I think Average Joe (c) should be able to make sense of the commonly used functions.
Comment #14
Dries CreditAttribution: Dries commentedI don't understand why you renamed all instances of $type to $date_type? I prefer $type and would prefer to see you undo those changes. Ditto for changing $format into $date_format.
This should be 'dfid' instead of 'dfif'. This typo is repeated a few times.
Is that correct? Shouldn't 'dfif' be 'dfid'? This typo is repeated a few times.
I don't understand why you renamed $type to $date_type. $type is cleaner IMO.
Powered by Dreditor.
Comment #15
tstoecklerI can live with $type but not really with $format for the above mentioned reasons. To explain in detail:
We currently use $format for both, which leads to confusion (at least it confused me), that's why I introduced the discrepancy between $format vs. $date_format.
Comment #16
jhodgdonI agree with tstoeckler on #15. There are three different kinds of information floating around with date formats, and I think they need to be distinguished. Having different variable names for each kind of information will help in that effort.
Comment #17
tstoecklerEven though I personally find #13 to be the best solution, something that Dries might like:
$type vs. $format vs. $format_string ?!
Comment #18
jhodgdonThat works for me... Though maybe $format_info or $format_item would be more descriptive for the array of format information?
Comment #19
tstoeckler$format_info vs. $format_string. That actually gets a +1 from me. That simple change would have saved me around half an hour when porting Clock module. And then together with $type Dries will be happy, too!?
Comment #20
jhodgdonLet's try it out in the form of a patch and see if it flies. I actually haven't reviewed the previous patch yet...
Comment #21
jhodgdonHere's a new patch.
It has fewer local variable name changes than the previous patch, and more changes to the documentation. I've attempted to be very clear about what is a PHP date format string, what is an array of information, and what is the name of a date type.
Comment #22
jhodgdon#21: 854396-21.patch queued for re-testing.
Comment #23
tstoecklerNot (yet) touched by this patch, but that's a pretty weird sentence.
If the function always returns FALSE if $langcode is not specified, why is it optional?
I just looked at the patch for now. I'll dive into the code now and then reroll.
Powered by Dreditor.
Comment #24
jhodgdonCan you provide just $type?
Comment #25
tstoecklerI rewrote the documentation at system.api.php a bit. I left the system.module hunks alone as I think this is as good as we can do in D7.
My second question in #23 is valid, but it would be an API change to clean that up.
Let's go with this and clean up the whole mess in D8.
Also, thanks jhodgdon for bumping this and working on it, I had long since forgotten!
Comment #26
jhodgdonThis is looking pretty good. A few mostly minor suggestions:
a)
"An" should be capitalized (list items should start with capital letters, and the rest of this list is fine).
b)
Alters -> Alter. Hooks are written as if to say "You can use this hook to...".
c)
Looking at the code for http://api.drupal.org/api/drupal/modules--system--system.module/function... -- it looks like it returns an object and not an array? It's using fetch(), which I think should be returning an object.
Comment #27
jhodgdonComment #28
jhodgdonI was going to fix up these few problems... but I'm having another problem with the patch. After patching, for some reason my system.api.php file is ending up with Windows rather than Unix line endings. The other two files are not having that problem... could be an artifact of Eclipse I suppose, but I'm not sure. So I guess I'll leave this to someone else to fix up.
Comment #29
tstoecklerHere we go.
Sorry, I had forgotten this issue (again!).
Comment #30
jhodgdonThis looks good to me, thanks tstoeckler!
Please note that although it looks like code changes occasionally in this patch, it's really just local variable names and doc blocks that are changing. There should be no API or functionality change. It should be clearer what's going on in the date format world after this patch, though.
Comment #31
tstoecklerThat is correct, except the above hunk.
I noted this above, but to reiterate for people coming late to this issue, that is actually a bug fix. If you look at _system_date_formats_build() you will see, though that
1. This code is only reached in edge cases (two modules provide the same date format)
2. The fix is incredibly obvious, because $format (which HEAD currently uses in the array_merge()) doesn't even exist in that function.
Since we were changing the internal variables anyway it would make no sense whatsoever to not make that change.
Thanks for the RTBC. Let's finally get this in!
Powered by Dreditor.
Comment #32
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Feel free to re-open based on #31. Thanks all.
Comment #33
tstoecklerBy #31 I just meant that, strictly speaking, because of that one hunk, this patch is not only docs changes.
I still advocated committing it and see no reason to re-open this.
Sorry for being unclear about that.
Comment #34
jhodgdonRelated issue filed today:
#989366: format_date() doesn't use programmatically created format.