[policy] Use @since tag for new functions/arguments added in point releases (>7.0, >8.0, etc.)

Issues for discussing new coding standards need a tag. :)

So, we need some buy-in by the 7.x branch maintainer, the 8.x branch maintainer (after 8.0 is released), and the core development team in general that they are committed to adding @since tag to new functions going forward. tsvenson has volunteered to take care of patching things between 7.0 and now.

I give a +1 to the idea -- tsvenson has made an eloquent case for why this would be useful, and points to php.net as an example whose function docs have this information. But the idea won't be good if the core dev team is not committed to maintaining it going forward... I've also committed to implementing the @since on api.drupal.org if it's adopted. :)

Proposed standard:
------------
For any new functions, methods, constants, etc. that are added to a branch after the first full release, add the following to their documentation header, at the bottom:

* @since 7.2
*/

-------------

The problem is that when someone is developing the function, they don't really know what version it will get into. So they'll need to kind of guess, and then in the final commit I guess the committer will know for sure if it's the right version and could adjust? Hmmm...

You should be able to know what the next core release would be based on logic and the last known release, especially since we announce security releases ahead of time too.

RE #2 - yes, at the time the patch is committed, definitely you should know what the next release number will be. But at the time the patch is first written, you might not know when it will be committed. :)

RE #3 (placement of the @since) - I think it could be displayed at the top of api.drupal.org pages, but I think the bottom, where the @see, @ingroup, and other similar tags go, is the right place to put it in the comments.

OK. So the next thing to do is either:
a) Propose an alternative to the standard proposed in #1 or
b) Gather some representatives of the core dev community and core committers to agree to this as being a good standard for Drupal 7 and beyond.

Once that's agreed upon, we can then move forward on a patch.

Here is how to find when a function was added in, as well as much other rich information, without requiring developers to maintain the data in multiple places, and also without needing to clone the repo, which tsvenson expressed concern about doing.

1. Go to http://drupalcode.org/project/drupal.git.
2. Click on "tree."
3. Browse the file tree to the file where your function or hook definition lives, e.g. field.api.php. (You can see this information on api.d.o if you are not sure.)
4. Click the "blame" link at the top.
5. Use browser to search on the page for the name of your hook or function, e.g. hook_field_widget_form_alter (without parens).
6. Click on the commitref in the margin for the function name. (Here the whole docblock down to the function name is in one commit.)
   

You now have the exact point in the history of Drupal that the function definition was added or updated, with no need for anyone to maintain the information redundantly in the codebase. Edit: if it's an update, you can also see the last update by clicking the blame link again.

Edit: Boiling down to bullets point since I got kind of ranty there, sorry. :)

• It would be a hassle to guesstimate and reroll and update the point release in which a function was added.
• The information is already available in gitweb online.
• The information is already available in local git repositories.
• The information is already available in our change notifications: http://drupal.org/list-changes/drupal.
• The above three already include issue references and much other useful information that is not available by adding a number to our API docs.
• API additions in point releases are very infrequent already.
• The overall frequency with which developers need this information is very low.
• Maintaining version information in a function summary seems unwise to me, to the point of busywork, and this is coming from someone who rerolls patches to add the letter S to other people's verbs.
• I don't want to discourage people from working on a solution to expose this information if they think it is worthwhile, but it would be much better to improve change notifications (say, adding a field where you could enter added function or hook names) or expose the information from gitweb in a slightly nicer way (say, matching the next tag after the blamed commit).

xjm: thanks for writing up that explanation -- we should put it into the Community Documentation somewhere, along with an explanation of how to use Git Blame. And then make sure a link to that page is on api.drupal.org in a prominent location.

I don't think that it is actually all that feasible to add blame information directly on api.drupal.org, though, if that is what you were suggesting. I am not sure how the API module could (a) figure out what git repository to use or (b) figure out which exact blame command to run or (c) display the output. It's just not that tied in with git, and I don't think we probably want to build that... it seems complicated?

Maybe not in api.module itself; maybe in an added form on the page. The API doc already has the line numbers the function belongs to, no? We don't expose it, but it knows. A script could run a git blame for that line, get a hash, look forward for the right tag.

Edit: I am just brainstorming; as I said, this information is so rarely relevant or needed that I don't think it's worth doing at all, personally. But the nice thing about Drupal is that if someone else really thinks it's a good idea, they can work on it and we will still help them out. :) That is, to me, much better than putting the burden on developers who will probably look in change notices and/or their git log.

Just to clarify... The API module does not know what git repository the code it displays is coming from, and indeed it is quite likely it comes from more than one git repository (or thousands, in the case of some sites running the API module). So it will not really be feasible for api.drupal.org to display information from git blame.

If we want the information to be displayed directly on api.drupal.org, then we will need to incur the maintenance cost of putting the @since tags into the functions. Again, the proposal here is *only* to add it to functions added since the original full release (i.e., functions added since 7.0 for the 7.x branch). We're talking about something like 10 functions.

The development burden, as I mentioned in comment #1 above, is that the developer of the patch does not know when it will be committed, so it is not easy for them to do the @since. So it pretty much has to be done at the last moment or by the committer (or the committer may need to update what is in the @since tag). That is one more thing for committers to think about... not sure if that is acceptable to Dries, webchick, and catch (won't affect me since I'm not committing these types of patches).

On the other hand, as xjm has been pointing out in IRC, if there are only 10 functions that have been added so far in 7.x since 7.0, do we really even need to bother with this? How likely is it that a contrib module developer is (a) going to know about and use one of these new functions and (b) someone will be running an outdated 7.x version of Drupal and their module won't work? In that case, the user can just update Drupal to the latest version and they will be fine... so ... what's the problem this is solving, really?

I would pain to have API module be hard-coded to running git shell commands (and hence to Git) just to display information that is already a well-supported PHP documentation standard and would only affect less than 10 functions for the entire Drupal 7 code base (we haven't added that many things since 7.0)? I feel like we are severely overestimating the effort here. We don't need to add @since tags to every single new function being added to Drupal 8 right now, only new functions added *after* Drupal 8.0 is released. API module already gives us the major core versions that a function is available from so we don't need to add more work and be adding this to every single function in core.

For Drupal 7 this would affect the following functions that I know of:
hook_field_widget_form_alter() - 7.7
menu_tree_set_path() - 7.9
menu_tree_get_path() - 7.9
format_string() - 7.9

A demonstration of how @since is used on PHP.net. This is the array_fill_keys() function. It was introduced in PHP 5.2. I can see that information right up front and don't have to scan the comment section to figure out that I can only use this function if my module requires PHP 5.2. It should be just as easy to see that on api.drupal.org.

For concerns about getting the version number right before the version has been tagged, I did a bit more research I've found that Wordpress (http://codex.wordpress.org/Inline_Documentation) and other PHP projects use @since {@internal Unknown} which is then replaced after the release has been made (so in theory the -dev release always contains the proper version).

Alright, I said I was unfollowing this, but davereid (as usual) talks unreasonable amounts of sense. I would be less, uh, tense, toward something that machines can do for us. The remaining disadvantages I can see are:

• Still have to add the @since insert_placeholder_here (I guess this becomes a part of the reviewers' and committers' tasks when checking for API changes to make sure it's there. Upshot, nice red flag for an API change.)
• Our branch maintainer has to add yet another task to rolling a release. :(

If we can come up with good solutions for those two points, then I will willingly help contributors comply with this standard.

Edit: I spelled davereid's name wrong, which is impressive.

I bet we could come up with some kind of helpful core-tag command or script that helps take care of some of the things that our core committers have to do normally (changing the VERSION constant).

This is an interesting thing which I'm not quite sure how to handle: http://drupal.org/node/336697 New, optional parameters to existing functions?

However, count me as a definite +1 for this initiative if we can figure out the syntaxes and whatnot. Like Dave said, this really doesn't come up /that/ much, and I don't even think we need to be so thorough as to define /every/ new function during point releases... just the ones that are API functions we intend developers to call.

The "we don't know if the next release is 7.13 or 7.14" thing is annoying, for sure. But whatever, there are worse problems to have.

OK, it sounds like we're converging on "Use the @since tag" as the right solution here, and we need to agree on some details. Here is a new proposed standard for discussion:

------------
For any new functions, methods, constants, etc. that are added to a branch after the first full release, and that are intended for use by module/theme developers, add the following to the documentation header, at the bottom:

* @since 7.2
*/

Notes:

• The developer adding the new function may not know what version their patch will be added to. So, they can use the following syntax:
  

  * @since {UNKNOWN}
  

  At the time the patch is committed, the committer should know what the next release version will be, and can change the {UNKNOWN} to the correct version number.

• If the function existed previously, but a new parameter has been added, the following syntax is used:
  

  * @since New parameter $foo added in 7.8
  

-------------

I'm not sure about that last one on the new parameter... Just thinking about how we would display this on api.drupal.org. I'm thinking the generic thing would be to turn @since whatever into a line simply saying:
Since whatever
near the top of the page, but this doesn't really work with the "new parameter added" bit. Thoughts/suggestions?

Also, I don't think it will be horrible if for instance webchick changes an {UNKNOWN} to say 7.13, and then it turns out it really should be 7.14 due to a security release. If someone notices, we can make a quick one-line patch later on. But at least we have conveyed the information that this function didn't exist in 7.2 for instance.

OK. New proposal for standard:
------------------
For any new hooks, functions, methods, constants, etc. that are added to a branch after the first full release, and that are intended for use by module/theme developers, add the following to the documentation header, at the bottom:

 * @since Function added in 7.2.

(replace Function with Hook, Constant, etc. if something other than a function was added).

If a function had a new argument added to it, use:

 * @since Argument $foo added in 7.10.

Note that the developer adding the new function, hook, etc. may not know what version their patch will be added to. So, they can use the following syntax:

 * @since {UNKNOWN}

At the time the patch is committed, the committer should know what the next release version will be, and can change the {UNKNOWN} to the correct version number.
-------------

+++ b/modules/update/update.moduleundefined
@@ -880,6 +882,9 @@ function _update_manager_unique_identifier() {
+ * @since Argument @create added in 7.2.

Should be $create

I just don't change the .gitignore, if I need to I use a global gitignore file.

+++ b/modules/system/system.api.phpundefined
@@ -504,6 +504,8 @@ function hook_admin_paths_alter(&$paths) {
+ * @since Argument $langcode added in 7.2.

I don't think this is new. I believe $langcode was supported in 7.0 but it just wasn't documented so this does not reflect a functional change?

+++ b/modules/system/system.api.phpundefined
@@ -2691,6 +2693,8 @@ function hook_file_load($files) {
+ * @since Argument changed from &$file to $file in 7.2.

This is not a necessary @since documentation needed. Objects are passed by reference by default in PHP 5.

+++ b/modules/update/update.moduleundefined
@@ -880,6 +882,9 @@ function _update_manager_unique_identifier() {
+ * @since Function moved from update.manager.inc in 7.2.

Don't think we need to say this now since it's available in the .module file now and not an include.

On your command line, if you do
git diff [files]
you can get a patch that does not include the .gitignore.

You can do git status to see what files have changed, too.

Once again:

+ *
+ * @since Argument changed from &$field to $field in 7.2.
  */
 function hook_field_read_field($field) {

This is not really an API change. As someone tried to explain before, the & that had been there was not really doing anything, because $field is an object, and objects are always passed by reference in PHP. So this is unnecessary.

Other than that type of thing, I think the formatting here looks fine.

We still need to get buy-in by the core dev team that they are willing to maintain the practice going forward of adding these @since tags for after-release API additions and changes. And write up the standard and add it to node/1354. And get the API module to support this, which is the easy part.

I feel rather awkward saying this after there's been so much discussion... and so much work on patches :/

But at least with the way my API site is set up, every branch is built from a directory, and that directory is a git checkout. I assume api.d.org uses a git checkout too as a source for the documentation so it can be easily updated.

When I look at a function, it says: "includes/menu.inc, line 1158". That's the line that actually contains the function declaration. So the api module knows the line number already.

So all you'd need to do is stand in the directory of the branch that the function came from and say:

$git blame includes/menu.inc -L 1158,+1 -s
c9de4646 1158) function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail = FALSE) {

That gives me the commit ID that added that function declaration, as xjm pointed out earlier. I can't see a way to get just the SHA returned, but that's easily plucked out as it's the first 8 characters.

Follow that with:

$git describe --contains c9de4646
7.0-beta1~140

And that gives me the first tag that contains the commit, with the number of commits between the two. Strip the ~xxx suffix to get just the tag name.

Thank you joachim. We should put that on #1456460: Display point release versions on api.drupal.org using the @since tag (I'll go do that now).

Similar to joachim, I keep these in my (mac) bashrc:

next_tag() { eval "git describe --contains $1 | sed -E 's/(.+)~(.+)$/\1 minus \2 commits/'"; }
prev_tag() { eval "git describe --tags $1 | sed -E 's/(.+)-(.+)-g.+$/\1 plus \2 commits/'"; }

One shows how many commits this hash is after the previous tag, and the other how many commits are after the hash before the next tag.

I spot checked a couple of things in the patch with joachim's commands. The first few checked out fine. Then I got to:

-- a/includes/database/mysql/database.inc
+++ b/includes/database/mysql/database.inc
@@ -152,6 +152,10 @@ class DatabaseConnection_mysql extends DatabaseConnection {
 
   /**
    * Overridden to work around issues to MySQL not supporting transactional DDL.
+   *
+   * @since Function added in 7.4.
+   * @since Function changed from public in 7.8.
+   * @since Function renamed from popTransaction in 7.8.
    */
   protected function popCommittableTransactions() {

The result in this case told me the answer was 7.8. So I guess maybe this isn't workable... joachim, any advice on this? I think manual [EDIT]generation of @since tags[/EDIT] may be the only answer.

In other words, I think that git blame tells you when that particular line in the file adopted its present form (or its present form in the branch you have currently checked out probably?). Meaning that if arguments were added/changed (even just a name change), git blame will point to that commit, not the commit that first introduced the function. Right?

Drat! Curse breakfast optimism! However, I did see something in passing this morning about git log searching for a string.

Here's something, though it was very slow to run!

$ git log -S'function theme_links'
commit 1f273dac01429237ada147633138266a490ec378
Author: Dries Buytaert <dries@buytaert.net>
Date:   Sat Nov 8 11:56:33 2003 +0000

    - Committed stage 1 of the theme system improvements.
    
      Some TODO's:
       * The sidebars of theme Marvin and theme UnConeD look unstyled.
       * CSS-ify theme UnConeD so we can remove the class around the functions.
       * Rewrite theme_init() not to use a class and simplify theme()
         accordingly.

That shows me when theme_links() was added :)

Considerably quicker: tell git log to only go as far back as the tag for 7.0:

$ git log -S'function ajax_prepare_response' 7.0..
commit e830c0e095131281e5b8b5aa8ac03ef7fa24583d
Author: Angie Byron <webchick@24967.no-reply.drupal.org>
Date:   Thu Jan 27 23:55:38 2011 +0000

    #1018714 by effulgentsia: Fixed Image Upload Widget Not Working in IE8

An empty result means the function existed prior to that.

Well, it doesn't look like we can really automate everything we need with git, and also drumm posted to #839904: Add "blame" - history of either the function or every line that we should not count on api.drupal.org using git anyway. So, it's back to manual patching.

So.

We have a patch (or at least a start to a patch). We still don't have buy-in that I'm aware of from the key core maintainers that they want this to be a coding standard. The next thing we need to do is agree to the standard. The latest proposal is in comment #20, and as far as I know, no one has agreed to it yet. Thoughts?

OK, well now we know the scope of the problem -- quite a bit larger than what was being discussed before (I think the estimate was "just a few" changes would need to be documented, and this is definitely more than a few).

So... Here is the path to go forward on this idea:

a) Adopt a documentation standard. Proposal:
------------------
For any new hooks, functions, methods, constants, etc. that are added to a branch after the first full release on that branch, and that are API functions (i.e., intended for use by module/theme developers, meaning any function whose name does not start with an underscore), add the following to the documentation header, at the bottom:

 * @since Function added in 7.2.

(replace Function with Hook, Constant, etc. if something other than a function was added).

If an API function/method had a new argument added to it after the first full release on that branch, use:

 * @since Argument $foo added in 7.10.

Note that the developer adding the new function, hook, etc. may not know what version their patch will be added to. So, they can use the following syntax:

 * @since Function added in {UNKNOWN}

At the time the patch is committed, the committer should know what the next release version will be, and can change the {UNKNOWN} to the correct version number.
-------------

b) If (a) is adopted by the community, review the patch in #46 in detail to make sure the specific @since tags are appropriate, and get it committed for Drupal 7.

c) If (a) is adopted, get a fix for #1456460: Display point release versions on api.drupal.org using the @since tag into the API module.

I'll put this into the Issue Summary.

create an issue summary

I think internal (_-start) functions and private methods on classes should not be included. They are not meant to be used by other developers, and are understood to be subject to change. Agreed on your patching strategy though. :)

Anyway, do we have any votes for/against the standard, or suggestions on how to change it? I am agnostic and not voting one way or the other.

We need to revive this issue and make a decision.

There is a proposal in #47 (and the issue summary) that every new function/method or new function/method argument added after the 7.0 release will get a @since tag added to its documentation, and that going forward, we would continue to do this throughout the 7.x lifetime (and similarly with 8.x after 8.0 is released, etc.).

As noted in #46 , we are looking at something like 125 @since tags added as of Drupal 7.12. When we started discussing this in IRC, we had thought it was something more on the order of 10 or 20 changes, so this has a much larger scope that was originally anticipated. It also means that probably when we start a new major branch, we should get rid of all existing @since tags... yet another thing on the unwritten (I think) list of how to start a new major branch of Drupal...

So... Any votes up or down on whether we should do this? I am willing for this to be adopted if the branch maintainers are willing to help make sure it happens whenever this type of change goes into a post-release branch. Without this being enforced, it will be spotty and I think much less useful, so my tentative vote is no (but I'm willing to be convinced otherwise if the community thinks the pain of doing it is worth the value we get from having it done, and is committed to making sure it happens).

That's a lot of manual work to put those in and then take them out of 8.x. I still think the way forward is with automation. Drumm's objection (on #839904: Add "blame" - history of either the function or every line) to having git on api.d.o was:

> And I don't always want API to need Git. Localize.drupal.org grabs tarballs and parses those, which I think good for API parsing more projects.

Which is fair enough -- API module shouldn't *need* git just to run. But couldn't we need git for more advanced fancy features such as this one?

RE #53 - The first question is whether we want to do this at all -- let's focus on that first.

RE #52 - This issue is about making "since" information available on api.drupal.org. We can't get anything approaching the information in #50 out of git blame in an automated way (right???), and as noted in #52, we can't even count on getting *any* git information on api.drupal.org (even if the API module might theoretically support it), so I think our choice is either do the @since tag or don't have the information available on api.drupal.org.

My feeling is that the @since tag, given that there are so many changes post-release, is hard to maintain, and that if it's not well-adhered-to, it's not going to be worth doing at all. Any other votes on that?

Nice framing of the question tsvenson! Too bad I don't have answers... anyone else?

> We can't get anything approaching the information in #50 out of git blame in an automated way (right???)

Hmm... that sounds like a challenge ;)

For menu_link_save():

$ git log -S'menu_link_save' 7.0..

Gets me a list of commits that touched that function (could probably do to search more specifically). Pass them to perl and iterate over them...

$ git show f2d2cf8a85df17c6c3bef236a8db147739529259 | grep '^[-+]function menu_link_save'

output is:

-function menu_link_save(&$item) {
+function menu_link_save(&$item, $existing_item = array(), $parent_candidates = array()) {

That tells me the function args have changed. Now find the tag:

$ git describe --contains f2d2cf8a85df17c6c3bef236a8db147739529259

Says:

7.6~18

I can now tell you that menu_link_save() acquired two params for 7.6.

Now for classes... that's going to be a whole lot harder.

> How do I need to setup my local dev environment, incl. do I need git clones of everything, to get this working?

Yup. I am issuing those commands in a git clone of core on the 7.x branch.

> Will it be possible to automate checking the core compatibility for which 7.x version my module requires?

Ah I hadn't seen about coder checking stuff... I was working on just showing output on api.d.org. Presumably coder would check every function call in the module it's checking, then check with api.d.org to see when those functions were added (since coder module can't guarantee it has an up to date copy of core locally). If we're outputting machine-friendly data on api.d.org, no reason we can't add stuff we generate rather than just what's in the source.

> If I understand correct, with the blame approach I need to "manually" query about when something was added

I'm not entirely sure what you mean, but I think the answer is 'yes' :)
The way I was seeing this would work is that when api.d.org processes functions to figure out things like the callers, implementations, callbacks, etc, it can also execute the commands to examine the git history and deduce the @since data, and then store that for output. Perhaps I'm on the wrong page here though?

I think we all agree that developers occasionally need this information, or rather that the users of their modules might need to know this information (although of course no one should, in principle, be running old versions of Drupal 7.x if a security release has made them obsolete).

What we don't agree upon is whether the burden should be put on the individual developer (use git blame to figure it out) or the core development maintainers and team (use @since to document it).

Given that our policy is that if there is a security release, everyone should be running the latest version anyway, and that nearly all Drupal Core releases are security releases, how much of a problem is this really? I'm more and more inclined to think it's a really large burden on the core dev/maintainer team, and really not a problem for the developer or user as long as they are using the latest version anyway...

I'm ok with this, but

- The sentence format is way too verbose. I expect @since 7.8 only.

- The previous point inherently means that this won't work for added/removed/changed arguments, which is an acceptable level of detail at which to stop.

- As this is purely historical documentation (and normally our docs in code only document current code) I rather make the compromise between overly verbose @since values and usefulness.

- Speaking of benefits of this, I'm not that excited about potential uses. The only reasonable usage I can see would be by Coder review module. But even that requires relatively complex and possibly longer running code parsing logic to actually leverage this data. For all other use-cases that have been explained to me, I don't see the average module developer Joe take notice of the @since information (and the rather complex core-contrib version dependencies scenario/problem) in the first place.

Is this still relevant?

It didn't seem to get much momentum. For now closing as Won't Fix. We can reopen later if people want to do it for instance for 8.1.x vs. 8.0.x., but I personally think that api.drupal.org and/or git history will tell us this without manually needing to add an @since tag.