http://api.drupal.org/api/function/drupal_deliver_html_page/7
This page has a see also that references drupal_deliver_page()

However the parentheses are missing on drupal_deliver_page(), so the function is appearing as plain text rather than a link.

CommentFileSizeAuthor
#44 fixAtSeeV3.patch56.63 KBaspilicious
#39 fixAtSeeV2.patch57.05 KBaspilicious
#36 fixAtSee.patch48.04 KBaspilicious
#31 broken_see_31.patch2.64 KBtrevjs
#28 file-inc28b.patch518 bytestrevjs
#28 menu-inc-28.patch609 bytestrevjs
#22 681538new.patch7.06 KBjhodgdon
#14 681538_followup_14.diff98.32 KBaspilicious
#11 681538_followup_11.diff89.08 KBaspilicious
#8 681538_followup_7.patch5.01 KBaspilicious
#5 681538_followup_5.patch5.01 KBaspilicious
#1 681538.patch896 bytesjhodgdon

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 681538.patch896 bytes

Here's a patch. Also brings doc into compliance with doc header standards.

dries’s picture

dries commented

There are many other possible culprits (ignore the many false positives):

deimos:drupal-cvs dries$ grep -r "@see" . | grep -v "()" | grep -v ".php" | grep -v "Interface" | grep -v "http://"

./includes/.#common.inc.1.1064: * @see drupal_deliver_page
./includes/.#common.inc.1.1064: * @see drupal_page_header
./includes/.#common.inc.1.1064: * @see element_info('page')
./includes/.#common.inc.1.1064: * @see forum.info
./includes/.#common.inc.1.1064: * @see garland.info
./includes/.#common.inc.1.1064: * @see DrupalEntityController
./includes/.#common.inc.1.1064: * @see DrupalDefaultEntityController
./includes/.#theme.inc.1.561: *     @see MARK_NEW, MARK_UPDATED, MARK_READ
./includes/.#theme.inc.1.561: * @see drupal_render_page
./includes/.#theme.inc.1.561: * @see template_process_page
./includes/actions.inc: * submit functions using \@see. Conversely, form, validate, and submit
./includes/actions.inc: * functions should reference the action function using \@see. For examples of
./includes/batch.inc: * @see form.inc
./includes/cache.inc: * @see DrupalDatabaseCache
./includes/common.inc: * @see drupal_page_header
./includes/common.inc: * @see element_info('page')
./includes/common.inc: * @see forum.info
./includes/common.inc: * @see garland.info
./includes/common.inc: * @see DrupalEntityController
./includes/common.inc: * @see DrupalDefaultEntityController
./includes/common.inc.orig: * @see drupal_deliver_page
./includes/common.inc.orig: * @see drupal_page_header
./includes/common.inc.orig: * @see element_info('page')
./includes/common.inc.orig: * @see forum.info
./includes/common.inc.orig: * @see garland.info
./includes/common.inc.orig: * @see DrupalEntityController
./includes/common.inc.orig: * @see DrupalDefaultEntityController
./includes/database/database.inc:   * @see SelectQuery
./includes/database/database.inc:   * @see InsertQuery
./includes/database/database.inc:   * @see MergeQuery
./includes/database/database.inc:   * @see UpdateQuery
./includes/database/database.inc:   * @see DeleteQuery
./includes/database/database.inc:   * @see TruncateQuery
./includes/database/database.inc:   * @see DatabaseTransaction
./includes/database/database.inc:   * @see DatabaseTransaction
./includes/database/database.inc:   * @see DatabaseTransaction
./includes/database/database.inc:   * @see DatabaseTransaction
./includes/database/database.inc:   * @see DatabaseLog
./includes/database/database.inc:   * @see DatabaseLog
./includes/database/database.inc: * @see SearchQuery
./includes/database/schema.inc:   *   or index including it in this array. @see db_change_field for more
./includes/file.inc: * @see
./includes/file.inc.orig: * @see
./includes/form.inc: * functions using \@see. Conversely, validate and submit functions should
./includes/form.inc: * reference the form builder function using \@see. For examples, of this see
./includes/form.inc: *     @see theme_table
./includes/form.inc.orig: * functions using \@see. Conversely, validate and submit functions should
./includes/form.inc.orig: * reference the form builder function using \@see. For examples, of this see
./includes/form.inc.orig: *     @see theme_table
./includes/menu.inc: *   - options     An array of options, @see l for more.
./includes/theme.inc: *     @see MARK_NEW, MARK_UPDATED, MARK_READ
./includes/theme.inc: * @see drupal_render_page
./includes/theme.inc: * @see template_process_page
./includes/theme.inc.orig: *     @see MARK_NEW, MARK_UPDATED, MARK_READ
./includes/theme.inc.orig: * @see drupal_render_page
./includes/theme.inc.orig: * @see template_process_page
./includes/updater.inc:   *   @see self::getInstallArgs
./includes/updater.inc:   *   @see self::getInstallArgs
./misc/drupal.js: * @see Drupal.attachBehaviors
./modules/node/node.test:   * @see TaxonomyNodeFilterTestCase
./modules/node/node.test.orig:   * @see TaxonomyNodeFilterTestCase
./modules/overlay/overlay-parent.js:  // @see overlay-parent.css .overlay-loaded #overlay-element
./modules/overlay/overlay-parent.js:  // @see https://bugs.webkit.org/show_bug.cgi?id=19033
./modules/overlay/overlay-parent.js: * @see Drupal.overlayChild.behaviors.addClickHandler
./modules/overlay/overlay-parent.js.orig:  // @see https://bugs.webkit.org/show_bug.cgi?id=19033
./modules/overlay/overlay-parent.js.orig: * @see Drupal.overlayChild.behaviors.addClickHandler
./modules/simpletest/tests/database_test.test:   * @see FakeRecord
./modules/simpletest/tests/mail.test:   * @see DefaultMailSystem
./modules/system/system.admin.inc: * @see drupal_parse_info_file for information on module.info descriptors.
./modules/system/system.admin.inc.orig: * @see drupal_parse_info_file for information on module.info descriptors.
./modules/system/system.module: * @see FileTransfer
./modules/system/system.module: * @see system_authorized_init
./sites/all/modules/mollom/tests/mollom.test:   * @see mollom_test.module
dries’s picture

dries commented
Title: drupal_deliver_html_page() has see also that's missing link » Possibly broken @see also links
Status: Needs review » Needs work

I committed the patch from #1 to CVS HEAD. Thanks a lot.

I guess I'll leave this as 'needs work' because of #2?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Sounds like a plan.

If someone wants to take a look at patching this, it would be a good project for a novice patchmaker. :)

I'm not sure what we're doing about displaying classes on api.drupal.org or documenting them. Anything in CamelCase in the above list is likely a class, so don't put parentheses on those. However, things like self::installArgs are probably member functions from classes and probably need parens.

Items in ALL_CAPS are constants and should also be left as-is.

File names like file.info are OK as is.

Obviously, links are OK too.

aspilicious’s picture

aspilicious commented
StatusFileSize
new 681538_followup_5.patch5.01 KB

I made a patch with following changes:

@see drupal_page_header ==> @see drupal_page_header()
@see db_change_field ==> @see db_change_field()
@see theme_table ==> @see theme_table()
@see drupal_render_page ==> @see drupal_render_page()
@see template_process_page ==> @see template_process_page()
@see self::getInstallArgs ==> @see self::getInstallArgs() (2 times)
@see drupal_parse_info_file ==> @see drupal_parse_info_file()
@see system_authorized_init ==> @see system_authorized_init()

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review

start the testbot

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Good try... There's one little problem: lines like this are still not OK:

- * @see drupal_parse_info_file for information on module.info descriptors.
+ * @see drupal_parse_info_file() for information on module.info descriptors.

This is what it results in: http://api.drupal.org/api/function/system_modules/7

@see needs to be on its own line, not embedded in other text. Those two places where @see is in the middle of a paragraph, the @ should be removed removed so it's just the word "see", and the () needs to be added to the function so it will make a link in-line with the paragraph.

aspilicious’s picture

aspilicious commented
StatusFileSize
new 681538_followup_7.patch5.01 KB

This one better?

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review

start testbot

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Yes... except that all @see lines really should be at the end of each doc header, because they make a section when rendered on api.drupal.org.

So for instance:

    * @param array $overrides
    *   An array of settings to override defaults
-   *   @see self::getInstallArgs
+   *   @see self::getInstallArgs()
    * @return array

This should probably also be just a plain See rather than an @see, because we don't really want a See Also header in the middle of the Parameters section.

Also, there are numerous other problems with the doc around these @see links, such as that it should be wrapped at 80 lines and there are grammar problems... Sigh. If I were making this patch, I would fix the doc up in each doc header that was being fixed, but I'm willing to let it go if you will just make sure that @see is only used at the very end of each doc header.

So if the @see is particular to a section, change it to just See. If it's a reference to a similar/related function, put it at the end. Does that make sense?

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review
StatusFileSize
new 681538_followup_11.diff89.08 KB

I made a massive patch that NEEDS review cause I wasn't sure about everything I did...
Took me 90 minutes....

Check for trailing white spaces or the use of capital in see/See

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Wow, thanks for all that work. Needs a few small fixes, but I think it's almost there:

a) There are several places where you changed @see to "see" and it needed to be "See" because it was the beginning of a new sentence. For instance this password.inc segment:

  * Based on the Portable PHP password hashing framework.
- * @see http://www.openwall.com/phpass/
+ * see http://www.openwall.com/phpass/

Also, in these cases, The See (whatever) sentences should flow from the previous paragraph, and not be on their own lines. Wrap at as close to 80 characters as possible.

b) This looks a bit funny to me -- not sure why we're linking to php.net in one place and realpath() in another? Also again see needs to be See, and there are some wrapping problems:

  * scheme or the wrapper implementation does not implement realpath, then
  * FALSE will be returned.
  *
- * @see http://php.net/manual/en/function.realpath.php
+ * see http://php.net/manual/en/function.realpath.php
  *
  * Compatibility: normal paths and stream wrappers.
- * @see http://drupal.org/node/515192
+ * see http://drupal.org/node/515192
  *
  * @param $uri
  *   A string containing the URI to verify. If this value is omitted,
@@ -1815,8 +1815,8 @@
  * @return
  *   The absolute pathname, or FALSE on failure.
  *
- * @see realpath()
  * @ingroup php_wrappers
+ * @see realpath()
  */
 function drupal_realpath($uri) {

c) You weren't consistent about whether you put a blank line between the @ingroup and the @see. I think there should be, but the standards are not clear (http://drupal.org/node/1354). At least be consistent within your patch. :)

aspilicious’s picture

aspilicious commented

The code is just not consistent in the first place ;)
Cause in some places every @statement is put against eachother, in others they are seperated!

aspilicious’s picture

aspilicious commented
StatusFileSize
new 681538_followup_14.diff98.32 KB

I made it more consistent and changed some rules I found in between.
But I think it's best to open 2 new topics:

1) checking 80 characters a line for comments
2) a white line between the @xxx blocks
==> white line between @param and @return for example

I don't rly know what you mean with something wrong with wrapping.

I'm gonna go on a skiing holiday tomorow, so if this isn't good enough, reply fast. Else you gotta do it yourself ;)

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review

...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Well, that is true. There are a lot of places where Drupal does not follow its standards. What we try to do is whenever a new doc patch is put in, improve the situation rather than making it worse or leaving it as out of compliance.

You might want to read the standards doc, if you haven't already: http://drupal.org/node/1354

aspilicious’s picture

aspilicious commented

I read it but you need to change A LOT so it's to difficult to do it at once (due to constantly changing head). I'm prety sure I fixed (almost?) every see in core. So why not commit this and when I'm back I'll look on those other issues

aspilicious’s picture

aspilicious commented

NOOOOOOOOOOOOOOOOO

====>

Second, I found that although the documentation suggests using @see over several lines is ok

@see drupal_render()
@see hook_theme()

It actually results in strange results on the finished page. Multiple @see should be written @see drupal_render(), hook_theme(). I don't think this is just me, you can see this error on, for example, http://api.drupal.org/api/group/themeable/6.

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review

Stil need review, what do you think about the problem stated in 18

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Regarding #18, the consecutive @see rendering issue is a known issue in the API module and is being worked on. It's all over Drupal, and also is the current coding standard to do it that way.

Regarding #17, we don't generally commit patches that remove one error and introduce new errors. I'm not comfortable committing the patch in #14 as it is.

We can go back to the patch in #8, though: just fix those few functions up to conform to standards, rather than the huge patch you are trying in #17 that tries to fix all @see related problems in the entire code base. That is the kind of incremental change that is OK to commit, because it fixes things and doesn't introduce new problems.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Forgot status change. We can't commit the patch in #14.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 681538new.patch7.06 KB

This patch goes back to the patch in #8 and fixes up some additional problems in those doc headers.

dries’s picture

dries commented
Status: Needs review » Needs work

Committed the patch in #22. Let's figure out what other changes need to be incorporated from patches above. Thanks for all the hard work, folks.

trevjs’s picture

trevjs commented

Re-ran Dries's grep, a lot of these are references to files or class functions:

trev@Ubuntu:/var/www/dev/drupal-7.x-head/drupal-cvs$ grep -r "@see" . | grep -v "()" | grep -v ".php" | grep -v "Interface" | grep -v "http://"
./misc/drupal.js: * @see Drupal.attachBehaviors
./includes/batch.inc: * @see form.inc
./includes/database/database.inc: * @see SelectQuery
./includes/database/database.inc: * @see InsertQuery
./includes/database/database.inc: * @see MergeQuery
./includes/database/database.inc: * @see UpdateQuery
./includes/database/database.inc: * @see DeleteQuery
./includes/database/database.inc: * @see TruncateQuery
./includes/database/database.inc: * @see DatabaseTransaction
./includes/database/database.inc: * @see DatabaseTransaction
./includes/database/database.inc: * @see DatabaseTransaction
./includes/database/database.inc: * @see DatabaseTransaction
./includes/database/database.inc: * @see DatabaseLog
./includes/database/database.inc: * @see DatabaseLog
./includes/database/database.inc: * @see SearchQuery
./includes/common.inc: * @see element_info('page')
./includes/common.inc: * @see forum.info
./includes/common.inc: * @see garland.info
./includes/common.inc: * @see DrupalEntityController
./includes/common.inc: * @see DrupalDefaultEntityController
./includes/form.inc: * functions using \@see. Conversely, validate and submit functions should
./includes/form.inc: * reference the form builder function using \@see. For examples, of this see
./includes/cache.inc: * @see DrupalDatabaseCache
./includes/menu.inc: * - options An array of options, @see l for more.
./includes/theme.inc: * @see MARK_NEW, MARK_UPDATED, MARK_READ
./includes/file.inc: * @see
./modules/node/node.test: * @see TaxonomyNodeFilterTestCase
./modules/system/system.module: * @see FileTransfer
./modules/simpletest/tests/mail.test: * @see DefaultMailSystem
./modules/simpletest/tests/database_test.test: * @see FakeRecord
./modules/overlay/overlay-parent.js: // @see overlay-parent.css .overlay-loaded #overlay-element
./modules/overlay/overlay-parent.js: // @see https://bugs.webkit.org/show_bug.cgi?id=19033
./modules/overlay/overlay-parent.js: * @see Drupal.overlayChild.behaviors.addClickHandler

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Of those, the only ones I think need fixing:
./includes/common.inc: * @see element_info('page')
./includes/menu.inc: * - options An array of options, @see l for more.
./includes/file.inc: * @see

The rest are file names, class names, constants, or URLs I think.

That second one should be "- options: An array of options, see l() for more."
That last one should probably be removed, but you'll need to look at the file to see what's up there.
The first one I don't think will turn into a link with the 'page' in there, but you could look at api.drupal.org to see if I'm wrong about that.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

You might also take a look at the patch in #14, with the comments in #12 and after #14.

Hmmm... The link is not working for me, but the patch is here:
http://drupal.org/files/issues/681538_followup_14.diff

trevjs’s picture

trevjs commented
Assigned: Unassigned » trevjs

Talked with jhodgdon about this in irc. I should be able to take care of what is left.

trevjs’s picture

trevjs commented
StatusFileSize
new menu-inc-28.patch609 bytes
new file-inc28b.patch518 bytes

Here is 2 of the 3

trevjs’s picture

trevjs commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Just FYI - we usually try to make 1 patch per issue -- make one patch that combines all the files together. You can probably just use a text editor to cram them into one file, because it looks like you're not using CVS to make your patches?

Anyway, that aside...

The menu.inc patch -- You don't want to use @see in the middle of a line. Putting @see in a doc header produces a See Also section in the doc, so it needs to be on its own line. So that @see should be replaced with just the word "see".

Also, that menu.inc doc section is broken. I think it's trying to create a list, but it's not doing it the right way.
See http://drupal.org/node/1354#general (Lists section)
It would be lovely if you could fix that along with the @see fix.

trevjs’s picture

trevjs commented
Status: Needs work » Needs review
StatusFileSize
new broken_see_31.patch2.64 KB

Lets see if this takes care of those three.

What am I looking for in the patch from 14?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

The patch in #31 looks good to me.

Regarding the patch from #14/#26, there may be some additional broken @see spots in that patch too that haven't yet been fixed. I'm not sure though.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Unfortunately someone (sigh) also decided to file a separate issue on this exact same subject:
#748550: Fix incorrect usage of @see directive.

aspilicious’s picture

aspilicious commented

#31 and the patch in #748550: Fix incorrect usage of @see directive. are not rly the same

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

That is true, but I think the other item is expanding on your earlier patch from #14, isn't it? So I'm not sure why you filed another issue?

aspilicious’s picture

aspilicious commented
StatusFileSize
new fixAtSee.patch48.04 KB

Get ready for a monster patch.
I included the fixes of #748550: Fix incorrect usage of @see directive. and marked that topic as duplicated of this one.

What to do with these matches:

C:\Users\bram\workspace\drupal\includes\form.inc
      11   * functions using \@see. Conversely, validate and submit functions should
      12   * reference the form builder function using \@see. For examples, of this see
      17   * \@see user_pass_validate().
      18   * \@see user_pass_submit().

I don't want to reroll this to much :(

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Comments on the patch:

a) menu.inc section - if you are going to get that up to standards, you could also put in a blank line before the @return. :)

b) There are a couple of spots like this, where @see -> See, and the result is a sentence, so needs a period at the end now:

+ *     See MARK_NEW, MARK_UPDATED, MARK_READ

c) Along that line, all // comments within code are supposed to end in . and be wrapped at 80 characters. A couple of the fixed ones now don't.

d) Wrapping:

  *       is set, the cache ID is created automatically from these keys.
- *       @see drupal_render_cid_create()
+ *       See drupal_render_cid_create()

See should be on the line above.

Farther down:

 * Available variables:
  * - $header: The table header. This is pre-generated with click-sorting
- *   information. If you need to change this, @see template_preprocess_forum_topic_list().
+ *   information. If you need to change this, see template_preprocess_forum_topic_list().

Is this longer than 80 characters? Not sure, just looking at the patch...

e) I don't think this is a good idea:

 /**
- * @see registry_update().
+ * @see registry_update()
  */
 function _registry_update() {

Functions need to start with a plain text (i.e. no @directive) one line description. So this should be "See registry_update()." rather than using @see. Or better yet: "Does the work for registry_update()." so that it describes what the function does.

f) Missed one place to split into two @see:

+ *
  * @see file_test_get_calls() and file_test_reset()
  */
 function _file_test_log_call($op, $args) {

(actually, the next couple of these are the same)

g) This should be on two lines:

 /**
- * Return only visible regions. @see system_region_list().
+ * Return only visible regions. See system_region_list().
  */
 define('REGIONS_VISIBLE', 'visible');

Same with the next patch section.

h) field.api.php section: Needs blank line before each @see block.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Regarding the form.inc stuff in #36 -- leave it as-is, since it is documentation on how to document form builder functions. Supposedly/hopefully, the \ before @see will escape the @.

aspilicious’s picture

aspilicious commented
StatusFileSize
new fixAtSeeV2.patch57.05 KB

Second try, hopefully this one is OK (but I doubt it :) )

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, fixAtSeeV2.patch, failed testing.

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review
Issue tags: +Novice

#39: fixAtSeeV2.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Looks mostly good!

A couple of items still:

a)

- * For details on how this is used @see node_feed()
+ * For details on how this is used, see node_feed()

Probably needs . at end of sentence here?

Here also:

+  // so that the template doesn't have to worry about it. See 
+  // theme_rdf_template_variable_wrapper()
   if (!empty($variables['rdf_template_variable_attributes_array'])) {

And here:

   // Give everyone full grants so we don't break other node tests.
   // Our node access tests asserts three realms of access.
-  // @see testGrantAlter()
+  // See testGrantAlter()
   return array(

b) WTF?

/**
- * Return only visible regions. @see system_region_list().
+ * Return only visible regions. 
+ *
+ * @see system_region_list()
  */
 define('REGIONS_VISIBLE', 'visible');
 
 /**
- * Return all visible regions. @see system_region_list().
+ * Return all visible regions. 
+ *
+ * @see system_region_list()
  */
 define('REGIONS_ALL', 'all');

Probably the REGIONS_ALL flag means "Return all regions." not "visible"? Otherwise it looks like it is the same as REGIONS_VISIBLE? Someone should verify this...

c) You should remove the .project section at the end of the patch file... :)

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review
StatusFileSize
new fixAtSeeV3.patch56.63 KB

a) fixed

b) fixed (I guess you're right, we have to leave visible away)

c) ... eclipse sneaked that file in....

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Note on (b) -- I looked at the code using this flag, and definitely REGIONS_ALL does mean to return all regions (not just the visible ones) in system_region_list().

Nice work, and thanks for your persistence!

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed this monster patch. Great job. :)

Status: Fixed » Closed (fixed)
Issue tags: -Novice

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Novice

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.