Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#32139 closed enhancement (fixed)

Add PHPDoc for all global $wp_query in /wp-includes/query.php

Reported by: mikeschinkel's profile MikeSchinkel Owned by: drewapicture's profile DrewAPicture
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Query Keywords:
Focuses: docs Cc:

Description

The attached patch will add PHPDoc comments for all instances of global $wp_query in /wp-includes/query.php. This patch does nothing other than add/change documentation; it has no effect on runtime logic.

In addition to improving code documentation this change will also benefit developers using PhpStorm:

  1. Method calls will no longer be flagged as "not found" in PhpStorm
  1. Developers will be able to use the "Navigate To" functionality found in PhpStorm to jump directly to the declarations of the WP_Query method while using PhpStorm.

In addition to PHPDoc for $wp_query PhpStorm flagged several functions that had return statements where no value was being returned by the expression because it the functions where returning the value of a void method call. I removed the return statements and any erroneous @return PHPDoc comments from these three (3) functions:

  1. set_query_var()
  2. rewind_posts()
  3. the_comment()

After these changes PhpStorm no longer flags any errors in the file and is now (fully?) navigable.

Attachments (2)

32139.diff (7.4 KB) - added by MikeSchinkel 9 years ago.
Patch adds PHPDoc for all global $wp_query in /wp-includes/query.php plus fixes return statements+PHPDoc on three (3) void functions.
32139-2.diff (7.2 KB) - added by MikeSchinkel 9 years ago.
Removes an inadvertent change found in the first patch.

Download all attachments as: .zip

Change History (17)

@MikeSchinkel
9 years ago

Patch adds PHPDoc for all global $wp_query in /wp-includes/query.php plus fixes return statements+PHPDoc on three (3) void functions.

#1 @MikeSchinkel
9 years ago

  • Keywords has-patch dev-feedback added

@MikeSchinkel
9 years ago

Removes an inadvertent change found in the first patch.

#2 follow-up: @jdgrimes
9 years ago

Isn't documenting the value in a million places kind of overkill? It only needs to be documented once aand PHPStorm will pick it up everywhere else. (BTW, I'd like to see $wpdb and other globals documented too.)

Last edited 9 years ago by jdgrimes (previous) (diff)

#3 in reply to: ↑ 2 ; follow-up: @MikeSchinkel
9 years ago

Replying to jdgrimes:

It only needs to be documented once aand PHPStorm will pick it up everywhere else.

That is not my experience, but you may know something I do not. I just verified that if I remove one of those declarations PhpStorms does not recognize the type for the method with the missing PHPDOc, as should be expected for local scoping.

How would you document them differently? Is there a way to set global scoping in one place that PhpStorm will recognize?

(BTW, I'd like to see $wpdb and other globals documented too.)

I would too. And I'd be happy to submit the patch(es), but I posted this ticket first to see if those on the core team will accept these types of patches. If they won't, documenting the other globals would just be a waste of time.

#4 in reply to: ↑ 3 ; follow-up: @jdgrimes
9 years ago

Replying to MikeSchinkel:
Just the other day I was getting tired of the lack of auto-completion for $wpdb, so I created a file in the PHP path with this in it:

<?php

/** @var wpdb $wpdb */
global $wpdb;

I now have autocompletion for $wpdb everywhere.

#5 in reply to: ↑ 4 ; follow-up: @MikeSchinkel
9 years ago

Replying to jdgrimes:

So I modified core and added a file /wp-includes/phpdoc/globals.php. Then just after the call to wp_initial_constants() in /wp-settings.php I added the following code:

set_include_path( get_include_path() . ':' . ABSPATH . WPINC . '/phpdoc' );
require( ABSPATH . WPINC . '/phpdoc/globals.php' );

In /phpdoc/globals.php I added the following:

<?php
/**
 * @var WP_Query $wp_query
 */

I then ran PhpStorm to the line after the require( ABSPATH . WPINC . '/phpdoc/globals.php' ); in the debugger and navigated to /wp-includes/query.php but PhpStorm did not recognize the type of $wp_query in that file.

Did I do something wrong? Asked another way, what patch can we (you & I) propose that will allow PhpStorm to recognize well-known global variables in WordPress?

(BTW, doing it with the PHP path seems like just a hack for PhpStorm; documenting every location at the source seems more like the correct thing to do since it is not just for PhpStorm, no?)

#6 in reply to: ↑ 5 ; follow-up: @jdgrimes
9 years ago

Replying to MikeSchinkel:

In /phpdoc/globals.php I added the following:

<?php
/**
 * @var WP_Query $wp_query
 */

I think you have to add global $wp_query right under there.

(BTW, doing it with the PHP path seems like just a hack for PhpStorm; documenting every location at the source seems more like the correct thing to do since it is not just for PhpStorm, no?)

Maybe so. It just seems like a huge amount of redundancy. Especially since it's already pretty easy to guess what class it is anyway ($wpdb -> wpdb, $wp_query -> WP_Query, $wp_filesystem -> WP_Filesystem*, etc.).

#7 in reply to: ↑ 6 ; follow-ups: @MikeSchinkel
9 years ago

Replying to jdgrimes:

So it seems you are correct, though PHP path is unimportant here. They just have to be declared using both PHPDoc @var and global statements while in global scope.

Looking at core it would seem that the most appropriate place for these would be in /wp-includes/vars.php. It we define and declare all well-know WordPress global variables in that file then PhpStorm will recognize them throughout.

So the question for the core committers comes to this:

  1. Would you prefer well-known WordPress global variables be documented in every function and method where they are found,
  1. Would you prefer to document all well-known WordPress global variables only once, as in /wp-includes/vars.php, or
  1. Will we find that you (the core committers) are simply uninterested in this and that any effort on this will be a waste of time?

If Option 1, I will post more patches for this over time.

If Option 2 I will post a new patch very soon.

If Option 3, well then...

This ticket was mentioned in Slack in #core by mikeschinkel. View the logs.


9 years ago

#9 in reply to: ↑ 7 ; follow-up: @jdgrimes
9 years ago

Replying to MikeSchinkel:

Looking at core it would seem that the most appropriate place for these would be in /wp-includes/vars.php. It we define and declare all well-know WordPress global variables in that file then PhpStorm will recognize them throughout.

I think that would be useful for everyone, just to have a nice list of these globals and what they are in one place, even if they aren't using an IDE with auto-completion.

#10 in reply to: ↑ 9 @MikeSchinkel
9 years ago

Replying to jdgrimes:

I think that would be useful for everyone, just to have a nice list of these globals and what they are in one place, even if they aren't using an IDE with auto-completion.

Definitely! :)

#11 @johnbillion
9 years ago

  • Keywords needs-patch added; has-patch removed
  • Version trunk deleted

#12 @swissspidy
9 years ago

Although PhpStorm 9 now automatically recognizes common WordPress global variables like $wpdb or $wp_query, it would be nice to improve documentation wherever we can.

#13 @wonderboymusic
9 years ago

  • Milestone changed from Awaiting Review to 4.3
  • Resolution set to fixed
  • Status changed from new to closed

[32620] fixed this ticket inadvertently using our current docs standards

#14 in reply to: ↑ 7 @DrewAPicture
9 years ago

  • Keywords dev-feedback needs-patch removed
  • Milestone changed from 4.3 to 4.4
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to MikeSchinkel:

So the question for the core committers comes to this:

  1. Would you prefer well-known WordPress global variables be documented in every function and method where they are found,
  1. Would you prefer to document all well-known WordPress global variables only once, as in /wp-includes/vars.php, or
  1. Will we find that you (the core committers) are simply uninterested in this and that any effort on this will be a waste of time?

Sorry, @MikeSchinkel: didn't see your comment above from a few months ago. I'm going to go with door number 3 for now. I think globals are adequately accounted for in the docs standard currently, and any specific updates largely for the benefit of IDEs and not much else falls pretty low on the priority list.

Replying to wonderboymusic:

[32620] fixed this ticket inadvertently using our current docs standards

I believe our docs standards call for descriptions on @global tags :-) Let's get the descriptions added then we can call this fixed.

#15 @DrewAPicture
9 years ago

  • Owner set to DrewAPicture
  • Resolution set to fixed
  • Status changed from reopened to closed

In 34337:

Docs: Add descriptions for $wp_query global phpDoc references in wp-includes/query.php, partially documented in [32620].

Fixes #32139.

Note: See TracTickets for help on using tickets.